improve "near" relative locator behaviour by alpatron · Pull Request #11290 · SeleniumHQ/selenium

alpatron · 2022-11-21T22:26:54Z

Description

I changed the implementation and behaviour of the "near" relative locator.

The documentation states that the near locator finds elements that "are at most 50px away from the provided locator".¹
Currently, the code accomplishes this by first checking some kinds of edge-to-edge distances to determine one way of closeness. Then, if this check fails, it checks to see if the centres of the bounding rectangles are within the specified distance. If this check fails as well, the elements are deemed not close. This approach breaks in certain cases as described in issue #11272.

I replaced and simplified this algorithm like so:

One element's bounding rectangle is scaled up, adding the specified look-up distance to its width and height, keeping the centre of the rectangle the same.
We check if the scaled-up bounding box intersects with the non-scaled-up bounding box of the second element.
The intersection checking is performed by the built-in ìntersects method, since the code uses goog.math.Rect objects that have this feature.²

This diagram might better illustrate how this approach works:

It should be noted that this thus changes the behaviour of the near locator, and existing code that expects precisely the old behaviour of the near locator might break, but since the behaviour of the near locator was always just specified as finding elements "at most 50px away from the provided locator", I don't believe that much code exists that utilises the at-times-odd behaviour of the current near locator. On the other hand, this new implementation locates elements that seemingly should be located by this locator but aren't, as described in issue #11272.

Another noteworthy mention is that the way I implemented the locator doesn't use Euclidean distance to determine "nearness", but rather Chebyshev distance. A diagram probably best describes the difference. Shown is what a proper Euclidean near area would look like.

If proper Euclidean distance is deemed crucial to be used in Selenium, it's possible to implement it by using the algorithm for example on this StackOverflow post (https://stackoverflow.com/a/26178015). But this would require more complex and longer code, and I think that the rectangular Chebyshev approach is sufficient for use and probably more natural to use than the proper Euclidean way, as I'd believe most developers think in regular rectangles rather than rectangles with rounded corners.

Technically, the documentation need not change, since the code still looks for elements that "are at most 50px away from the provided locator". What has changed is how that definition is interpreted. But given this definition is not unambiguous, the documentation probably ought to be updated in any case—whether this PR is accepted or not. I suppose if this PR is deemed acceptable, I could update the relevant documentation.

I modified the relative_locators.html file to trigger the edge case behaviour in issue #11272.

I had some trouble with tests: I developed this PR using gitpod, as was recommended in the README, but some tests were failing even on a fresh repo with the latest commit from the trunk branch with no changes made. I don't know if this a problem with gitpod or whether this is expected behaviour. But after performing the fix, no extra failures were recorded. All the relative-locator tests are passing.

Motivation and Context

Fixes issue #11272. The near relative behaviour sometimes doesn't find elements that seem like they should be within 50px of the referenced element.

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.

CLAassistant · 2022-11-21T22:27:02Z

All committers have signed the CLA.

diemol

Thank you for this, @alpatron.

I believe we would need a blog post to describe the changes, which is basically the description of the PR you are sending.

Also, have you checked the relative locators tests? Don't mind the unrelated failing ones. This should probably break most of them across bindings.

alpatron · 2023-03-20T17:58:40Z

Hi, @diemol.

I checked the relative-locator tests (at least the ones that I was able to run—Python, JavaScript, and Java), and they all pass. Well, they pass now: I had modified the relative_locators.html file to trigger the edge case behaviour in issue #11272, but since some tests were using that file for the testing of relative-locator result ordering (the order of elements returned by driver.find_elements), my modification broke those tests. So I rolled back the change to the relative_locators.html file and now this PR only includes the change to the relative-locator implementation.

This brings me to another point: Since this is a change to the Selenium atoms, I assume that it's needed to write tests that verify the fix for all language bindings. Given I can get only the Python, JavaScript, and Java tests to run on the provided gitpod environment, this could be difficult. Or are there atom-specific tests?

Then I have a question about some code in the atoms: The Selenium documentation says that, by default, the "near" relative locator finds elements that are "at most 50px away from the provided locator". However, it seems that the code is using a default distance of 100 px. See the code excerpt from the unmodified relative.js file.

selenium/javascript/atoms/locators/relative.js

Lines 142 to 153 in 6d94706

    
           bot.locators.relative.near_ = function (selector, opt_distance) { 
        
             var distance; 
        
             if (opt_distance) { 
        
               distance = opt_distance; 
        
             } else if (goog.isNumber(selector['distance'])) { 
        
               distance = /** @type {number} */ (selector['distance']); 
        
               // delete selector['distance']; 
        
             } 
        
             if (!distance) { 
        
               distance = 100; 
        
             }

It seems that the code is setting the default distance to 100 px, not 50 px. And I don't think this has to do with weird geometry rules either. The relative.js code was doing some edge-to-edge and centre-distance comparisons, nothing where some geometrical compensation would be necessary. See the code excerpt for the current "nearness" check.

selenium/javascript/atoms/locators/relative.js

Lines 166 to 211 in 6d94706

    
           var rect1 = bot.dom.getClientRect(element); 
        
           var rect2 = bot.dom.getClientRect(compareTo); 
        
           // Ascii art time! 
        
           // 
        
           //    +---+ 
        
           //    | 1 | 
        
           //    +---+     +---+ 
        
           //              | 2 | 
        
           //              +---+ 
        
           // 
        
           // As you can see, the right hand side of 1 is "left of" the left-most 
        
           // edge of 2. The top edge of 2 is at the same level as the bottom 
        
           // edge of 1. This means that 1 is "above" 2, and 2 is "below" one. 
        
           // Distance from left edge to right edge 
        
           var leftDistance = Math.abs(rect1.left - (rect2.left + rect2.width)); 
        
           // Distance from right edge to left edge 
        
           var rightDistance = Math.abs((rect1.left + rect1.width) - rect2.left); 
        
           // Distance from top to bottom 
        
           var topDistance = Math.abs(rect1.top - (rect2.top + rect2.height)); 
        
           // Distance from bottom to top 
        
           var bottomDistance = Math.abs((rect1.top + rect1.height) - rect2.top); 
        
           var horizontallyClose = leftDistance <= distance || rightDistance <= distance; 
        
           var verticallyClose = topDistance <= distance || bottomDistance <= distance; 
        
           if (horizontallyClose && verticallyClose) { 
        
             return true; 
        
           } 
        
           // Distance from centre points 
        
           var x1 = rect1.left + (rect1.width / 2); 
        
           var y1 = rect1.top + (rect1.height / 2); 
        
           var x2 = rect2.left + (rect2.width / 2); 
        
           var y2 = rect2.top + (rect2.height / 2); 
        
           var xDistance = Math.abs(x1 - x2); 
        
           var yDistance = Math.abs(y1 - y2); 
        
           var dist = Math.sqrt(Math.pow(xDistance, 2) + Math.pow(yDistance, 2)); 
        
           return dist <= distance;

I believe that the distance = 100 assignment is done in error and that the "near" relative locator is using a default distance of 100 px instead of 50 px. But I might be wrong, and there might be something I'm not aware of. Either way, if the locator should find elements within 50 px, then I believe I'd need to change the default distance = 100 assignment to distance = 50 because my changed code definitely expects distance to be, well, the desired distance from the source element. However, if Selenium has actually been looking for elements in a 100-px distance for all these years, then I suppose it could be better to keep the 100-px default distance for better backward compatibility and update the documentation with info about the bug.

And finally, speaking of the current way of the "near" locator, I also created a simple illustration to illustrate how the current way works. It'd be nice to get a second pair of eyes to look at it so I could check whether I understand the current way of how the "near" relative locator works. The red R2 rectangles are examples of "near" elements that the current way deems as not near, but my changed code would deem as near.

diemol · 2023-03-30T19:00:51Z

I was doing a quick check and it does seem a couple of languages do pass the distance as 50px, but others end up with the default.

I was not able to understand what breaks and what does not from your comment. I will let our tests run and see what breaks. I guess we need to modify relative_locators.html so the case can be tested and then adjust tests. Plus, also make all bindings use the 50px distance.

diemol · 2023-03-30T20:08:57Z

After checking the CI, I believe we need to make changes in the relative_locators.html to properly test the change. If you want, please add a test in the language you prefer and I can help replicate it across the other languages.

Changes the behaviour of the "near" relative locator to use the distance between the two rectangles' boundaries for determining closeness. The relative_locators.html file is changed so that the center cell and cell number two's centers are far away but their boundaries are near each other. Fixes SeleniumHQ#11272

alpatron · 2023-04-03T13:04:36Z

Hi, @diemol.

I added tests for the Python bindings. I changed the relative_locators.html file by adding the buggy case I described in issue #11272. I added two tests: One test that should find a rectangle near the other. And the other than should not, because the rectangles are more than 50 px from each other.

I also changed the default search distance in relative.js to 50. As I said, I don't know whether previously the default value of 100 was a bug or whether it was needed for some weird reason, but my new code needs this value to be 50 to look within 50 px of the anchor.

codecov-commenter · 2023-06-02T10:38:43Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (ea82c66) 55.69% compared to head (3586eda) 55.69%.

❗ Current head 3586eda differs from pull request most recent head fb48141. Consider uploading reports for the commit fb48141 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #11290   +/-   ##
=======================================
  Coverage   55.69%   55.69%           
=======================================
  Files          86       86           
  Lines        5421     5421           
  Branches      223      223           
=======================================
  Hits         3019     3019           
  Misses       2179     2179           
  Partials      223      223

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

diemol

Thank you, @alpatron!

alpatron mentioned this pull request Nov 21, 2022

[🐛 Bug]: "Near" relative locator fails to find elements under certain conditions #11272

Closed

titusfortner added the B-atoms JavaScript chunks generated by Google closure label Nov 28, 2022

diemol force-pushed the improve-near-locator branch from b5f8b66 to d8bb2cd Compare March 6, 2023 13:53

diemol reviewed Mar 6, 2023

View reviewed changes

alpatron force-pushed the improve-near-locator branch 6 times, most recently from 0476cb5 to bb15e39 Compare March 20, 2023 16:05

alpatron force-pushed the improve-near-locator branch from d53bc62 to 7663053 Compare April 3, 2023 12:49

Fix default distance in relative locator

e5b60d1

alpatron force-pushed the improve-near-locator branch from 7663053 to 603591f Compare April 3, 2023 13:00

Add Python tests

dc53e48

alpatron force-pushed the improve-near-locator branch from 603591f to dc53e48 Compare April 4, 2023 12:39

diemol added this to the 4.10 milestone Apr 18, 2023

diemol added 3 commits May 2, 2023 21:21

Merge branch 'trunk' into improve-near-locator

2f447ed

Merge branch 'trunk' into improve-near-locator

e8c7cde

Merge branch 'trunk' into improve-near-locator

c351445

diemol and others added 3 commits June 2, 2023 17:29

[java] Adding test in Java and setting default to 50px

0de4f49

[py] Fixing linter

5019e35

Merge branch 'trunk' into improve-near-locator

fb48141

diemol approved these changes Jun 2, 2023

View reviewed changes

diemol merged commit d10f8c2 into SeleniumHQ:trunk Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

improve "near" relative locator behaviour#11290

improve "near" relative locator behaviour#11290
diemol merged 9 commits intoSeleniumHQ:trunkfrom
alpatron:improve-near-locator

alpatron commented Nov 21, 2022 •

edited

Loading

Uh oh!

CLAassistant commented Nov 21, 2022 •

edited

Loading

Uh oh!

diemol left a comment

Uh oh!

alpatron commented Mar 20, 2023

Uh oh!

diemol commented Mar 30, 2023 •

edited

Loading

Uh oh!

diemol commented Mar 30, 2023

Uh oh!

alpatron commented Apr 3, 2023

Uh oh!

codecov-commenter commented Jun 2, 2023 •

edited

Loading

Uh oh!

diemol left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

alpatron commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

Checklist

Footnotes

Uh oh!

CLAassistant commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

alpatron commented Mar 20, 2023

Uh oh!

diemol commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diemol commented Mar 30, 2023

Uh oh!

alpatron commented Apr 3, 2023

Uh oh!

codecov-commenter commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alpatron commented Nov 21, 2022 •

edited

Loading

CLAassistant commented Nov 21, 2022 •

edited

Loading

diemol commented Mar 30, 2023 •

edited

Loading

codecov-commenter commented Jun 2, 2023 •

edited

Loading