Fixed bug #13642 related to relative locators

[antoinebrunet1]

trafficstars

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 https://github.com/SeleniumHQ/selenium/issues/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

• [x] 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

• [x] I have read the contributing document.
• [x] My change requires a change to the documentation.
• [x] I have updated the documentation accordingly.
• [x] 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

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro[bot]]

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-merge-pro[bot]]

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]

@antoinebrunet1 can you run the format script, please?

[antoinebrunet1]

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

[diemol]

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

[antoinebrunet1]

I ran it. What is the next step?

[diemol]

Push the corrections it performed.

[antoinebrunet1]

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

[diemol]

Here is the diff after running the format script

[diemol]

I've run the script and pushed the diff

[diemol]

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

[antoinebrunet1]

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.

[bischoffdev]

Is this still worked on?