Skip to content

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Oct 28, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the set_screen_orientation_override command to the Emulation module - https://w3c.github.io/webdriver-bidi/#command-emulation-setScreenOrientationOverride.

🔧 Implementation Notes

Usage:

  1. Using ENUM:

    orientation = ScreenOrientation(
            natural=ScreenOrientationNatural.LANDSCAPE,
            type=ScreenOrientationType.LANDSCAPE_PRIMARY,
        )
    driver.emulation.set_screen_orientation_override(screen_orientation=orientation, contexts=[context_id])
  2. Using strings:

    orientation = ScreenOrientation(natural="landscape", type="landscape-secondary")
    driver.emulation.set_screen_orientation_override(screen_orientation=orientation, user_contexts=[user_context])

💡 Additional Considerations

Supported by both chromium and Firefox.

We can set the natural orientation and the type via ENUMs or through strings directly. _convert_to_enum internally converts str to enum and raises value error in wrong string is passed.

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Adds set_screen_orientation_override command to Emulation module

  • Implements ScreenOrientation class with enum support

  • Supports both enum and string-based orientation configuration

  • Includes comprehensive tests for contexts and user contexts


Diagram Walkthrough

flowchart LR
  A["ScreenOrientationNatural<br/>ScreenOrientationType<br/>Enums"] -->|"_convert_to_enum"| B["ScreenOrientation<br/>Class"]
  B -->|"to_dict()"| C["set_screen_orientation_override<br/>Command"]
  C -->|"execute"| D["Emulation Module"]
  E["Test Cases<br/>Contexts & UserContexts"] -->|"verify"| D
Loading

File Walkthrough

Relevant files
Enhancement
emulation.py
Implement screen orientation override functionality           

py/selenium/webdriver/common/bidi/emulation.py

  • Added ScreenOrientationNatural and ScreenOrientationType enums for
    orientation values
  • Implemented _convert_to_enum() helper function to convert strings to
    enums with validation
  • Created ScreenOrientation class to encapsulate natural and type
    orientation settings
  • Added set_screen_orientation_override() method to apply orientation
    overrides to contexts or user contexts
  • Includes validation to ensure either contexts or user_contexts is
    provided, but not both
+89/-0   
Tests
bidi_emulation_tests.py
Add comprehensive screen orientation override tests           

py/test/selenium/webdriver/common/bidi_emulation_tests.py

  • Added imports for ScreenOrientation, ScreenOrientationNatural, and
    ScreenOrientationType
  • Implemented get_screen_orientation() helper function to retrieve
    current screen orientation and angle
  • Added test_set_screen_orientation_override_with_contexts() to test
    orientation changes with browsing contexts
  • Added parametrized
    test_set_screen_orientation_override_with_user_contexts() covering all
    orientation combinations
  • Tests verify orientation type, angle values, and clearing overrides
+109/-1 

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Oct 28, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 28, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Information disclosure

Description: The generic ValueError message "Invalid orientation: {value}" in _convert_to_enum leaks
raw input back to callers, which could expose unexpected or sensitive input values in
logs; consider using a safer, standardized error message or sanitizing the value.
emulation.py [40-47]

Referred Code
def _convert_to_enum(value, enum_class):
    if isinstance(value, enum_class):
        return value
    try:
        return enum_class(value)
    except ValueError:
        raise ValueError(f"Invalid orientation: {value}")
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve "Error: ConnectFailure (Connection refused)" occurring on
subsequent ChromeDriver instantiations on Ubuntu 16.04.4 with Chrome 65 / ChromeDriver
2.35 using Selenium 3.9.0.
Provide guidance or fixes ensuring multiple ChromeDriver instances do not produce
connection refused errors after the first instance.
🟡
🎫 #1234
🔴 Ensure WebDriver click() triggers JavaScript in href for Firefox (noted regression from
2.47.1 to 2.48.x with Firefox 42).
Provide fix or tests verifying alert fires when clicking links with JavaScript in href.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

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

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

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new command execution for setting screen orientation override performs a critical
emulation change without any auditing or logging of who initiated it, when, or the
outcome.

Referred Code
def set_screen_orientation_override(
    self,
    screen_orientation: Optional[ScreenOrientation] = None,
    contexts: Optional[list[str]] = None,
    user_contexts: Optional[list[str]] = None,
) -> None:
    """Set screen orientation override for the given contexts or user contexts.

    Args:
        screen_orientation: ScreenOrientation object to emulate, or None to clear the override.
        contexts: List of browsing context IDs to apply the override to.
        user_contexts: List of user context IDs to apply the override to.

    Raises:
        ValueError: If both contexts and user_contexts are provided, or if neither
            contexts nor user_contexts are provided.
    """
    if contexts is not None and user_contexts is not None:
        raise ValueError("Cannot specify both contexts and userContexts")

    if contexts is None and user_contexts is None:


 ... (clipped 12 lines)
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Input validation: The method validates mutual exclusivity of contexts but does not validate list contents
(e.g., empty lists or invalid IDs) and may pass None for screenOrientation without
explicit handling beyond server-side errors.

Referred Code
if contexts is not None and user_contexts is not None:
    raise ValueError("Cannot specify both contexts and userContexts")

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

params: dict[str, Any] = {
    "screenOrientation": screen_orientation.to_dict() if screen_orientation is not None else None
}

if contexts is not None:
    params["contexts"] = contexts
elif user_contexts is not None:
    params["userContexts"] = user_contexts

self.conn.execute(command_builder("emulation.setScreenOrientationOverride", params))
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Enum conversion: The _convert_to_enum helper raises a generic ValueError with the raw value, and the method
does not sanitize or validate elements of contexts/user_contexts lists beyond presence,
which may allow invalid external inputs to propagate.

Referred Code
def _convert_to_enum(value, enum_class):
    if isinstance(value, enum_class):
        return value
    try:
        return enum_class(value)
    except ValueError:
        raise ValueError(f"Invalid orientation: {value}")


class ScreenOrientation:
    """Represents screen orientation configuration."""

    def __init__(
        self,
        natural: Union[ScreenOrientationNatural, str],
        type: Union[ScreenOrientationType, str],
    ):
        """Initialize ScreenOrientation.

        Args:
            natural: Natural screen orientation ("portrait" or "landscape").


 ... (clipped 9 lines)
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 28, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Refactor context handling into a helper

Extract the duplicated validation and parameter-building logic for contexts and
user_contexts from methods in the Emulation class into a shared private helper
method.

Examples:

py/selenium/webdriver/common/bidi/emulation.py [388-401]
        if contexts is not None and user_contexts is not None:
            raise ValueError("Cannot specify both contexts and userContexts")

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

        params: dict[str, Any] = {
            "screenOrientation": screen_orientation.to_dict() if screen_orientation is not None else None
        }


 ... (clipped 4 lines)

Solution Walkthrough:

Before:

class Emulation:
    def set_geolocation_override(self, ..., contexts=None, user_contexts=None):
        # Duplicated validation and parameter handling for contexts
        if contexts is not None and user_contexts is not None:
            raise ValueError(...)
        if contexts is None and user_contexts is None:
            raise ValueError(...)
        params = {}
        if contexts is not None:
            params["contexts"] = contexts
        elif user_contexts is not None:
            params["userContexts"] = user_contexts
        # ... method-specific logic

    def set_screen_orientation_override(self, ..., contexts=None, user_contexts=None):
        # Duplicated validation and parameter handling for contexts
        if contexts is not None and user_contexts is not None:
            raise ValueError(...)
        if contexts is None and user_contexts is None:
            raise ValueError(...)
        params = {}
        if contexts is not None:
            params["contexts"] = contexts
        elif user_contexts is not None:
            params["userContexts"] = user_contexts
        # ... method-specific logic

After:

class Emulation:
    def _get_context_params(self, params, contexts, user_contexts):
        if contexts is not None and user_contexts is not None:
            raise ValueError("Cannot specify both contexts and userContexts")
        if contexts is None and user_contexts is None:
            raise ValueError("Must specify either contexts or userContexts")
        
        if contexts is not None:
            params["contexts"] = contexts
        elif user_contexts is not None:
            params["userContexts"] = user_contexts

    def set_geolocation_override(self, ..., contexts=None, user_contexts=None):
        params = { ... }
        self._get_context_params(params, contexts, user_contexts)
        # ... method-specific logic

    def set_screen_orientation_override(self, ..., contexts=None, user_contexts=None):
        params = { ... }
        self._get_context_params(params, contexts, user_contexts)
        # ... method-specific logic
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies duplicated logic for handling contexts and user_contexts across multiple methods and proposes a valid refactoring that would improve code maintainability and consistency.

Low
General
Add assertion for clearing override

In the test test_set_screen_orientation_override_with_user_contexts, add an
assertion to verify that the screen orientation is reset to its initial state
after the override is cleared.

py/test/selenium/webdriver/common/bidi_emulation_tests.py [503-529]

 def test_set_screen_orientation_override_with_user_contexts(driver, pages, natural, orientation_type, expected_angle):
     user_context = driver.browser.create_user_context()
     try:
         context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
         try:
             driver.switch_to.window(context_id)
+            initial_orientation = get_screen_orientation(driver, context_id)
 
             # Set the specified orientation
             orientation = ScreenOrientation(natural=natural, type=orientation_type)
             driver.emulation.set_screen_orientation_override(
                 screen_orientation=orientation, user_contexts=[user_context]
             )
 
             url = pages.url("formPage.html")
             driver.browsing_context.navigate(context_id, url, wait="complete")
 
             # Verify the orientation was set
             current_orientation = get_screen_orientation(driver, context_id)
 
             assert current_orientation["type"] == orientation_type
             assert current_orientation["angle"] == expected_angle
 
             driver.emulation.set_screen_orientation_override(screen_orientation=None, user_contexts=[user_context])
+            assert get_screen_orientation(driver, context_id) == initial_orientation
         finally:
             driver.browsing_context.close(context_id)
     finally:
         driver.browser.remove_user_context(user_context)
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a missing assertion in the test case to verify that clearing the screen orientation override works as expected, which improves the test's thoroughness.

Low
Learned
best practice
Ensure test cleanup in finally

Ensure the override is always cleared even if assertions fail by moving the
cleanup into a finally block.

py/test/selenium/webdriver/common/bidi_emulation_tests.py [449-485]

 def test_set_screen_orientation_override_with_contexts(driver, pages):
     context_id = driver.current_window_handle
     initial_orientation = get_screen_orientation(driver, context_id)
-    ...
-    driver.emulation.set_screen_orientation_override(screen_orientation=None, contexts=[context_id])
-    # Verify orientation was cleared
-    assert get_screen_orientation(driver, context_id) == initial_orientation
+    try:
+        # Set landscape-primary orientation
+        orientation = ScreenOrientation(
+            natural=ScreenOrientationNatural.LANDSCAPE,
+            type=ScreenOrientationType.LANDSCAPE_PRIMARY,
+        )
+        driver.emulation.set_screen_orientation_override(screen_orientation=orientation, contexts=[context_id])
+        url = pages.url("formPage.html")
+        driver.browsing_context.navigate(context_id, url, wait="complete")
+        current_orientation = get_screen_orientation(driver, context_id)
+        assert current_orientation["type"] == "landscape-primary", f"Expected landscape-primary, got {current_orientation}"
+        assert current_orientation["angle"] == 0, f"Expected angle 0, got {current_orientation['angle']}"
+        # Set portrait-secondary orientation
+        orientation = ScreenOrientation(
+            natural=ScreenOrientationNatural.PORTRAIT,
+            type=ScreenOrientationType.PORTRAIT_SECONDARY,
+        )
+        driver.emulation.set_screen_orientation_override(screen_orientation=orientation, contexts=[context_id])
+        current_orientation = get_screen_orientation(driver, context_id)
+        assert current_orientation["type"] == "portrait-secondary", (f"Expected portrait-secondary, got {current_orientation}")
+        assert current_orientation["angle"] == 180, f"Expected angle 180, got {current_orientation['angle']}"
+    finally:
+        driver.emulation.set_screen_orientation_override(screen_orientation=None, contexts=[context_id])
+        assert get_screen_orientation(driver, context_id) == initial_orientation

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Enforce deterministic and safe resource handling in tests by ensuring cleanup on all code paths.

Low
Strengthen enum input validation

Validate the incoming type and include the expected enum name in the error to
aid debugging. Add type hints and a docstring for clarity.

py/selenium/webdriver/common/bidi/emulation.py [40-46]

-def _convert_to_enum(value, enum_class):
+def _convert_to_enum(value: Union[str, Enum], enum_class: type[Enum]) -> Enum:
+    """Convert a value to the given Enum, raising a clear error on invalid input."""
     if isinstance(value, enum_class):
         return value
     try:
         return enum_class(value)
-    except ValueError:
-        raise ValueError(f"Invalid orientation: {value}")
+    except ValueError as e:
+        raise ValueError(f"Invalid {enum_class.__name__} value: {value!r}") from e
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Validate inputs and states early with precise checks and clear, consistent error messages.

Low
  • Update

@navin772 navin772 requested a review from cgoldberg October 28, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants