Skip to content

[py][bidi]: Implement low-level API for Input BiDi module #16049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Jul 14, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the BiDi INPUT module for the python bindings - https://w3c.github.io/webdriver-bidi/#module-input

🔧 Implementation Notes

The PR adds 3 commands and 1 event (input.fileDialogOpened).

Commands:

  1. perform_actions
  2. release_actions
  3. set_files

Event handlers:

  1. add_file_dialog_handler
  2. remove_file_dialog_handler

💡 Additional Considerations

This is a low-level API and thus I have added support for all the types mentioned in the BiDi spec.

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Implement BiDi INPUT module with 3 commands and 1 event

  • Add comprehensive action classes for keyboard, pointer, and wheel inputs

  • Include file dialog event handling with subscription management

  • Provide extensive test coverage for all input functionality


Diagram Walkthrough

flowchart LR
  A["Input Module"] --> B["Action Classes"]
  A --> C["Commands"]
  A --> D["Event Handling"]
  B --> E["Key/Pointer/Wheel Actions"]
  C --> F["perform_actions/release_actions/set_files"]
  D --> G["File Dialog Events"]
Loading

File Walkthrough

Relevant files
Enhancement
input.py
New BiDi INPUT module implementation                                         

py/selenium/webdriver/common/bidi/input.py

  • Complete BiDi INPUT module implementation with action classes
  • Three main commands: perform_actions, release_actions, set_files
  • File dialog event handling with subscription management
  • Comprehensive type definitions for all input action types
+474/-0 
webdriver.py
WebDriver integration for INPUT module                                     

py/selenium/webdriver/remote/webdriver.py

  • Add input property to WebDriver class
  • Initialize _input attribute in constructor
  • Provide lazy loading of Input module instance
+25/-0   
Bug fix
script.py
Script execute method null handling fix                                   

py/selenium/webdriver/common/bidi/script.py

  • Fix execute method to handle None result values
  • Return empty dict when result is None instead of None
+1/-1     
Tests
bidi_input_tests.py
Complete INPUT module test suite                                                 

py/test/selenium/webdriver/common/bidi_input_tests.py

  • Comprehensive test suite for all INPUT module functionality
  • Tests for keyboard, pointer, wheel, and combined actions
  • File handling and dialog event testing
  • Edge cases and error scenarios coverage
+415/-0 

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 14, 2025
@navin772 navin772 requested a review from Copilot July 16, 2025 11:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the BiDi INPUT module for Python WebDriver bindings, providing low-level API access to the W3C WebDriver BiDi input specification. The implementation adds comprehensive input action support including keyboard, pointer, and wheel interactions, plus file dialog event handling.

Key changes include:

  • Implementation of all BiDi input commands (perform_actions, release_actions, set_files)
  • Support for keyboard, pointer, and wheel input sources with proper action sequences
  • File dialog event handling with add/remove handler functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
py/selenium/webdriver/common/bidi/input.py Core implementation of BiDi input module with action classes, source actions, and Input class
py/selenium/webdriver/remote/webdriver.py Integration of input module as a property in WebDriver class
py/test/selenium/webdriver/common/bidi_input_tests.py Comprehensive test suite covering all input functionality

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

• Fix issue where JavaScript in link's href is not triggered on click() in version 2.48
• Ensure compatibility with Firefox 42.0 32bit on 64bit machine
• Restore functionality that worked in version 2.47.1

5678 - Not compliant

Non-compliant requirements:

• Fix ConnectFailure (Connection refused) error when instantiating ChromeDriver
• Resolve issue affecting Ubuntu 16.04.4 with Linux 4.10.0
• Fix problem with Selenium 3.9.0, Chrome 65.0.3325.181, ChromeDriver 2.35
• Address error occurring on subsequent ChromeDriver instantiations (first instance works fine)

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 Security concerns

File path exposure:
The set_files method accepts arbitrary file paths from user input without validation, potentially allowing access to sensitive system files if not properly sanitized by the calling code. The test code creates temporary files but the production method should validate file paths and permissions.

⚡ Recommended focus areas for review

Validation Logic

The validation logic in PointerCommonProperties.post_init uses hardcoded mathematical constants that may not handle edge cases properly, particularly for altitude_angle and azimuth_angle bounds checking.

def __post_init__(self):
    if self.width < 1:
        raise ValueError("width must be at least 1")
    if self.height < 1:
        raise ValueError("height must be at least 1")
    if not (0.0 <= self.pressure <= 1.0):
        raise ValueError("pressure must be between 0.0 and 1.0")
    if not (0.0 <= self.tangential_pressure <= 1.0):
        raise ValueError("tangential_pressure must be between 0.0 and 1.0")
    if not (0 <= self.twist <= 359):
        raise ValueError("twist must be between 0 and 359")
    if not (0.0 <= self.altitude_angle <= math.pi / 2):
        raise ValueError("altitude_angle must be between 0.0 and π/2")
    if not (0.0 <= self.azimuth_angle <= 2 * math.pi):
        raise ValueError("azimuth_angle must be between 0.0 and 2π")
Resource Management

The Input class maintains subscription and callback state but lacks proper cleanup mechanisms in case of connection failures or unexpected disconnections.

def __init__(self, conn):
    self.conn = conn
    self.subscriptions = {}
    self.callbacks = {}
Test Reliability

Several tests use hardcoded timing with time.sleep() and WebDriverWait without proper error handling, which could lead to flaky test behavior in CI environments.

time.sleep(1)
assert len(file_dialog_events) == 0

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Convert action sequences to lists

The dataclass fields use field(default_factory=list) which creates mutable lists
directly. When these action sequences are passed from external sources, they
should be converted to lists to ensure mutability and consistency. This prevents
issues when the stored collection needs to support operations or when
maintaining consistent internal storage format.

py/selenium/webdriver/common/bidi/input.py [274-336]

 actions: List[PauseAction] = field(default_factory=list)
-actions: List[Union[PauseAction, KeyDownAction, KeyUpAction]] = field(default_factory=list)
-actions: List[Union[PauseAction, PointerDownAction, PointerUpAction, PointerMoveAction]] = field(
-    default_factory=list
-)
-actions: List[Union[PauseAction, WheelScrollAction]] = field(default_factory=list)
 
+def __post_init__(self):
+    self.actions = list(self.actions)
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Convert sequences to lists when storing them internally to ensure mutability and consistency, especially when the stored collection needs to support operations like append() or when maintaining a consistent internal storage format.

Low
General
Fix cleanup order for consistency

The method should call self.conn.remove_callback() before modifying internal
state to ensure proper cleanup order. If the connection removal fails, the
internal state remains consistent.

py/selenium/webdriver/common/bidi/input.py [454-474]

 def remove_file_dialog_handler(self, callback_id: int) -> None:
     """Remove a file dialog handler.
 
     Parameters:
     -----------
         callback_id: The callback ID returned by add_file_dialog_handler.
     """
+    self.conn.remove_callback(FileDialogOpened, callback_id)
+    
     if callback_id in self.callbacks:
         del self.callbacks[callback_id]
 
     if FileDialogOpened.event_class in self.subscriptions:
         if callback_id in self.subscriptions[FileDialogOpened.event_class]:
             self.subscriptions[FileDialogOpened.event_class].remove(callback_id)
 
         # If no more callbacks for this event, unsubscribe
         if not self.subscriptions[FileDialogOpened.event_class]:
             session = Session(self.conn)
             self.conn.execute(session.unsubscribe(FileDialogOpened.event_class))
             del self.subscriptions[FileDialogOpened.event_class]
 
-    self.conn.remove_callback(FileDialogOpened, callback_id)
-
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion to reorder cleanup operations is a minor improvement for robustness, but the current implementation is not incorrect.

Low
  • More

@navin772 navin772 requested a review from cgoldberg July 25, 2025 12:00
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 Possible security concern Review effort 4/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants