[py] Fix : Mypy type annotation errors - 2

[ShauryaDusht]

User description

🔧 What does this PR do?

This PR fixes several type annotation errors reported by Mypy in the following files:

• selenium/webdriver/common/bidi/browser.py - changes reverted to avoid conflict from another PR fixed errors in browser.py for 15697 #15847
• selenium/webdriver/common/bidi/browsing_context.py
• selenium/webdriver/common/action_chains.py - recently added
• selenium/webdriver/common/utils.py - recently added

---

💡 Additional Notes

• This is part of ongoing efforts to improve type annotations across the Selenium codebase.
• No behavior changes are introduced; only type hints are corrected.
• This is my second PR on the issue.

---

🔄 Types of changes

• Code cleanup (type annotation fixes, no functional changes)

---

PR Type

Bug fix, Enhancement

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Variable Shadowing The variable event_obj is created to replace the shadowing of the parameter event that was happening in the original code. This is a good fix, but should be verified that it doesn't affect any other code that might be relying on the original variable name. event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) Default Values The PR adds default empty string values for string types and 0 for numeric types. Verify that these defaults are appropriate for the application context and won't cause unexpected behavior when API responses are missing fields. context=str(json.get("context", "")), navigation=json.get("navigation"), timestamp=int(json.get("timestamp", 0)), url=str(json.get("url", "")),

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 5eec4b8

Category	Suggestion	Impact
Incremental [*]	✅ Prevent potential KeyError Suggestion Impact: The commit fixed the potential KeyError issue by restructuring the code to safely access self.subscriptions[event_name]. The implementation uses a different approach than suggested but addresses the same issue by ensuring proper checks before accessing the dictionary. code diff: event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) - if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: - self.subscriptions[event_name].remove(callback_id) - if len(self.subscriptions[event_name]) == 0: - params = {"events": [event_name]} - session = Session(self.conn) - self.conn.execute(session.unsubscribe(**params)) - del self.subscriptions[event_name] + if event_name in self.subscriptions: + callbacks = self.subscriptions[event_name] + if callback_id in callbacks: + callbacks.remove(callback_id) + if not callbacks: + params = {"events": [event_name]} + session = Session(self.conn) + self.conn.execute(session.unsubscribe(**params)) + del self.subscriptions[event_name] The code attempts to access self.subscriptions[event_name] after checking if event_name is in self.subscriptions, but then immediately accesses it again on the next line without checking. This could cause a KeyError if event_name is not in self.subscriptions. py/selenium/webdriver/common/bidi/browsing_context.py [716-718] + self.conn.remove_callback(event_obj, callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) ++ if len(self.subscriptions[event_name]) == 0: ++ params = {"events": [event_name]} ++ session = Session(self.conn) ++ self.conn.execute(session.unsubscribe(**params)) ++ del self.subscriptions[event_name] [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This identifies a real bug where self.subscriptions[event_name] is accessed on line 719 without checking if event_name exists in self.subscriptions, which could cause a KeyError. The improved code correctly moves the length check inside the conditional block.	Medium
General	✅ Add warning for unexpected types Suggestion Impact: The commit implemented the exact suggestion by replacing the list comprehension with a for loop that adds a warning when encountering non-dictionary child types in the browsing context. code diff: +import warnings from typing import Any, Callable, Optional, Union from selenium.webdriver.common.bidi.common import command_builder @@ -110,7 +111,12 @@ children = None raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] + children = [] + for child in raw_children: + if isinstance(child, dict): + children.append(BrowsingContextInfo.from_json(child)) + else: + warnings.warn(f"Unexpected child type in browsing context: {type(child)}") The current implementation may silently ignore non-dictionary items in the children list without any warning. Consider adding a warning or log message when encountering unexpected child types to help with debugging. py/selenium/webdriver/common/bidi/browsing_context.py [111-113] raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] + children = [] + for child in raw_children: + if isinstance(child, dict): + children.append(BrowsingContextInfo.from_json(child)) + else: + import warnings + warnings.warn(f"Unexpected child type in browsing context: {type(child)}") [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion adds debugging warnings for unexpected child types, which could be helpful but is not critical. The PR already improved safety by adding isinstance(child, dict) filtering.	Low
Update

---

Previous suggestions

✅ Suggestions up to commit e75da48

Category	Suggestion	Impact
Possible issue	✅ Prevent ValueError on removal Suggestion Impact: The commit directly implemented the suggestion by adding a check to verify if event_name exists in subscriptions and if callback_id exists in subscriptions[event_name] before attempting to remove it code diff: - self.subscriptions[event_name].remove(callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) Check if the callback_id exists in the subscriptions before attempting to remove it. The current implementation will raise a ValueError if the callback_id is not in the list, which could happen if it was already removed or never added. py/selenium/webdriver/common/bidi/browsing_context.py [716] def remove_event_handler(self, event: str, callback_id: int) -> None: """Remove an event handler from the browsing context. Parameters: ---------- event: The event to unsubscribe from. callback_id: The callback id to remove. """ try: event_name = self.EVENTS[event] except KeyError: raise Exception(f"Event {event} not found") event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) - self.subscriptions[event_name].remove(callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) Suggestion importance[1-10]: 7 __ Why: Valid suggestion that prevents potential ValueError when attempting to remove a non-existent callback_id from the subscriptions list, improving error handling robustness.	Medium
	✅ Validate dictionary type before processing Suggestion Impact: The commit directly implemented the suggestion by adding the 'isinstance(child, dict)' check in the list comprehension, exactly as suggested code diff: - children = [BrowsingContextInfo.from_json(child) for child in raw_children] + children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] Add type checking for each child element before calling from_json to prevent runtime errors. The current code assumes each child is a dictionary, but if any non-dictionary value is present in the list, it will cause a runtime error. py/selenium/webdriver/common/bidi/browsing_context.py [111-113] raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children] + children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: Valid suggestion to prevent runtime errors when non-dictionary values are in the children list, though this is likely an edge case in normal usage.	Low

[ShauryaDusht]

Hi @cgoldberg, please review my PR (I have added a few more files in this PR)

[navin772]

@ShauryaDusht another PR (#15847) is resolving the type errors for browser.py file, can you pick another file to avoid conflict?

[ShauryaDusht]

@ShauryaDusht another PR (#15847) is resolving the type errors for browser.py file, can you pick another file to avoid conflict?

Will remove browser.py changes and update the PR with another file. Thanks!

[navin772]

@ShauryaDusht I don't think silently defaulting to a value (like an empty string) when the value is not found is a good idea. This will introduce uncertainty and does not conform to the BiDi spec.
For example, the changes made for UserPromptOpenedParams, default_value is set to an empty string if not found but the spec says:
the defaultValue field set to default value if default value is not null or omitted otherwise
which means it should either be omitted from the payload or should be None.

So, we should do something like this:

		default_value = json.get("defaultValue")
        if default_value is not None and not isinstance(default_value, str):
           raise ValueError("defaultValue must be a string if provided")

Same with other classes and params.

[ShauryaDusht]

Hi @navin772
Please review my changes

[navin772]

@ShauryaDusht I have made some changes as per the BiDi spec.

LGTM!

[ShauryaDusht]

Thanks for the changes and review