[py][bidi] Route navigation commands through BiDi#17715
Open
shossain786 wants to merge 2 commits into
Open
Conversation
Route get/back/forward/refresh through the BiDi browsingContext module when BiDi is enabled (webSocketUrl present in capabilities), falling back to Classic HTTP commands otherwise. The pageLoadStrategy capability is mapped to a BiDi readiness state (normal=COMPLETE, eager=INTERACTIVE, none=NONE), and the current window handle is used as the context id. Mirrors the merged Ruby implementation (SeleniumHQ#14094). Part of SeleniumHQ#13995. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
Contributor
PR Summary by QodoRoute Python navigation commands through WebDriver BiDi when enabled Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
17 rules 1. webSocketUrl type unchecked
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+29
to
+68
| def test_bidi_is_enabled(driver): | ||
| """The driver should report BiDi as enabled when web_socket_url is set.""" | ||
| assert driver._is_bidi_enabled() is True | ||
|
|
||
|
|
||
| def test_get_navigates_via_bidi(driver, pages): | ||
| """driver.get() should load the page through BiDi and block until complete.""" | ||
| url = pages.url("simpleTest.html") | ||
| driver.get(url) | ||
| assert driver.current_url == url | ||
| assert "Hello WebDriver" in driver.title | ||
|
|
||
|
|
||
| def test_back_and_forward_via_bidi(driver, pages): | ||
| """back()/forward() should traverse history through BiDi.""" | ||
| first = pages.url("simpleTest.html") | ||
| second = pages.url("formPage.html") | ||
|
|
||
| driver.get(first) | ||
| driver.get(second) | ||
| assert driver.current_url == second | ||
|
|
||
| driver.back() | ||
| assert driver.current_url == first | ||
|
|
||
| driver.forward() | ||
| assert driver.current_url == second | ||
|
|
||
|
|
||
| def test_refresh_via_bidi(driver, pages): | ||
| """refresh() should reload the current context through BiDi.""" | ||
| url = pages.url("formPage.html") | ||
| driver.get(url) | ||
| driver.refresh() | ||
| assert driver.current_url == url | ||
|
|
||
|
|
||
| def test_page_load_readiness_default_is_complete(driver): | ||
| """With no explicit pageLoadStrategy, readiness should default to COMPLETE.""" | ||
| assert driver._page_load_readiness() == ReadinessState.COMPLETE |
Contributor
There was a problem hiding this comment.
1. bidi_navigation_tests missing type hints 📘 Rule violation ✧ Quality
New pytest test functions were added without parameter type annotations and without explicit `-> None` return annotations. This violates the requirement that new function signatures include full type annotations.
Agent Prompt
## Issue description
New test functions in `bidi_navigation_tests.py` are missing parameter and return type annotations (e.g., `driver`, `pages`, and `-> None`).
## Issue Context
Compliance requires explicit type annotations on newly added function/method signatures.
## Fix Focus Areas
- py/test/selenium/webdriver/common/bidi_navigation_tests.py[29-68]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+736
to
+742
| def _is_bidi_enabled(self) -> bool: | ||
| """Returns True if WebDriver BiDi is enabled for this session. | ||
| BiDi is enabled when the remote end advertised a ``webSocketUrl`` in | ||
| the session capabilities (i.e. the user requested it via options). | ||
| """ | ||
| return bool(self.caps.get("webSocketUrl")) |
Contributor
There was a problem hiding this comment.
4. Websocketurl type unchecked 🐞 Bug ≡ Correctness
_is_bidi_enabled() treats any truthy caps['webSocketUrl'] as BiDi-enabled, so a boolean value can incorrectly route navigation through BiDi and then fail when BiDi startup uses that value as a websocket URL.
Agent Prompt
### Issue description
`WebDriver._is_bidi_enabled()` currently returns `True` for any truthy `webSocketUrl` capability value. Some remote ends can return `webSocketUrl` as a boolean (e.g., echoing the requested capability) rather than an actual websocket URL string. When this happens, navigation methods (`get/back/forward/refresh`) will take the BiDi path and eventually try to create a websocket connection using a non-string value.
### Issue Context
This PR newly gates core navigation behavior on `_is_bidi_enabled()`, so mis-detection now impacts common flows even if users don’t explicitly call BiDi APIs.
### Fix Focus Areas
- py/selenium/webdriver/remote/webdriver.py[736-754]
- py/selenium/webdriver/remote/webdriver.py[1243-1256]
### What to change
- Make `_is_bidi_enabled()` return `True` only when `caps['webSocketUrl']` is a valid websocket URL **string** (and ideally validate it parses as `ws://` or `wss://`).
- Harden `_start_bidi()` to validate the type/value before constructing `WebSocketConnection`, and raise a clear `WebDriverException` if invalid.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+717
to
+726
| if self._is_bidi_enabled(): | ||
| self.browsing_context.traverse_history(context=self.current_window_handle, delta=-1) | ||
| else: | ||
| self.execute(Command.GO_BACK) | ||
|
|
||
| def forward(self) -> None: | ||
| """Goes one step forward in the browser history.""" | ||
| self.execute(Command.GO_FORWARD) | ||
| if self._is_bidi_enabled(): | ||
| self.browsing_context.traverse_history(context=self.current_window_handle, delta=1) | ||
| else: |
Contributor
There was a problem hiding this comment.
5. Back/forward blocking changed 🐞 Bug ☼ Reliability
back()/forward() now call BiDi browsing_context.traverse_history() without any readiness/wait handling, which can change Classic blocking semantics and introduce races where these methods return before navigation completes and tests (or callers) observe stale state. This is visible in the new BiDi navigation test that asserts current_url immediately after back()/forward(), making outcomes timing-dependent if traversal is asynchronous.
Agent Prompt
## Issue description
When BiDi is enabled, `WebDriver.back()` and `WebDriver.forward()` call `browsing_context.traverse_history(...)`, but unlike Classic semantics (and unlike other navigation APIs that honor `pageLoadStrategy` readiness), this call path does not include a readiness/wait step; as a result, `back()/forward()` may return before the navigation reaches the expected ready state and tests/callers may observe the old URL/title/document state.
## Issue Context
Existing BiDi tests in this repo treat `traverse_history` as requiring an explicit wait for the page state to change (e.g., waiting for a title update), suggesting the operation can complete asynchronously relative to observable page readiness. The newly added BiDi navigation test asserts `driver.current_url` immediately after `back()`/`forward()`, which becomes timing-dependent if `traverse_history` does not block until the target readiness is reached.
## Fix Focus Areas
- py/selenium/webdriver/remote/webdriver.py[714-755]
- py/test/selenium/webdriver/common/bidi_navigation_tests.py[42-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 98827cc |
cgoldberg
reviewed
Jun 24, 2026
| else: | ||
| self.execute(Command.REFRESH) | ||
|
|
||
| def _is_bidi_enabled(self) -> bool: |
Member
There was a problem hiding this comment.
I'm not sure this method is needed. Elsewhere in this class, we already use self._websocket_connection to check for BiDi (it is None when BiDi is not enabled).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Part of #13995
💥 What does this PR do?
Implements the Python binding for routing the Classic navigation commands through WebDriver BiDi when BiDi is enabled, following the pattern of the merged Ruby PR (#14094) and the in-review Java PR (#17232).
When the session has BiDi enabled (the remote end advertised a
webSocketUrl),WebDrivernow routes:get(url)browsing_context.navigate(context, url, wait=<readiness>)back()browsing_context.traverse_history(context, delta=-1)forward()browsing_context.traverse_history(context, delta=1)refresh()browsing_context.reload(context, wait=<readiness>)When BiDi is not enabled, the existing Classic HTTP commands are used unchanged.
The
pageLoadStrategycapability is mapped to a BiDi readiness state:normal → COMPLETE,eager → INTERACTIVE,none → NONE(defaulting toCOMPLETEto preserve Classicget()blocking semantics). The current window handle is used as the browsing-context id.🔧 Implementation Notes
The generated
BrowsingContextmodule already exposesnavigate/traverse_history/reload, so no generator changes were needed — the change is contained inwebdriver.pyplus a new test module.Unlike Ruby (which introduced a dedicated
BiDiBridge), the Python binding has a singleWebDriverclass, so this uses an inlineif self._is_bidi_enabled()branch in the four navigation methods plus two small helpers (_is_bidi_enabled,_page_load_readiness). If maintainers prefer a separate bridge/mixin to keep the Classic and BiDi paths cleanly split, I'm happy to refactor — see Additional Considerations.🤖 AI assistance
webdriver.py, the two helper methods, and the newbidi_navigation_tests.py💡 Additional Considerations
WebDrivervs. a dedicated BiDi bridge/mixin. Open to either — guidance welcome.--bidiflag (auto-globbed intoBIDI_TESTSvia the*bidi*_tests.pypattern).🔄 Types of changes