[py][bidi]: add event handler support for browsing context

[navin772]

User description

🔗 Related Issues

💥 What does this PR do?

Adds support for event handlers for browsing context bidi module.

🔧 Implementation Notes

Replaces #15902 as discussed in #15902 (comment)

Usage:

1. Adding an event handler:

   events_received = []
   def on_context_created(info):
           events_received.append(info)
   
   callback_id = driver.browsing_context.add_event_handler("context_created", on_context_created)

2. Removing an event handler:

   driver.browsing_context.remove_event_handler("context_created", callback_id)

Get a list of supported event strings:

driver.browsing_context.get_event_names()

Returns:
['context_created', 'context_destroyed', 'dom_content_loaded', 'download_will_begin', 'fragment_navigated', 'history_updated', 'load', 'navigation_aborted', 'navigation_committed', 'navigation_failed', 'navigation_started', 'user_prompt_closed', 'user_prompt_opened']

💡 Additional Considerations

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement

---

Description

• Add event handler support for browsing context BiDi module

• Implement 13 event classes for different browsing context events

• Create _EventManager class for subscription and callback management

• Add comprehensive test coverage for all event types

---

Diagram Walkthrough

flowchart LR
  A["BrowsingContext"] --> B["_EventManager"]
  B --> C["Event Classes"]
  B --> D["Subscription Management"]
  C --> E["ContextCreated"]
  C --> F["NavigationStarted"]
  C --> G["UserPromptOpened"]
  D --> H["add_event_handler()"]
  D --> I["remove_event_handler()"]

Loading

File Walkthrough

	Relevant files
Enhancement	browsing_context.py Implement event handler architecture for browsing context py/selenium/webdriver/common/bidi/browsing_context.py Replace generic BrowsingContextEvent with 13 specific event classes Add _EventManager class for centralized event handling Refactor event subscription/unsubscription logic Add get_event_names() classmethod for available events +272/-119
Tests	bidi_browsing_context_tests.py Add comprehensive event handler test coverage                        py/test/selenium/webdriver/common/bidi_browsing_context_tests.py Add comprehensive tests for all 13 event types Test event handler addition, removal, and multiple handlers Include thread safety and context-specific event tests Add tests for user prompts, navigation, and download events +428/-0

---

[Copilot]

Pull Request Overview

This PR adds event handler support for the browsing context BiDi module in the Python Selenium bindings. It enables users to subscribe to various browsing context events and handle them with custom callback functions.

Key Changes

• Refactored event handling architecture from a simple dictionary to a class-based system with dedicated event classes for each event type
• Introduced _EventManager class to centralize event subscription management and callback tracking
• Added comprehensive test coverage for all supported browsing context events including context creation/destruction, navigation events, user prompts, and downloads

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
py/selenium/webdriver/common/bidi/browsing_context.py	Core implementation with new event classes, EventManager, and refactored event handling methods
py/test/selenium/webdriver/common/bidi_browsing_context_tests.py	Comprehensive test suite covering all event types and edge cases including thread safety and multiple handlers

Comments suppressed due to low confidence (1)

py/selenium/webdriver/common/bidi/browsing_context.py:544

• [nitpick] The error message in the ValueError exception could be more helpful by suggesting how to get available events. Consider mentioning the get_event_names() method.

        if not event_config:

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 1234 - Not compliant Non-compliant requirements: • Fix JavaScript execution in link's href attribute when using click() method • Ensure compatibility with Firefox 42.0 32bit on 64bit machine • Restore functionality that worked in version 2.47.1 but broke in 2.48.0 and 2.48.2 5678 - Not compliant Non-compliant requirements: • Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4 • Resolve "Connection refused" errors for subsequent ChromeDriver instances • Ensure first ChromeDriver instance works without console errors • Support Chrome 65.0.3325.181 with ChromeDriver version 2.35

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Duplication Multiple event classes have identical from_json implementations that check for the same type and call the same method. This creates maintenance overhead and potential inconsistencies. class ContextCreated: """Event class for browsingContext.contextCreated event.""" event_class = "browsingContext.contextCreated" @classmethod def from_json(cls, json: dict): if isinstance(json, BrowsingContextInfo): return json return BrowsingContextInfo.from_json(json) class ContextDestroyed: """Event class for browsingContext.contextDestroyed event.""" event_class = "browsingContext.contextDestroyed" @classmethod def from_json(cls, json: dict): if isinstance(json, BrowsingContextInfo): return json return BrowsingContextInfo.from_json(json) class NavigationStarted: """Event class for browsingContext.navigationStarted event.""" event_class = "browsingContext.navigationStarted" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class NavigationCommitted: """Event class for browsingContext.navigationCommitted event.""" event_class = "browsingContext.navigationCommitted" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class NavigationFailed: """Event class for browsingContext.navigationFailed event.""" event_class = "browsingContext.navigationFailed" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class NavigationAborted: """Event class for browsingContext.navigationAborted event.""" event_class = "browsingContext.navigationAborted" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class DomContentLoaded: """Event class for browsingContext.domContentLoaded event.""" event_class = "browsingContext.domContentLoaded" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class Load: """Event class for browsingContext.load event.""" event_class = "browsingContext.load" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class FragmentNavigated: """Event class for browsingContext.fragmentNavigated event.""" event_class = "browsingContext.fragmentNavigated" @classmethod def from_json(cls, json: dict): if isinstance(json, NavigationInfo): return json return NavigationInfo.from_json(json) class DownloadWillBegin: """Event class for browsingContext.downloadWillBegin event.""" event_class = "browsingContext.downloadWillBegin" @classmethod def from_json(cls, json: dict): return DownloadWillBeginParams.from_json(json) class UserPromptOpened: """Event class for browsingContext.userPromptOpened event.""" event_class = "browsingContext.userPromptOpened" @classmethod def from_json(cls, json: dict): return UserPromptOpenedParams.from_json(json) class UserPromptClosed: """Event class for browsingContext.userPromptClosed event.""" event_class = "browsingContext.userPromptClosed" @classmethod def from_json(cls, json: dict): return UserPromptClosedParams.from_json(json) class HistoryUpdated: """Event class for browsingContext.historyUpdated event.""" event_class = "browsingContext.historyUpdated" @classmethod def from_json(cls, json: dict): return HistoryUpdatedParams.from_json(json) Missing Validation The _EventManager.__init__ method doesn't validate that event_configs parameter is not None or empty, which could lead to runtime errors when accessing the dictionary. def __init__(self, conn, event_configs: dict[str, EventConfig]): self.conn = conn self.event_configs = event_configs self.subscriptions: dict = {} self._bidi_to_class = {config.bidi_event: config.event_class for config in event_configs.values()} self._available_events = ", ".join(sorted(event_configs.keys())) Test Reliability Several tests use timing-dependent assertions and WebDriverWait without proper error handling, which could lead to flaky test failures in CI environments. driver.find_element(By.ID, "alert").click() WebDriverWait(driver, 5).until(EC.alert_is_present()) assert len(events_received) == 1 assert events_received[0].type == "alert" assert events_received[0].message == "cheese" # Clean up the alert driver.browsing_context.handle_user_prompt(context=driver.current_window_handle) driver.browsing_context.remove_event_handler("user_prompt_opened", callback_id) def test_add_event_handler_user_prompt_closed(driver, pages): """Test adding event handler for user_prompt_closed event.""" events_received = [] def on_user_prompt_closed(info): events_received.append(info) callback_id = driver.browsing_context.add_event_handler("user_prompt_closed", on_user_prompt_closed) assert callback_id is not None create_prompt_page(driver, pages) driver.execute_script("prompt('Enter something')") WebDriverWait(driver, 5).until(EC.alert_is_present()) driver.browsing_context.handle_user_prompt( context=driver.current_window_handle, accept=True, user_text="test input" ) assert len(events_received) == 1 assert events_received[0].accepted is True assert events_received[0].user_text == "test input" driver.browsing_context.remove_event_handler("user_prompt_closed", callback_id) @pytest.mark.xfail_chrome @pytest.mark.xfail_firefox @pytest.mark.xfail_edge def test_add_event_handler_history_updated(driver, pages): """Test adding event handler for history_updated event.""" events_received = [] def on_history_updated(info): events_received.append(info) callback_id = driver.browsing_context.add_event_handler("history_updated", on_history_updated) assert callback_id is not None # Navigate to a page and use history API to trigger the event context_id = driver.current_window_handle url = pages.url("simpleTest.html") driver.browsing_context.navigate(context=context_id, url=url, wait=ReadinessState.COMPLETE) # Use history.pushState to trigger history updated event driver.execute_script("history.pushState({}, '', '/new-path');") assert len(events_received) == 1 assert any("/new-path" in event.url for event in events_received) driver.browsing_context.remove_event_handler("history_updated", callback_id) @pytest.mark.xfail_firefox def test_add_event_handler_download_will_begin(driver, pages): """Test adding event handler for download_will_begin event.""" events_received = [] def on_download_will_begin(info): events_received.append(info) callback_id = driver.browsing_context.add_event_handler("download_will_begin", on_download_will_begin) assert callback_id is not None # click on a download link to trigger the event context_id = driver.current_window_handle url = pages.url("downloads/download.html") driver.browsing_context.navigate(context=context_id, url=url, wait=ReadinessState.COMPLETE) download_xpath_file_1_txt = '//*[@id="file-1"]' driver.find_element(By.XPATH, download_xpath_file_1_txt).click() WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0)

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 086bc51

Category	Suggestion	Impact
Possible issue	Import missing Session class The method creates a new Session instance on every call but doesn't import it. This will cause a NameError at runtime since Session is not defined in the current scope. py/selenium/webdriver/common/bidi/browsing_context.py [548-559] def subscribe_to_event(self, bidi_event: str, contexts: Optional[list[str]] = None) -> None: """Subscribe to a BiDi event if not already subscribed. Parameters: ---------- bidi_event: The BiDi event name. contexts: Optional browsing context IDs to subscribe to. """ if bidi_event not in self.subscriptions: + from selenium.webdriver.common.bidi.session import Session session = Session(self.conn) self.conn.execute(session.subscribe(bidi_event, browsing_contexts=contexts)) self.subscriptions[bidi_event] = [] Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the Session class is used without being imported, which would cause a NameError at runtime, and provides a valid fix.	High
	Import Session and fix instantiation The method creates a Session instance without importing it, causing a NameError. Additionally, the session should be created inside the loop to ensure proper cleanup for each event. py/selenium/webdriver/common/bidi/browsing_context.py [607-623] def clear_event_handlers(self) -> None: """Clear all event handlers from the browsing context.""" if not self.subscriptions: return - session = Session(self.conn) + from selenium.webdriver.common.bidi.session import Session for bidi_event, callback_ids in list(self.subscriptions.items()): event_class = self._bidi_to_class.get(bidi_event) if event_class: # Remove all callbacks for this event for callback_id in callback_ids: self.conn.remove_callback(event_class, callback_id) + session = Session(self.conn) self.conn.execute(session.unsubscribe(bidi_event)) self.subscriptions.clear() Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the Session class is used without being imported, which would cause a NameError, and provides a valid fix to prevent this runtime error.	High
Learned best practice	Add null validation for parameters The from_json methods in event classes don't validate that the json parameter is not None before using it. This could cause AttributeError if None is passed. Add null checks before processing the json parameter. py/selenium/webdriver/common/bidi/browsing_context.py [382-386] @classmethod def from_json(cls, json: dict): + if json is None: + raise ValueError("json parameter cannot be None") if isinstance(json, BrowsingContextInfo): return json return BrowsingContextInfo.from_json(json) Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors. This includes checking dictionary keys exist before accessing them, validating required JSON properties, and ensuring objects are not null before method calls.	Low
Update

---

Previous suggestions

Suggestions

Category	Suggestion	Impact
Possible issue	Wrap callback for proper deserialization The callback registration uses the event class directly, but the callback should be wrapped to handle the JSON deserialization using the event class's from_json method. py/selenium/webdriver/common/bidi/browsing_context.py [582-593] def add_event_handler(self, event: str, callback: Callable, contexts: Optional[list[str]] = None) -> int: event_config = self.validate_event(event) - callback_id = self.conn.add_callback(event_config.event_class, callback) + def wrapped_callback(event_data): + parsed_data = event_config.event_class.from_json(event_data.params) + callback(parsed_data) + + callback_id = self.conn.add_callback(event_config.event_class, wrapped_callback) # Subscribe to the event if needed self.subscribe_to_event(event_config.bidi_event, contexts) # Track the callback self.add_callback_to_tracking(event_config.bidi_event, callback_id) return callback_id Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical bug where the raw event data is passed to the callback instead of the deserialized object, which would cause a runtime error.	High
Learned best practice	Add null parameter validation Add null checks for the json parameter before using it to prevent runtime errors. The method should validate that json is not None and handle cases where BrowsingContextInfo.from_json might fail. py/selenium/webdriver/common/bidi/browsing_context.py [383-386] def from_json(cls, json: dict): + if json is None: + raise ValueError("json parameter cannot be None") if isinstance(json, BrowsingContextInfo): return json return BrowsingContextInfo.from_json(json) Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors. This includes checking dictionary keys exist before accessing them, validating required JSON properties, and ensuring objects are not null before method calls.	Low

[qodo-merge-pro]

Persistent suggestions updated to latest commit 086bc51

[shbenzer]

I have a couple questions regarding test_event_handler_thread_safety(), and I apologize if any of them are dumb:

1. If this test fails, is that an issue we should fix on our end, or something the developers should fix on the driver-side?
2. Isn't append atomic? If so, would there not need to be another line in on_context_created() accessing the events_received list to fully ensure thread safety?
3. Do we import threading anywhere else, or is this a new import? EDIT: It's used in webserver.py

[navin772]

@shbenzer These are some good questions.

I inspected and realized that the test_event_handler_thread_safety() test doesn't really tests thread safety, it just creates 3 contexts sequentially and receives their events in the list.

I wrote a test that tests it in a much better way and realized that we need to modify our _EventManager class to support thread safety (basically add a thread lock - threading.Lock()). So:

1. Yes, thread safety needs to be supported on our end.
2. Yes, currently it is atomic but we can add multiple operations (my latest commit does it)

The question now is do we add support for this? I think yes. I tried adding thread lock support for events and wrote a better thread test.
I will commit it for you to review.

[shbenzer]

The EventManager code looks better. Lets break this big test into a few smaller ones so it's easier to diagnose

[navin772]

I have divided the thread test into 4 tests, I used AI to do it (with some minor changes) since the logic is the same, so do review it :)