[java][BiDi] implement emulation.setScreenOrientationOverride

[Delta456]

User description

🔗 Related Issues

💥 What does this PR do?

Implements emulation.setScreenOrientationOverride from W3C spec - https://w3c.github.io/webdriver-bidi/#command-emulation-setScreenOrientationOverride

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

---

PR Type

Enhancement

---

Description

• Implements emulation.setScreenOrientationOverride BiDi command

• Adds screen orientation enums and parameter classes

• Includes comprehensive tests for context and user context

• Supports clearing orientation overrides with null parameter

---

Diagram Walkthrough

flowchart LR
  A["Emulation.java"] -->|calls| B["setScreenOrientationOverride"]
  C["SetScreenOrientationOverrideParameters"] -->|extends| D["AbstractOverrideParameters"]
  E["ScreenOrientation"] -->|uses| F["ScreenOrientationNatural"]
  E -->|uses| G["ScreenOrientationType"]
  B -->|sends| H["BiDi Command"]
  I["SetScreenOrientationOverrideTest"] -->|tests| B

Loading

File Walkthrough

	Relevant files
Enhancement	Emulation.java Add setScreenOrientationOverride method                                    java/src/org/openqa/selenium/bidi/emulation/Emulation.java Added setScreenOrientationOverride method to Emulation class Method accepts SetScreenOrientationOverrideParameters and sends BiDi command Follows same pattern as existing setUserAgentOverride method +7/-0      ScreenOrientation.java Create ScreenOrientation model class                                          java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java New class representing screen orientation configuration Contains natural orientation and type properties Validates non-null parameters in constructor Provides toMap() method for BiDi serialization +52/-0    ScreenOrientationNatural.java Create ScreenOrientationNatural enum                                          java/src/org/openqa/selenium/bidi/emulation/ScreenOrientationNatural.java New enum defining natural screen orientations Supports PORTRAIT and LANDSCAPE values Implements custom toString() for BiDi protocol serialization +34/-0    ScreenOrientationType.java Create ScreenOrientationType enum                                                java/src/org/openqa/selenium/bidi/emulation/ScreenOrientationType.java New enum defining screen orientation types Supports PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCAPE_PRIMARY, LANDSCAPE_SECONDARY Implements custom toString() for BiDi protocol serialization +36/-0    SetScreenOrientationOverrideParameters.java Create SetScreenOrientationOverrideParameters class            java/src/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideParameters.java New parameter class extending AbstractOverrideParameters Accepts optional ScreenOrientation object in constructor Supports contexts() and userContexts() for targeting specific contexts Handles null orientation for clearing overrides +41/-0
Tests	SetScreenOrientationOverrideTest.java Add comprehensive screen orientation override tests            java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java Test for setting orientation override in specific browsing context Test for setting orientation override in user context Validates orientation changes via JavaScript screen.orientation API Tests clearing overrides by passing null parameter Verifies angle values match expected orientation types +161/-0

---

[qodo-code-review]

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
🟡	🎫 #5678 🔴 Diagnose and resolve repeated "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04.4 with Chrome 65 / ChromeDriver 2.35 using Selenium 3.9.0. Provide a fix or guidance so that subsequent ChromeDriver instantiations do not log the ConnectFailure errors. ⚪ Verify behavior through reproduction steps similar to the reporter’s environment.
🟡	🎫 #1234 🔴 Ensure Selenium 2.48 triggers JavaScript in a link's href on click() in Firefox (e.g., restoring behavior seen in 2.47.1). Provide tests confirming alerts fire when clicking links whose href contains JavaScript in Firefox 42.
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 Learn more about managing compliance generic rules or creating your own custom rules
	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: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed 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 BiDi command invocation for setting screen orientation overrides adds a critical state change without any corresponding audit logging of the action, user, or outcome. Referred Code public void setScreenOrientationOverride(SetScreenOrientationOverrideParameters parameters) { Require.nonNull("SetScreenOrientationOverride parameters", parameters); bidi.send( new Command<>("emulation.setScreenOrientationOverride", parameters.toMap(), Map.class)); } 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 error handling: The method sends the BiDi command without visible handling or reporting of command failures or invalid parameters, relying entirely on underlying layers with no contextual error messages here. Referred Code public void setScreenOrientationOverride(SetScreenOrientationOverrideParameters parameters) { Require.nonNull("SetScreenOrientationOverride parameters", parameters); bidi.send( new Command<>("emulation.setScreenOrientationOverride", parameters.toMap(), Map.class)); } 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-code-review]

PR Code Suggestions ✨

Latest suggestions up to c719c86

Category	Suggestion	Impact
Possible issue	Enforce orientation consistency In ScreenOrientation.java, add validation to the toMap() method to ensure the natural orientation and type are consistent, preventing combinations like a PORTRAIT natural orientation with a landscape type. java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [46-51] public Map<String, Object> toMap() { + // Validate consistency between natural and type + boolean isPortraitType = type == ScreenOrientationType.PORTRAIT_PRIMARY + || type == ScreenOrientationType.PORTRAIT_SECONDARY; + boolean isLandscapeType = type == ScreenOrientationType.LANDSCAPE_PRIMARY + || type == ScreenOrientationType.LANDSCAPE_SECONDARY; + if ((natural == ScreenOrientationNatural.PORTRAIT && !isPortraitType) + || (natural == ScreenOrientationNatural.LANDSCAPE && !isLandscapeType)) { + throw new IllegalArgumentException( + "Inconsistent screen orientation: natural=" + natural + ", type=" + type); + } Map<String, Object> map = new HashMap<>(); map.put("natural", natural.toString()); map.put("type", type.toString()); return map; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential for inconsistent state and proposes a validation check that improves the robustness of the ScreenOrientation class by preventing invalid combinations from being sent.	Medium
General	Add polling to avoid flakiness In SetScreenOrientationOverrideTest.java, replace the immediate assertion with a polling loop that waits for the screen orientation to update before asserting the expected values, preventing potential test flakiness. java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [71-73] -Map<String, Object> currentOrientation = getScreenOrientation(contextId); +Map<String, Object> currentOrientation; +long end = System.currentTimeMillis() + 3000; +do { + currentOrientation = getScreenOrientation(contextId); + if ("landscape-primary".equals(currentOrientation.get("type")) + && Integer.valueOf(0).equals(currentOrientation.get("angle"))) { + break; + } + try { + Thread.sleep(100); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + break; + } +} while (System.currentTimeMillis() < end); assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); assertThat(currentOrientation.get("angle")).isEqualTo(0); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential source of test flakiness due to the asynchronous nature of applying screen orientation changes and proposes a robust polling mechanism to mitigate it.	Medium
Learned best practice	✅ Return unmodifiable serialized map Suggestion Impact: The commit changed toMap() to return Map.of(...), which produces an immutable/unmodifiable map, aligning with the suggestion's intent to return an unmodifiable map. code diff: public Map<String, Object> toMap() { - Map<String, Object> map = new HashMap<>(); - map.put("natural", natural.toString()); - map.put("type", type.toString()); - return map; + return Map.of( + "natural", natural.toString(), + "type", type.toString()); } Return an unmodifiable copy to prevent callers from mutating the returned map; this preserves immutability of serialized state. java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [46-51] public Map<String, Object> toMap() { Map<String, Object> map = new HashMap<>(); map.put("natural", natural.toString()); map.put("type", type.toString()); - return map; + return java.util.Collections.unmodifiableMap(map); } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Use defensive copying and immutability for maps returned by accessors to avoid external mutation.	Low
Possible issue	Omit key instead of using null In the SetScreenOrientationOverrideParameters constructor, use map.remove("screenOrientation") instead of map.put("screenOrientation", null) when the screenOrientation parameter is null. java/src/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideParameters.java [22-28] public SetScreenOrientationOverrideParameters(ScreenOrientation screenOrientation) { if (screenOrientation == null) { - map.put("screenOrientation", null); + map.remove("screenOrientation"); } else { map.put("screenOrientation", screenOrientation.toMap()); } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that omitting an optional field is generally more robust for protocol interactions than sending an explicit null, improving code quality and adherence to best practices.	Low
Update

---

Previous suggestions

✅ Suggestions up to commit b943316

Category	Suggestion	Impact
General	✅ Remove inconsistent and unnecessary page reload Suggestion Impact: The commit removed the page reload (context.navigate) after calling setScreenOrientationOverride in the test, matching the suggestion to eliminate the unnecessary reload. code diff: @@ -64,9 +67,6 @@ emulation.setScreenOrientationOverride( new SetScreenOrientationOverrideParameters(landscapeOrientation) .contexts(List.of(contextId))); - - // Reload the page to apply the orientation change - context.navigate(url, ReadinessState.COMPLETE); Map<String, Object> currentOrientation = getScreenOrientation(contextId); assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); @@ -124,9 +124,6 @@ new SetScreenOrientationOverrideParameters(landscapeOrientation) .userContexts(List.of(userContext))); - // Reload the page to apply the orientation override - context.navigate(url, ReadinessState.COMPLETE); - Map<String, Object> currentOrientation = getScreenOrientation(contextId); assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); assertThat(currentOrientation.get("angle")).isEqualTo(0); Remove the inconsistent and unnecessary page reload after the first setScreenOrientationOverride call to make the test accurately reflect the expected dynamic behavior. java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [68-93] -// Reload the page to apply the orientation change -context.navigate(url, ReadinessState.COMPLETE); - Map<String, Object> currentOrientation = getScreenOrientation(contextId); assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); assertThat(currentOrientation.get("angle")).isEqualTo(0); // Set portrait-secondary orientation ScreenOrientation portraitOrientation = new ScreenOrientation( ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY); emulation.setScreenOrientationOverride( new SetScreenOrientationOverrideParameters(portraitOrientation) .contexts(List.of(contextId))); currentOrientation = getScreenOrientation(contextId); assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary"); assertThat(currentOrientation.get("angle")).isEqualTo(180); // Clear the override emulation.setScreenOrientationOverride( new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId))); currentOrientation = getScreenOrientation(contextId); assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type")); assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an inconsistent and likely unnecessary page reload in the test, improving test correctness and consistency.	Medium
	✅ Use idiomatic null checks for consistency Suggestion Impact: The commit removed manual null checks and assigned fields using Require.nonNull(), aligning with the suggestion. code diff: +import org.openqa.selenium.internal.Require; public class ScreenOrientation { private final ScreenOrientationNatural natural; private final ScreenOrientationType type; public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) { - if (natural == null) { - throw new IllegalArgumentException("Natural orientation cannot be null"); - } - if (type == null) { - throw new IllegalArgumentException("Orientation type cannot be null"); - } - this.natural = natural; - this.type = type; + this.natural = Require.nonNull("natural", natural); + this.type = Require.nonNull("type", type); In the ScreenOrientation constructor, replace manual null checks with the idiomatic Require.nonNull() for consistency with the rest of the codebase. java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [27-36] -public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) { - if (natural == null) { - throw new IllegalArgumentException("Natural orientation cannot be null"); +import org.openqa.selenium.internal.Require; + +public class ScreenOrientation { + // ... + public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) { + this.natural = Require.nonNull("Natural orientation", natural); + this.type = Require.nonNull("Orientation type", type); } - if (type == null) { - throw new IllegalArgumentException("Orientation type cannot be null"); - } - this.natural = natural; - this.type = type; + // ... } Suggestion importance[1-10]: 4 __ Why: The suggestion improves code consistency by proposing the use of the idiomatic Require.nonNull() helper, aligning the new class with existing codebase conventions.	Low
Possible issue	✅ Fetch screen orientation properties atomically Suggestion Impact: The commit replaced two separate executeScript calls with a single atomic call returning both type and angle, and adjusted the return map accordingly. code diff: - String type = (String) executor.executeScript("return screen.orientation.type;"); - Number angle = (Number) executor.executeScript("return screen.orientation.angle;"); + Map<String, Object> orientation = + (Map<String, Object>) + executor.executeScript( + "return { type: screen.orientation.type, angle: screen.orientation.angle };"); - return Map.of("type", type, "angle", angle.intValue()); + return Map.of( + "type", orientation.get("type"), "angle", ((Number) orientation.get("angle")).intValue()); Refactor the getScreenOrientation method to fetch screen orientation type and angle in a single, atomic JavaScript execution to prevent potential race conditions. java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [36-44] private Map<String, Object> getScreenOrientation(String context) { driver.switchTo().window(context); JavascriptExecutor executor = (JavascriptExecutor) driver; - String type = (String) executor.executeScript("return screen.orientation.type;"); - Number angle = (Number) executor.executeScript("return screen.orientation.angle;"); + Map<String, Object> orientation = + (Map<String, Object>) + executor.executeScript( + "return { type: screen.orientation.type, angle: screen.orientation.angle };"); - return Map.of("type", type, "angle", angle.intValue()); + return Map.of("type", orientation.get("type"), "angle", ((Number) orientation.get("angle")).intValue()); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential race condition in a test helper method and proposes a more robust, atomic operation which also slightly improves performance.	Low
Learned best practice	Ensure context cleanup with finally Ensure the created BrowsingContext is closed even if navigation or assertions fail by wrapping its lifecycle in try/finally. java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [49-93] BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle()); -String contextId = context.getId(); +try { + String contextId = context.getId(); -// Navigate to a page first to ensure screen.orientation is available -String url = appServer.whereIs("formPage.html"); -context.navigate(url, ReadinessState.COMPLETE); + // Navigate to a page first to ensure screen.orientation is available + String url = appServer.whereIs("formPage.html"); + context.navigate(url, ReadinessState.COMPLETE); -Map<String, Object> initialOrientation = getScreenOrientation(contextId); + Map<String, Object> initialOrientation = getScreenOrientation(contextId); -Emulation emulation = new Emulation(driver); + Emulation emulation = new Emulation(driver); -// Set landscape-primary orientation -ScreenOrientation landscapeOrientation = - new ScreenOrientation( - ScreenOrientationNatural.LANDSCAPE, ScreenOrientationType.LANDSCAPE_PRIMARY); -emulation.setScreenOrientationOverride( - new SetScreenOrientationOverrideParameters(landscapeOrientation) - .contexts(List.of(contextId))); + // Set landscape-primary orientation + ScreenOrientation landscapeOrientation = + new ScreenOrientation( + ScreenOrientationNatural.LANDSCAPE, ScreenOrientationType.LANDSCAPE_PRIMARY); + emulation.setScreenOrientationOverride( + new SetScreenOrientationOverrideParameters(landscapeOrientation) + .contexts(List.of(contextId))); -// Reload the page to apply the orientation change -context.navigate(url, ReadinessState.COMPLETE); + // Reload the page to apply the orientation change + context.navigate(url, ReadinessState.COMPLETE); -Map<String, Object> currentOrientation = getScreenOrientation(contextId); -assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); -assertThat(currentOrientation.get("angle")).isEqualTo(0); + Map<String, Object> currentOrientation = getScreenOrientation(contextId); + assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary"); + assertThat(currentOrientation.get("angle")).isEqualTo(0); -// Set portrait-secondary orientation -ScreenOrientation portraitOrientation = - new ScreenOrientation( - ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY); -emulation.setScreenOrientationOverride( - new SetScreenOrientationOverrideParameters(portraitOrientation) - .contexts(List.of(contextId))); + // Set portrait-secondary orientation + ScreenOrientation portraitOrientation = + new ScreenOrientation( + ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY); + emulation.setScreenOrientationOverride( + new SetScreenOrientationOverrideParameters(portraitOrientation) + .contexts(List.of(contextId))); -currentOrientation = getScreenOrientation(contextId); -assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary"); -assertThat(currentOrientation.get("angle")).isEqualTo(180); + currentOrientation = getScreenOrientation(contextId); + assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary"); + assertThat(currentOrientation.get("angle")).isEqualTo(180); -// Clear the override -emulation.setScreenOrientationOverride( - new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId))); + // Clear the override + emulation.setScreenOrientationOverride( + new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId))); -currentOrientation = getScreenOrientation(contextId); -assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type")); -assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle")); + currentOrientation = getScreenOrientation(contextId); + assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type")); + assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle")); +} finally { + context.close(); +} Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always wrap creation of external contexts/resources in try/finally and ensure explicit cleanup to prevent leaks on failure.	Low
Possible issue	Omit null field from payload Modify the SetScreenOrientationOverrideParameters constructor to omit the screenOrientation key from the payload when it is null, instead of explicitly setting it to null. java/src/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideParameters.java [22-28] public SetScreenOrientationOverrideParameters(ScreenOrientation screenOrientation) { - if (screenOrientation == null) { - map.put("screenOrientation", null); - } else { + if (screenOrientation != null) { map.put("screenOrientation", screenOrientation.toMap()); } + // If null, omit the key to clear the override } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that omitting a key is often better than sending an explicit null in JSON-RPC protocols, improving robustness and adherence to the BiDi specification.	Medium
	Serialize using enum values In ScreenOrientation.toMap(), use a dedicated getValue() method for serializing the natural and type enums instead of relying on toString(). java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [46-51] public Map<String, Object> toMap() { Map<String, Object> map = new HashMap<>(); - map.put("natural", natural.toString()); - map.put("type", type.toString()); + map.put("natural", natural.getValue()); + map.put("type", type.getValue()); return map; } Suggestion importance[1-10]: 5 __ Why: The suggestion improves code robustness by decoupling serialization logic from the toString() method, which is a good practice even though the current implementation works correctly.	Low
General	Add explicit enum value getter In the ScreenOrientationNatural enum, add an explicit getValue() accessor for the protocol value and change toString() to return the enum's name for better debugging. java/src/org/openqa/selenium/bidi/emulation/ScreenOrientationNatural.java [20-34] public enum ScreenOrientationNatural { PORTRAIT("portrait"), LANDSCAPE("landscape"); private final String value; ScreenOrientationNatural(String value) { this.value = value; } + public String getValue() { + return value; + } + @Override public String toString() { - return value; + return name(); } } Suggestion importance[1-10]: 5 __ Why: This is a good code quality suggestion that improves maintainability by separating the enum's serialization value from its string representation, preventing potential future bugs.	Low

[asolntsev]

P.S. Welcome to the club! DOTNET tests are failing, though you only changed Java code. :(

[asolntsev]

Looks good, thanks!