[py][bidi] Allow resetting viewport

[cgoldberg]

User description

🔗 Related Issues

Fixes: #16622
Also see: ##16601 and w3c/webdriver-bidi#1039

💥 What does this PR do?

The BiDi spec allows you to reset the viewport size and device pixel ratio by sending "viewport": null or "devicePixelRatio": null:

https://w3c.github.io/webdriver-bidi/#command-browsingContext-setViewport

This PR accomplishes that by sending "viewport": null or "devicePixelRatio": null, when the arguments to set_viewport() are explicitly set to None.

For example:

set_viewport(viewport=None)
set_viewport(device_pixel_ratio=None)
set_viewport(viewport=None, device_pixel_ratio=None)

If either argument is not passed, the behavior remains the same as before (viewport and/or devicePixelRatio is not included in the JSON command payload).

This PR also includes a test for this and updates the other viewport tests to reset the viewport and device pixel ratio to default values in their teardown so they don't affect other tests.

🔧 Implementation Notes

To differentiate between being invoked with the arguments set to None and not being provided, I used a sentinel value as the default for the keyword args. Python doesn't have a built-in utility for defining sentinel values (it may someday, see PEP661: https://peps.python.org/pep-0661/), so rather than writing my own, I used
the Sentinel class from typing_extensions (we already include this as a dependency): https://typing-extensions.readthedocs.io/en/latest/#sentinel-objects

🔄 Types of changes

• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement, Bug fix

---

Description

• Allow resetting viewport and device pixel ratio to defaults via None

• Use sentinel value to differentiate explicit None from unspecified arguments

• Add comprehensive tests for viewport reset functionality

• Update existing tests with teardown to reset viewport state

---

Diagram Walkthrough

flowchart LR
  A["set_viewport arguments"] -->|UNDEFINED sentinel| B["Skip parameter"]
  A -->|None value| C["Send null to BiDi"]
  A -->|dict/float value| D["Send value to BiDi"]
  C --> E["Reset to defaults"]
  D --> F["Apply custom settings"]

Loading

File Walkthrough

	Relevant files
Enhancement	browsing_context.py Implement viewport reset with sentinel value pattern          py/selenium/webdriver/common/bidi/browsing_context.py Import Sentinel from typing_extensions and define UNDEFINED sentinel value Update set_viewport() method signature to use sentinel defaults for viewport and device_pixel_ratio parameters Implement conditional logic to differentiate between unspecified arguments (UNDEFINED), explicit None (reset), and actual values Update docstrings to clarify that None resets parameters to defaults +19/-7
Tests	bidi_browsing_context_tests.py Add viewport reset tests and cleanup test teardowns            py/test/selenium/webdriver/common/bidi_browsing_context_tests.py Add try-finally blocks to existing viewport tests to reset viewport state in teardown Update test values to be unique (251x301, 252x302) to distinguish test cases Add new test_set_viewport_back_to_default() test to verify reset functionality Verify that passing None restores default viewport and device pixel ratio values +43/-12

---

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misassigned value: In setting parameters, 'devicePixelRatio' is assigned the 'viewport' variable instead of 'device_pixel_ratio', which is misleading and incorrect naming/usage. Referred Code if device_pixel_ratio is UNDEFINED: pass elif device_pixel_ratio is None: params["devicePixelRatio"] = None else: params["devicePixelRatio"] = viewport Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new viewport-setting pathway does not add or reference any audit logging for critical actions, and it is unclear if such logging is handled elsewhere. Referred Code self, context: str | None = None, viewport: dict | None | UNDEFINED = UNDEFINED, device_pixel_ratio: float | UNDEFINED = UNDEFINED, user_contexts: list[str] | None = None, ) -> None: """Modifies specific viewport characteristics on the given top-level traversable. Args: context: The browsing context ID. viewport: The viewport parameters (`None` resets to default). device_pixel_ratio: The device pixel ratio (`None` resets default). user_contexts: The user context IDs. Raises: Exception: If the browsing context is not a top-level traversable. """ params: dict[str, Any] = {} if context is not None: params["context"] = context if viewport is UNDEFINED: ... (clipped 15 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing validation: New logic does not validate 'viewport' dict structure or 'device_pixel_ratio' bounds, potentially allowing invalid inputs without explicit error handling. Referred Code params: dict[str, Any] = {} if context is not None: params["context"] = context if viewport is UNDEFINED: pass elif viewport is None: params["viewport"] = None else: params["viewport"] = viewport if device_pixel_ratio is UNDEFINED: pass elif device_pixel_ratio is None: params["devicePixelRatio"] = None else: params["devicePixelRatio"] = viewport Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Input validation: The method forwards 'viewport' and 'device_pixel_ratio' without explicit validation or sanitization, which may be acceptable if validated upstream but is not evident here. Referred Code params: dict[str, Any] = {} if context is not None: params["context"] = context if viewport is UNDEFINED: pass elif viewport is None: params["viewport"] = None else: params["viewport"] = viewport if device_pixel_ratio is UNDEFINED: pass elif device_pixel_ratio is None: params["devicePixelRatio"] = None else: params["devicePixelRatio"] = viewport Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to ce185c6

Category	Suggestion	Impact
General	Make viewport size asserts tolerant In test_set_viewport, modify the viewport dimension assertions to allow a small tolerance (e.g., 2 pixels) to prevent flaky tests caused by platform-specific rounding or UI rendering differences. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [346-359] def test_set_viewport(driver, pages): """Test setting the viewport.""" context_id = driver.current_window_handle driver.get(pages.url("formPage.html")) try: driver.browsing_context.set_viewport(context=context_id, viewport={"width": 251, "height": 301}) viewport_size = driver.execute_script("return [window.innerWidth, window.innerHeight];") - assert viewport_size[0] == 251 - assert viewport_size[1] == 301 + assert abs(viewport_size[0] - 251) <= 2 + assert abs(viewport_size[1] - 301) <= 2 finally: driver.browsing_context.set_viewport(context=context_id, viewport=None, device_pixel_ratio=None) [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential source of test flakiness due to platform-specific rendering differences and proposes a reasonable solution to improve test robustness by using a tolerance for assertions.	Medium
	✅ Align type hints with behavior Suggestion Impact: The commit changed the type hint to include None as suggested and also fixed assigning the correct value to params["devicePixelRatio"]. code diff: - device_pixel_ratio: float | UNDEFINED = UNDEFINED, + device_pixel_ratio: float | None | UNDEFINED = UNDEFINED, user_contexts: list[str] | None = None, ) -> None: """Modifies specific viewport characteristics on the given top-level traversable. @@ -1013,7 +1013,7 @@ elif device_pixel_ratio is None: params["devicePixelRatio"] = None else: - params["devicePixelRatio"] = viewport + params["devicePixelRatio"] = device_pixel_ratio Update the type hint for the device_pixel_ratio parameter in set_viewport to float | None | UNDEFINED to match the implementation that accepts None for resetting the value. py/selenium/webdriver/common/bidi/browsing_context.py [984-990] def set_viewport( self, context: str | None = None, viewport: dict | None | UNDEFINED = UNDEFINED, - device_pixel_ratio: float | UNDEFINED = UNDEFINED, + device_pixel_ratio: float | None | UNDEFINED = UNDEFINED, user_contexts: list[str] | None = None, ) -> None: [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the type hint for device_pixel_ratio is inconsistent with the implementation, which explicitly handles None as a valid input to reset the value.	Low
Learned best practice	Clarify tri-state parameter semantics Update the docstring to clearly document the tri-state behavior using the UNDEFINED sentinel versus None, so callers understand omission vs reset semantics. py/selenium/webdriver/common/bidi/browsing_context.py [984-1001] def set_viewport( self, context: str | None = None, viewport: dict | None | UNDEFINED = UNDEFINED, device_pixel_ratio: float | UNDEFINED = UNDEFINED, user_contexts: list[str] | None = None, ) -> None: """Modifies specific viewport characteristics on the given top-level traversable. Args: context: The browsing context ID. - viewport: The viewport parameters (`None` resets to default). - device_pixel_ratio: The device pixel ratio (`None` resets default). + viewport: Viewport parameters. Use UNDEFINED to leave unchanged, None to reset to default, or a dict with dimensions to set. + device_pixel_ratio: Device pixel ratio. Use UNDEFINED to leave unchanged, None to reset to default, or a float to set. user_contexts: The user context IDs. Raises: Exception: If the browsing context is not a top-level traversable. """ Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Enforce accurate and consistent documentation and naming to match actual behavior and APIs.	Low
Update

---

Previous suggestions

✅ Suggestions up to commit b91113a

Category	Suggestion	Impact
Possible issue	✅ Fix incorrect variable assignment bug Suggestion Impact: The commit changes params["devicePixelRatio"] from viewport to device_pixel_ratio, exactly addressing the bug. code diff: - params["devicePixelRatio"] = viewport + params["devicePixelRatio"] = device_pixel_ratio In set_viewport, fix the incorrect assignment to params["devicePixelRatio"] by using the device_pixel_ratio variable instead of viewport. py/selenium/webdriver/common/bidi/browsing_context.py [1011-1016] if device_pixel_ratio is UNDEFINED: pass elif device_pixel_ratio is None: params["devicePixelRatio"] = None else: - params["devicePixelRatio"] = viewport + params["devicePixelRatio"] = device_pixel_ratio [Suggestion processed] Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical copy-paste bug that assigns the wrong variable, breaking the set_viewport functionality for the device_pixel_ratio parameter.	High
	✅ Correct a test assertion typo Suggestion Impact: The commit updated the assertion to use default_viewport_size[1] for the height comparison, correcting the typo as suggested. code diff: assert viewport_size[0] == default_viewport_size[0] - assert viewport_size[1] == default_viewport_size[0] + assert viewport_size[1] == default_viewport_size[1] assert device_pixel_ratio == default_device_pixel_ratio In test_set_viewport_back_to_default, correct the assertion for viewport height to compare against the default height (default_viewport_size[1]) instead of the default width. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [402-404] assert viewport_size[0] == default_viewport_size[0] -assert viewport_size[1] == default_viewport_size[0] +assert viewport_size[1] == default_viewport_size[1] assert device_pixel_ratio == default_device_pixel_ratio [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a copy-paste error in a test assertion, which would cause the test to validate the wrong behavior and potentially pass incorrectly.	High

[cgoldberg]

I haven't tested this yet, so we'll see if browsers actually support it in CI.

This issue was discussed at length in Slack today, and there might be other BiDi functionality that needs to follow this same pattern when null is an accepted value in the BiDi spec.

This is kind of awkward in Python, but defaulting to a sentinel is the best approach I could think of.

[navin772]

Should we also test a flow where we don't pass the viewport param (the undefined case)?

It could be:
set_viewport with {width, height} -> assert viewport -> set_viewport with no viewport param(undefined) -> assert still same height, width -> set_viewport as None -> assert resets to default

We could add it to a previous test.

[navin772]

@cgoldberg I think using Ellipsis here would be elegant than Sentinel and would not require the if-else checks and the new import, here is how we can use it:

    def set_viewport(
      self,
      context: str | None = None,
      viewport: dict | None | type(Ellipsis) = ...,
      device_pixel_ratio: float | None | type(Ellipsis) = ...,
      user_contexts: list[str] | None = None,
    ) -> None:

        if context is not None and user_contexts is not None:
            raise ValueError("Cannot specify both context and user_contexts")

        if context is None and user_contexts is None:
            raise ValueError("Must specify either context or user_contexts")

        params: dict[str, Any] = {}

        if context is not None:
            params["context"] = context
        else:
            params["userContexts"] = user_contexts

        if viewport is not ...:
            params["viewport"] = viewport  # handles all 3 cases

        if device_pixel_ratio is not ...:
            params["devicePixelRatio"] = device_pixel_ratio

        self.conn.execute(command_builder("browsingContext.setViewport", params))

WDYT about this approach?

[cgoldberg]

I think using Ellipsis here would be elegant than Sentinel and would not require the if-else checks

That's interesting. Your code made me realize I can remove the if-else checks to simplify it even if I use the sentinel. I just updated the code.

I guess it just comes down to whether it's less confusing to use the Ellipses or the Sentinel.

They discuss it here:

https://peps.python.org/pep-0661/#use-the-existing-ellipsis-sentinel-value

I don't know what they mean by Ellipses "can’t be as confidently used in all cases".

[shbenzer]

LGTM. I personally prefer Sentinel as “…” can represent a few things depending on the use case (e.g. in arrays) @navin772 @cgoldberg

[navin772]

LGTM! Either of them looks good now.