Skip to content
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

[py] Fix select being able to select options hidden by css rules #15135

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

Conversation

FFederi
Copy link

@FFederi FFederi commented Jan 23, 2025

User description

Description

This is my first time trying to contribute on open source!

I noticed some inconsistencies regarding the behaviour of select_by_visible_text() type of functions, if we look at java bindings:

throw new NoSuchElementException("Invisible option with text: " + text);

we can see that selectByContainsVisibleText() checks if any css rule is hiding the option element before matching, while selectByVisibleText() does not:

public void selectByVisibleText(String text) {

I tried updating the python bindings and tests to match the java ones behaviour (raising NoSuchElementException on elements hidden by css rules), but I still haven't checked if other languages are the same or not.
I tried running tests on a Gitpod environment and they seem fine.
I think that the following things should be done:
● selectByVisibleText() in Select.java should also raise an exception on a hidden option
● maybe checking the other languages for the same kind of bug
● updating tests

Motivation and Context

Without this kind of check I think it ends up selecting options not visible to the user, rendering things like End to End tests a little bit more unreliable.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Tests


Description

  • Updated select_by_visible_text to raise exceptions for hidden options.

  • Added _has_css_property_and_visible method to check CSS visibility.

  • Enhanced tests to validate behavior with hidden options.

  • Ensured consistency with Java bindings for hidden option handling.


Changes walkthrough 📝

Relevant files
Bug fix
select.py
Add CSS visibility checks to selection methods                     

py/selenium/webdriver/support/select.py

  • Added _has_css_property_and_visible method to check visibility.
  • Updated select_by_visible_text and deselect_by_visible_text to raise
    exceptions for hidden options.
  • Ensured CSS visibility checks for visibility, display, and opacity.
  • +15/-0   
    Tests
    select_class_tests.py
    Add tests for hidden options in selection                               

    py/test/selenium/webdriver/common/select_class_tests.py

  • Added test for hidden options in multi-select.
  • Defined invisibleMultiSelect test data for hidden options.
  • Ensured exception is raised for hidden options in tests.
  • +7/-0     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • @CLAassistant
    Copy link

    CLAassistant commented Jan 23, 2025

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Edge Cases

    The CSS property check in _has_css_property_and_visible() may not catch all cases of hidden elements. Consider checking parent element visibility and other CSS properties like height/width.

    def _has_css_property_and_visible(self, option) -> bool:
        css_value_candidates = ["hidden", "none", "0", "0.0"]
        css_property_candidates = ["visibility", "display", "opacity"]
    
        for property in css_property_candidates:
            css_value = option.value_of_css_property(property)
            if css_value in css_value_candidates:
                return False
        return True
    Error Handling

    The NoSuchElementException is raised before checking other potential matches. This could prevent finding a visible option if multiple options have the same text.

    if not self._has_css_property_and_visible(opt):
        raise NoSuchElementException(f"Could not locate element with visible text: {text}")

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Fix premature exception for hidden options
    Suggestion Impact:The commit addresses the same issue but with a different approach - instead of collecting visible options first, it changes the error message to be more descriptive about invisible options

    code diff:

    @@ -115,7 +115,7 @@
             matched = False
             for opt in opts:
                 if not self._has_css_property_and_visible(opt):
    -                raise NoSuchElementException(f"Could not locate element with visible text: {text}")
    +                raise NoSuchElementException(f"Invisible option with text: {text}")
                 self._set_selected(opt)
                 if not self.is_multiple:
                     return

    The current implementation raises NoSuchElementException too early in the loop,
    preventing checking of other matching options that might be visible. Move the
    visibility check after examining all options.

    py/selenium/webdriver/support/select.py [116-119]

    -for opt in opts:
    -    if not self._has_css_property_and_visible(opt):
    -        raise NoSuchElementException(f"Could not locate element with visible text: {text}")
    +visible_opts = [opt for opt in opts if self._has_css_property_and_visible(opt)]
    +if not visible_opts:
    +    raise NoSuchElementException(f"Could not locate element with visible text: {text}")
    +for opt in visible_opts:
         self._set_selected(opt)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The current implementation incorrectly raises an exception on the first hidden option, potentially missing visible options that match the text. The suggested change correctly checks all options before raising the exception.

    9
    Add case-insensitive CSS value comparison

    CSS property values comparison should be case-insensitive as CSS values can be in
    different cases. Convert values to lowercase before comparison.

    py/selenium/webdriver/support/select.py [250-257]

     css_value_candidates = ["hidden", "none", "0", "0.0"]
     css_property_candidates = ["visibility", "display", "opacity"]
     for property in css_property_candidates:
    -    css_value = option.value_of_css_property(property)
    +    css_value = option.value_of_css_property(property).lower()
         if css_value in css_value_candidates:
             return False
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: CSS property values can be returned in different cases (e.g., "NONE" vs "none"), making the current comparison unreliable. Adding case-insensitive comparison prevents potential bugs with browser-specific CSS value formats.

    8

    …en if the argument text has spaces
    
    Fix error messages to be the same as java's one
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants