[java][js] Fixed bug #13642 related to relative locators by antoinebrunet1 · Pull Request #14336 · SeleniumHQ/selenium

antoinebrunet1 · 2024-08-02T14:07:17Z

User description

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.

Description

I created a browser test in Chrome called shouldBeAbleToFindElementWithToRightOfAndBelowWithBorderCollapseInTable reproducing the steps of the bug in the java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java Java class and got "Mexico".

I made all my modifications to the logic of the project inside the javascript/atoms/locators/relative.js JavaScript file. I modified the bot.locators.relative.below_ and the bot.locators.relative.rightOf_ functions and added a util function called twoElementsAreCellsOfATableThatUsesBorderCollapse.

Before my modifications, inside the bot.locators.relative.below_ function, for the nested function we returned, we only considered that B was below A if the top of B was below the bottom of A. In a table that has collapsing borders, neighboring cells touch each other. For the the bot.locators.relative.below_ function, I don't call the bot.locators.relative.proximity_ function since I need access to the elements, not just rect1 and rect2. I return a function in which I consider that B is below A also if the top of B is at the same vertical position as the bottom of A and if the two elements are cells of the same table. To verify the first part (before the "and") of the condition I added, I call a utility function I created called twoElementsAreCellsOfATableThatUsesBorderCollapse which takes in the two elements and returns true only if the condition described by its name is true. I modified the documentation of the function bot.locators.relative.below_. The old documentation had the two elements switched. My new documentation does not contain that switch.

Before my modifications, inside the bot.locators.relative.rightOf_ function, for the nested function we returned, we only considered that B was to the right of A if the left of B was to the right of the right of A. For the the bot.locators.relative.rightOf_ function, I don't call the bot.locators.relative.proximity_ function since I need access to the elements, not just rect1 and rect2. I return a function in which I consider that B is to the right of A also if the left of B is at the same horizontal position as the right of A and if the two elements are cells of the same table. To verify the first part (before the "and") of the condition I added, I call again the twoElementsAreCellsOfATableThatUsesBorderCollapse function.

As I was informed on the Slack application, the tests will be run after I make my pull request. I only ran the test I created, which passed.

Motivation and Context

The bug which can be found at #13642 happened in the table of the first example at https://www.w3schools.com/html/html_tables.aspFor. The table has 7 rows and 3 columns. I added the values of the first 3 rows below where each line represents a row and the values of each row are separated by a comma and a space. All cells are td elements apart from the cells of the first row which are th elements. The table has "collapse" as the value of the "border-collapse" CSS property.

Company, Contact, Country
Alfreds Futterkiste, Maria Anders, Germany
Centro comercial Moctezuma, Francisco Chang, Mexico

The person who raised the bug was trying, in Chrome and using relative locators, to get the cell that was to the right of the cell containing "Alfreds Futterkiste" and below the cell containing "Contact". They asserted that the text of the cell they were getting was "Maria Anders" but got "Germany" instead.

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

Added a new test case in RelativeLocatorTest.java to verify the bug fix for relative locators in tables with border-collapse.
Modified the below_ function in relative.js to correctly identify elements below each other in tables with border-collapse.
Modified the rightOf_ function in relative.js to correctly identify elements to the right of each other in tables with border-collapse.
Introduced a new utility function twoElementsAreCellsOfSameTable to check if two elements are cells of the same table.

Changes walkthrough 📝

Relevant files

Tests

RelativeLocatorTest.java `Add test case for verifying relative locators with border-collapse` java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java Added a new test case to reproduce and verify the bug fix. The test case checks for correct element identification in a table with border-collapse.	+14/-0

Bug fix

relative.js `Fix relative locators for tables with border-collapse` javascript/atoms/locators/relative.js Modified `below_` function to handle elements in tables with border-collapse. Modified `rightOf_` function to handle elements in tables with border-collapse. Added utility function `twoElementsAreCellsOfSameTable` to check table cell relationships.	+40/-17

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

The bug which can be found at SeleniumHQ#13642 happened in the table of the first example at https://www.w3schools.com/html/html_tables.aspFor. The table has 7 rows and 3 columns. I added the values of the first 3 rows below where each line represents a row and the values of each row are separated by a comma and a space. All cells are td elements apart from the cells of the first row which are th elements. The table has "collapse" as the value of the "border-collapse" CSS property. Company, Contact, Country Alfreds Futterkiste, Maria Anders, Germany Centro comercial Moctezuma, Francisco Chang, Mexico The person who raised the bug was trying, in Chrome and using relative locators, to get the cell that was to the right of the cell containing "Alfreds Futterkiste" and below the cell containing "Contact". They asserted that the text of the cell they were getting was "Maria Anders" but got "Germany" instead. I created a browser test in Chrome called shouldBeAbleToFindElementWithToRightOfAndBelowWithBorderCollapseInTable reproducing the steps of the bug in the java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java Java class and got "Mexico". I made all my modifications to the logic of the project inside the javascript/atoms/locators/relative.js JavaScript file. I modified the bot.locators.relative.below_ and the bot.locators.relative.rightOf_ functions and added a util function called twoElementsAreCellsOfATableThatUsesBorderCollapse. Before my modifications, inside the bot.locators.relative.below_ function, for the nested function we returned, we only considered that B was below A if the top of B was below the bottom of A. In a table that has collapsing borders, neighboring cells touch each other. For the the bot.locators.relative.below_ function, I don't call the bot.locators.relative.proximity_ function since I need access to the elements, not just rect1 and rect2. I return a function in which I consider that B is below A also if the top of B is at the same vertical position as the bottom of A and if the two elements are cells of the same table. To verify the first part (before the "and") of the condition I added, I call a utility function I created called twoElementsAreCellsOfATableThatUsesBorderCollapse which takes in the two elements and returns true only if the condition described by its name is true. I modified the documentation of the function bot.locators.relative.below_. The old documentation had the two elements switched. My new documentation does not contain that switch. Before my modifications, inside the bot.locators.relative.rightOf_ function, for the nested function we returned, we only considered that B was to the right of A if the left of B was to the right of the right of A. For the the bot.locators.relative.rightOf_ function, I don't call the bot.locators.relative.proximity_ function since I need access to the elements, not just rect1 and rect2. I return a function in which I consider that B is to the right of A also if the left of B is at the same horizontal position as the right of A and if the two elements are cells of the same table. To verify the first part (before the "and") of the condition I added, I call again the twoElementsAreCellsOfATableThatUsesBorderCollapse function. As I was informed on the Slack application, the tests will be run after I make my pull request. I only ran the test I created, which passed. Fixes SeleniumHQ#13642

CLAassistant · 2024-08-02T14:07:24Z

All committers have signed the CLA.

qodo-code-review · 2024-08-02T14:07:47Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review Possible Bug The new implementation of `below_` and `rightOf_` methods in `relative.js` might not handle all edge cases correctly, especially when elements are not part of a table but their coordinates match the new conditions added. This could lead to incorrect element matching. Code Clarity The function `twoElementsAreCellsOfSameTable` checks if both elements are table cells and belong to the same table. However, the function name and its implementation could be clearer in expressing that it checks for both cell type and same table membership.

qodo-code-review · 2024-08-02T14:08:27Z

PR Code Suggestions ✨

Category	Suggestion	Score
Robustness	Add error handling to improve robustness of table cell comparison Add error handling in the `twoElementsAreCellsOfSameTable` function to manage cases where `element` or `compareTo` are not found or are not within a table. javascript/atoms/locators/relative.js [116-117] +if (!element \|\| !compareTo) return false; var elementTable = element.closest(cssSelector); var compareToTable = compareTo.closest(cssSelector); +if (!elementTable \|\| !compareToTable) return false; Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding error handling enhances the robustness of the function, ensuring it handles edge cases where elements might not be found or are not within a table, thus preventing potential runtime errors.	9
Maintainability	✅ Improve test maintainability by externalizing URLs and locators Suggestion Impact: The suggestion to externalize the URL was implemented by replacing the direct URL with a method call to retrieve the URL. However, the locators were not externalized as suggested. code diff: - driver.get("https://www.w3schools.com/html/html_tables.asp"); + driver.get(appServer.whereIs("relative_locators.html")); Consider using a more robust and maintainable approach to locate elements by avoiding direct URL navigation and XPath expressions in tests. Instead, use page objects or externalize the locators and URLs. java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java [40-45] -driver.get("https://www.w3schools.com/html/html_tables.asp"); +driver.get(appServer.whereIs("html_tables_page")); ... -.toRightOf(By.xpath("//td[text()='Alfreds Futterkiste']")) -.below(By.xpath("//th[text()='Contact']")); +.toRightOf(locators.get("AlfredsFutterkiste")) +.below(locators.get("ContactHeader")); Suggestion importance[1-10]: 8 Why: Externalizing URLs and locators improves maintainability and readability, making the tests easier to update and less prone to errors when changes occur.	8
Performance	Optimize the table comparison logic for clarity and performance Refactor the `twoElementsAreCellsOfSameTable` function to use a more efficient method for checking if two elements are from the same table, by comparing their parent `table` elements directly. javascript/atoms/locators/relative.js [116-118] -var elementTable = element.closest(cssSelector); -var compareToTable = compareTo.closest(cssSelector); -var elementAndCompareToAreFromSameTable = elementTable.isEqualNode(compareToTable); +var elementAndCompareToAreFromSameTable = element.closest('table') === compareTo.closest('table'); Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggested change simplifies the code and improves performance by directly comparing the parent table elements, making the logic clearer and more efficient.	7
Best practice	Use constants for repeated string values to enhance maintainability Consider using constants for repeated strings such as tag names and selectors to avoid typos and facilitate changes. javascript/atoms/locators/relative.js [111-115] -var tableCellsTagNames = ["TD", "TH"]; -var cssSelector = "table"; +const TABLE_CELL_TAG_NAMES = ["TD", "TH"]; +const TABLE_SELECTOR = "table"; Apply this suggestion Suggestion importance[1-10]: 6 Why: Using constants for repeated strings reduces the risk of typos and makes it easier to update values in one place, improving code maintainability and readability.	6

diemol · 2024-08-02T16:23:06Z

@antoinebrunet1 can you run the format script, please?

antoinebrunet1 · 2024-08-02T16:25:39Z

@diemol What is the format script and how do I run it?

diemol · 2024-08-02T16:29:31Z

https://github.com/SeleniumHQ/selenium/blob/trunk/scripts/format.sh

antoinebrunet1 · 2024-08-02T16:41:30Z

I ran it. What is the next step?

diemol · 2024-08-02T17:21:44Z

Push the corrections it performed.

antoinebrunet1 · 2024-08-02T18:48:23Z

After running the script, I don't see any changes in the "Commit" section of IntelliJ in the left panel.

diemol · 2024-08-07T17:28:37Z

Here is the diff after running the format script

diemol · 2024-08-07T18:01:42Z

I've run the script and pushed the diff

diemol · 2024-08-07T18:30:56Z

Tests are failing https://github.com/SeleniumHQ/selenium/actions/runs/10289366776/job/28476949729?pr=14336#step:15:3235

antoinebrunet1 · 2024-08-08T17:44:19Z

The page of the link you shared keeps crashing for me. I was able to download the logs but they contain weird characters that look like question marks in boxes.

page called relative_locators.html which was already existing and used that page for my browser test

bischoffdev · 2024-11-05T15:31:01Z

Is this still worked on?

qodo-code-review Bot added P-bug fix PR addresses a known issue tests Review effort [1-5]: 3 labels Aug 2, 2024

Merge branch 'trunk' into bug-13642-fixing

44736fd

Running format script

a160044

Merge branch 'trunk' into bug-13642-fixing

9f5e0a9

diemol and others added 4 commits August 9, 2024 16:46

Merge branch 'trunk' into bug-13642-fixing

790e819

Added a table with collapsing borders for my browser test to the test

46a1281

page called relative_locators.html which was already existing and used that page for my browser test

Merge branch 'trunk' into bug-13642-fixing

5eb41f4

Merge branch 'trunk' into bug-13642-fixing

9523ef1

VietND96 changed the title ~~Fixed bug #13642 related to relative locators~~ [java][js] Fixed bug #13642 related to relative locators Nov 15, 2024

titusfortner removed the tests label Mar 30, 2025

github-actions Bot added the J-stale Applied to issues that become stale, and eventually closed. label Sep 26, 2025

github-actions Bot closed this Oct 12, 2025

MohabMohie mentioned this pull request Jul 27, 2026

[java] Add regression coverage for relative locators on collapsed table borders (#13642) #17827

Merged

7 of 9 tasks

Uh oh!

Conversation

antoinebrunet1 commented Aug 2, 2024 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

CLAassistant commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 2, 2024

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review Bot commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

diemol commented Aug 2, 2024

Uh oh!

antoinebrunet1 commented Aug 2, 2024

Uh oh!

diemol commented Aug 2, 2024

Uh oh!

antoinebrunet1 commented Aug 2, 2024

Uh oh!

diemol commented Aug 2, 2024

Uh oh!

antoinebrunet1 commented Aug 2, 2024

Uh oh!

diemol commented Aug 7, 2024

Uh oh!

diemol commented Aug 7, 2024

Uh oh!

diemol commented Aug 7, 2024

Uh oh!

antoinebrunet1 commented Aug 8, 2024

Uh oh!

bischoffdev commented Nov 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

antoinebrunet1 commented Aug 2, 2024 •

edited by qodo-code-review Bot

Loading

CLAassistant commented Aug 2, 2024 •

edited

Loading

qodo-code-review Bot commented Aug 2, 2024 •

edited

Loading