Skip to content

[py][bidi]: add unhandled_prompt_behavior param for create_user_context#16112

Merged
navin772 merged 4 commits intoSeleniumHQ:trunkfrom
navin772:unhandled_prompt_behavior-param-browser
Aug 1, 2025
Merged

[py][bidi]: add unhandled_prompt_behavior param for create_user_context#16112
navin772 merged 4 commits intoSeleniumHQ:trunkfrom
navin772:unhandled_prompt_behavior-param-browser

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Jul 31, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

💥 What does this PR do?

Adds support for the optional unhandled_prompt_behavior parameter for browser module's create_user_context command - https://w3c.github.io/webdriver-bidi/#command-browser-createUserContext

🔧 Implementation Notes

  1. The required UserPromptHandler and UserPromptHandlerType types are added to session.py since in the BiDi spec they come under the Session module types.

  2. Most of the WPT tests are failing for this parameter so currently I am verifying if the command supports the parameter in browsers (which it does). We also set unhandled_prompt_behavior to IGNORE in conftest.py which may affect the tests.

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement

Description

  • Add unhandled_prompt_behavior parameter to create_user_context method

  • Implement UserPromptHandler and UserPromptHandlerType classes

  • Add test for new prompt behavior functionality

Diagram Walkthrough

flowchart LR
  A["UserPromptHandler class"] --> B["create_user_context method"]
  B --> C["BiDi browser context"]
  C --> D["Prompt handling configuration"]
Loading

File Walkthrough
Relevant files
Enhancement
browser.py
Add prompt behavior parameter to context creation               

py/selenium/webdriver/common/bidi/browser.py

  • Add unhandled_prompt_behavior parameter to create_user_context method
  • Import UserPromptHandler from session module
  • Update method signature and parameter handling
 +13/-3   
session.py
Implement user prompt handler classes                                       

py/selenium/webdriver/common/bidi/session.py

  • Add UserPromptHandlerType class with valid prompt handler constants
  • Implement UserPromptHandler class with validation and serialization
  • Support alert, confirm, prompt, and other dialog types
 +82/-0   
Tests
bidi_browser_tests.py
Add test for prompt behavior configuration                             

py/test/selenium/webdriver/common/bidi_browser_tests.py

  • Add test for create_user_context with unhandled_prompt_behavior
  • Import new prompt handler classes
  • Test context creation and basic functionality
 +27/-0   

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 31, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jul 31, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

• Fix JavaScript execution in link's href attribute when using click() method
• Ensure compatibility with Firefox 42.0
• Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

5678 - Not compliant

Non-compliant requirements:

• Fix ChromeDriver connection failure error on Ubuntu 16.04.4
• Resolve "ConnectFailure (Connection refused)" error for subsequent ChromeDriver instances
• Ensure first ChromeDriver instance works without console errors
• Support Chrome 65.0.3325.181 with ChromeDriver version 2.35

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incomplete Test

The test for unhandled prompt behavior is incomplete with commented out functionality and a TODO note. The test doesn't actually verify that the prompt handling works as expected, making it ineffective for validating the new feature.

 
# TODO: trigger an alert and test that it is dismissed automatically, currently not working,
#  conftest.py unhandled_prompt_behavior set to IGNORE, see if it is related
# driver.find_element(By.ID, "alert").click()
# # accessing title should be possible since alert is auto handled
# assert driver.title == "Testing Alerts"
Missing Validation

The UserPromptHandler class doesn't validate that at least one handler type is provided during initialization, which could lead to creating empty handler objects that serve no purpose.

 
def __init__(
    self,
    alert: Optional[str] = None,
    before_unload: Optional[str] = None,
    confirm: Optional[str] = None,
    default: Optional[str] = None,
    file: Optional[str] = None,
    prompt: Optional[str] = None,
):
    """Initialize UserPromptHandler.

    Parameters:
    -----------
        alert: Handler type for alert prompts
        before_unload: Handler type for beforeUnload prompts
        confirm: Handler type for confirm prompts
        default: Default handler type for all prompts
        file: Handler type for file picker prompts
        prompt: Handler type for prompt dialogs

    Raises:
    ------
        ValueError: If any handler type is not valid
    """
    for field_name, value in [
        ("alert", alert),
        ("before_unload", before_unload),
        ("confirm", confirm),
        ("default", default),
        ("file", file),
        ("prompt", prompt),
    ]:
        if value is not None and value not in UserPromptHandlerType.VALID_TYPES:
            raise ValueError(
                f"Invalid {field_name} handler type: {value}. Must be one of {UserPromptHandlerType.VALID_TYPES}"
            )

    self.alert = alert
    self.before_unload = before_unload
    self.confirm = confirm
    self.default = default
    self.file = file
    self.prompt = prompt

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jul 31, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
✅ Simplify dictionary building logic
Suggestion Impact:The suggestion was directly implemented - the repetitive if statements were replaced with a field_mapping dictionary and a loop that uses getattr() to dynamically access attributes and build the result dictionary

code diff:

+        field_mapping = {
+            "alert": "alert",
+            "before_unload": "beforeUnload",
+            "confirm": "confirm",
+            "default": "default",
+            "file": "file",
+            "prompt": "prompt",
+        }
+
         result = {}
-        if self.alert is not None:
-            result["alert"] = self.alert
-        if self.before_unload is not None:
-            result["beforeUnload"] = self.before_unload
-        if self.confirm is not None:
-            result["confirm"] = self.confirm
-        if self.default is not None:
-            result["default"] = self.default
-        if self.file is not None:
-            result["file"] = self.file
-        if self.prompt is not None:
-            result["prompt"] = self.prompt
+        for attr_name, dict_key in field_mapping.items():
+            value = getattr(self, attr_name)
+            if value is not None:
+                result[dict_key] = value

The method manually checks each attribute and builds the dictionary, which is
repetitive and error-prone. Use a mapping approach to dynamically build the
result dictionary from instance attributes.

py/selenium/webdriver/common/bidi/session.py [80-100]

 def to_dict(self) -> Dict[str, str]:
     """Convert the UserPromptHandler to a dictionary for BiDi protocol.
 
     Returns:
     -------
         Dict[str, str]: Dictionary representation suitable for BiDi protocol
     """
+    field_mapping = {
+        'alert': 'alert',
+        'before_unload': 'beforeUnload',
+        'confirm': 'confirm',
+        'default': 'default',
+        'file': 'file',
+        'prompt': 'prompt',
+    }
+    
     result = {}
-    if self.alert is not None:
-        result["alert"] = self.alert
-    if self.before_unload is not None:
-        result["beforeUnload"] = self.before_unload
-    if self.confirm is not None:
-        result["confirm"] = self.confirm
-    if self.default is not None:
-        result["default"] = self.default
-    if self.file is not None:
-        result["file"] = self.file
-    if self.prompt is not None:
-        result["prompt"] = self.prompt
+    for attr_name, dict_key in field_mapping.items():
+        value = getattr(self, attr_name)
+        if value is not None:
+            result[dict_key] = value
     return result

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: This suggestion improves maintainability by replacing a series of if statements with a more scalable, loop-based approach, making it easier to add new fields in the future.

Low
Use dynamic parameter validation

The validation loop iterates over hardcoded field names and values, making it
fragile to changes. Use locals() or instance variables to dynamically validate
all parameters without hardcoding field names.

py/selenium/webdriver/common/bidi/session.py [60-71]

-for field_name, value in [
-    ("alert", alert),
-    ("before_unload", before_unload),
-    ("confirm", confirm),
-    ("default", default),
-    ("file", file),
-    ("prompt", prompt),
-]:
+params = {
+    'alert': alert,
+    'before_unload': before_unload,
+    'confirm': confirm,
+    'default': default,
+    'file': file,
+    'prompt': prompt,
+}
+
+for field_name, value in params.items():
     if value is not None and value not in UserPromptHandlerType.VALID_TYPES:
         raise ValueError(
             f"Invalid {field_name} handler type: {value}. Must be one of {UserPromptHandlerType.VALID_TYPES}"
         )
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion offers a minor stylistic improvement by separating data from logic, but the proposed code is functionally equivalent and no less "fragile" than the original.

Low
  • Update

@navin772 navin772 requested review from cgoldberg and shbenzer and removed request for shbenzer July 31, 2025 10:09
cgoldberg
cgoldberg requested changes Jul 31, 2025
View reviewed changes
Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

You can use the native dict instead of importing Dict from typing.

I also like the "Simplify dictionary building logic" suggestion from the review bot, but that's up to you.

other than that, LGTM

@navin772 navin772 requested a review from cgoldberg July 31, 2025 14:57
@navin772 navin772 merged commit 8cf788f into SeleniumHQ:trunk Aug 1, 2025
28 of 29 checks passed
@navin772 navin772 deleted the unhandled_prompt_behavior-param-browser branch August 1, 2025 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgoldberg cgoldberg cgoldberg approved these changes

@shbenzer shbenzer Awaiting requested review from shbenzer

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@navin772 @cgoldberg @selenium-ci