Skip to content

Refactor isDisplayed atom to remove Closure library code #15528

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 2 commits into
base: trunk
Choose a base branch
from

Conversation

jimevans
Copy link
Member

@jimevans jimevans commented Mar 28, 2025

User description

The Closure library adds a lot of complexity to the compiled and minified atoms, including for isDisplayed. Additionally, the web platform has evolved in the time since the atoms were originally written, with additional methods added that help simplify the atom. This commit removes (nearly) all of the Closure code from the specific atom used by the isDisplayed method in the various language bindings.

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

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

Enhancement, Bug fix


Description

  • Refactored isDisplayed atom to remove Closure library dependencies.

  • Simplified visibility checks using modern web platform methods.

  • Improved handling of <map> and <area> elements for visibility determination.

  • Enhanced overflow and size-based visibility logic for better accuracy.


Changes walkthrough 📝

Relevant files
Enhancement
dom.js
Refactored `isDisplayed` atom for modern web standards     

javascript/atoms/dom.js

  • Replaced Closure library methods with modern JavaScript APIs.
  • Added new helper functions for and visibility checks.
  • Enhanced logic for handling overflow and size-based visibility.
  • Improved compatibility with shadow DOM and modern web standards.
  • +219/-69

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • The Closure library adds a lot of complexity to the compiled and minified atoms,
    including for isDisplayed. Additionally, the web platform has evolved in the
    time since the atoms were originally written, with additional methods added that
    help simplify the atom. This commit removes (nearly) all of the Closure code
    from the specific atom used by the isDisplayed method in the various language
    bindings.
    @CLAassistant
    Copy link

    CLAassistant commented Mar 28, 2025

    CLA assistant check
    Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
    1 out of 2 committers have signed the CLA.

    ✅ diemol
    ❌ jimevanssfdc
    You have signed the CLA already but the status is still pending? Let us recheck it.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Bug

    The getAreaRelativeRect function doesn't properly parse coordinates as numbers, which could lead to incorrect calculations. The coordinates are split from a string but not converted to numbers before mathematical operations.

    const getAreaRelativeRect = (area) => {
      const shape = area.shape.toLowerCase();
      const coords = area.coords.split(',');
      if (shape == 'rect' && coords.length == 4) {
        const [x, y] = coords;
        return new DOMRect(x, y,coords[2] - x, coords[3] - y);
      } else if (shape == 'circle' && coords.length == 3) {
        const [centerX, centerY, radius] = coords;
        return new DOMRect(centerX - radius, centerY - radius, 2 * radius, 2 * radius);
      } else if (shape == 'poly' && coords.length > 2) {
        let [minX, minY] = coords, maxX = minX, maxY = minY;
        for (var i = 2; i + 1 < coords.length; i += 2) {
          minX = Math.min(minX, coords[i]);
          maxX = Math.max(maxX, coords[i]);
          minY = Math.min(minY, coords[i + 1]);
          maxY = Math.max(maxY, coords[i + 1]);
        }
        return new DOMRect(minX, minY, maxX - minX, maxY - minY);
      }
      return new DOMRect();
    };
    Edge Case Handling

    The overflow detection logic in checkIsHiddenByOverflow might not handle all edge cases correctly, particularly with nested scrollable containers and fixed position elements. The scroll position calculation could be improved.

    const checkIsHiddenByOverflow = (elem, style) => {
      const htmlElement = elem.ownerDocument.documentElement;
    
      const getNearestOverflowAncestor = (e, style) => {
        const elementPosition = style.getPropertyValue('position');
        const canBeOverflowed = (container) => {
          const containerStyle = getComputedStyle(container);
          if (container === htmlElement) {
            return true;
          }
          const containerDisplay = containerStyle.getPropertyValue('display');
          if (containerDisplay.startsWith('inline') || containerDisplay === 'contents') {
            return false;
          }
          const containerPosition = containerStyle.getPropertyValue('position');
          if (elementPosition === 'absolute' && containerPosition === 'static') {
            return false;
          }
          return true;
        };
        if (elementPosition === 'fixed') {
          return e === htmlElement ? null : htmlElement;
        }
        let container = e.parentElement;
        while (container && !canBeOverflowed(container)) {
          container = container.parentElement;
        }
        return container;
      };
    
      // Walk up the tree, examining each ancestor capable of displaying
      // overflow.
      let parentElement = getNearestOverflowAncestor(elem, style);
      while (parentElement) {
        const parentStyle = getComputedStyle(parentElement);
        const parentOverflowX = parentStyle.getPropertyValue('overflow-x');
        const parentOverflowY = parentStyle.getPropertyValue('overflow-y');
    
        // If the container has overflow:visible, the element cannot be hidden in its overflow.
        if (parentOverflowX !== 'visible' || parentOverflowY !== 'visible') {
          const parentRect = parentElement.getBoundingClientRect();
    
          // Zero-sized containers without overflow:visible hide all descendants.
          if (parentRect.width === 0 || parentRect.height === 0) {
            return true;
          }
    
          const elementRect = elem.getBoundingClientRect();
    
          // Check "underflow": if an element is to the left or above the container
          // and overflow is "hidden" in the proper direction, the element is hidden.
          const isLeftOf = elementRect.x + elementRect.width < parentRect.x;
          const isAbove = elementRect.y + elementRect.height < parentRect.y;
          if ((isLeftOf && parentOverflowX === 'hidden') ||
              (isAbove && parentOverflowY === 'hidden')) {
            return true;
          }
    
          // Check "overflow": if an element is to the right or below a container
          // and overflow is "hidden" in the proper direction, the element is hidden.
          const isRightOf = elementRect.x >= parentRect.x + parentRect.width;
          const isBelow = elementRect.y >= parentRect.y + parentRect.height;
          if ((isRightOf && parentOverflowX === 'hidden') ||
              (isBelow && parentOverflowY === 'hidden')) {
            return true;
          } else if ((isRightOf && parentOverflowX !== 'visible') ||
              (isBelow && parentOverflowY !== 'visible')) {
            // Special case for "fixed" elements: whether it is hidden by
            // overflow depends on the scroll position of the parent element
            if (style.getPropertyValue('position') === 'fixed') {
              const scrollPosition = parentElement.tagName === 'HTML' ?
                {
                  x: parentElement.ownerDocument.scrollingElement.scrollLeft,
                  y: parentElement.ownerDocument.scrollingElement.scrollTop
                } :
                {
                  x: parentElement.scrollLeft,
                  y: parentElement.scrollTop
                };
              if ((elementRect.x >= htmlElement.scrollWidth - scrollPosition.x) ||
                  (elementRect.y >= htmlElement.scrollHeight - scrollPosition.y)) {
                return true;
              }
            }
          }
        }
        parentElement = getNearestOverflowAncestor(parentElement, parentStyle);
      }
      return false;
    };
    Browser Compatibility

    The use of checkVisibility() method may not be supported in all browsers that Selenium targets. This could cause compatibility issues if not properly feature-detected or polyfilled.

    const checkElementVisibility = (elem) => {
      return elem.checkVisibility({
        visibilityProperty: true,
        opacityProperty: !ignoreOpacity,
        contentVisibilityAuto: true
      });
    };

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 28, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Parse string coordinates to numbers

    Fix the parsing of coordinates in the rectangle shape handler. The coordinates
    are currently being used as strings, which will cause incorrect calculations.
    Parse them to numbers first.

    javascript/atoms/dom.js [467-468]

    -const [x, y] = coords;
    -return new DOMRect(x, y,coords[2] - x, coords[3] - y);
    +const [x, y] = coords.map(Number);
    +return new DOMRect(x, y, Number(coords[2]) - x, Number(coords[3]) - y);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This is a critical fix. Without parsing the coordinates to numbers, JavaScript will treat them as strings, leading to string concatenation instead of numeric subtraction, causing incorrect rectangle calculations and potential bugs in element visibility detection.

    High
    Convert string coordinates to numbers

    The circle coordinates need to be parsed as numbers before performing arithmetic
    operations. Without parsing, JavaScript will perform string concatenation
    instead of numeric subtraction.

    javascript/atoms/dom.js [470-471]

    -const [centerX, centerY, radius] = coords;
    +const [centerX, centerY, radius] = coords.map(Number);
     return new DOMRect(centerX - radius, centerY - radius, 2 * radius, 2 * radius);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This is a critical fix for the circle shape handler. Without converting the string coordinates to numbers, arithmetic operations will result in string concatenation instead of numeric calculations, leading to incorrect DOMRect dimensions.

    High
    Convert polygon coordinates to numbers

    The polygon coordinates need to be parsed as numbers for proper comparison.
    Using Math.min and Math.max with string values can lead to incorrect results due
    to string comparison rules.

    javascript/atoms/dom.js [473-479]

    -let [minX, minY] = coords, maxX = minX, maxY = minY;
    +let [minX, minY] = coords.map(Number), maxX = minX, maxY = minY;
     for (var i = 2; i + 1 < coords.length; i += 2) {
    -  minX = Math.min(minX, coords[i]);
    -  maxX = Math.max(maxX, coords[i]);
    -  minY = Math.min(minY, coords[i + 1]);
    -  maxY = Math.max(maxY, coords[i + 1]);
    +  minX = Math.min(minX, Number(coords[i]));
    +  maxX = Math.max(maxX, Number(coords[i]));
    +  minY = Math.min(minY, Number(coords[i + 1]));
    +  maxY = Math.max(maxY, Number(coords[i + 1]));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This is a critical fix for polygon coordinate handling. String comparisons in Math.min/max will produce incorrect results when determining bounding rectangles, potentially causing visibility detection failures for polygon-shaped area elements.

    High
    • Update

    @jimevans
    Copy link
    Member Author

    @AutomatedTester @shs96c Here's the patch we paired on at SeConf in Valencia. Feel free to merge at your discretion.

    @titusfortner
    Copy link
    Member

    @jimevans you have to sign the CLA for @jimevanssfdc

    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.

    5 participants