[java] Refactored selectByContainsVisibleText and selectByVisibleText methods to remove code duplication

[vicky-iv]

User description

🔗 Related Issues

[🚀 Feature]: Unifying Select Class Across All Bindings #15265

💥 What does this PR do?

This is a tiny addition to my previous PR #16220
Refactored selectByContainsVisibleText and selectByVisibleText methods to remove code duplication

🔧 Implementation Notes

This change was inspired by the current .NET implementation, where there is only one method for both cases, using a boolean switch to toggle between partial and exact matches.
I preserved the existing Java interface (and existing implementation of both methods) by extracting the common selection logic into a new private method selectByVisibleText(String text, boolean isPartialMatch), and reused it across the existing selectByVisibleText and selectByContainsVisibleText methods.

All tests from the following packages are passed locally:
//java/test/org/openqa/selenium/support/ui/...

💡 Additional Considerations

🔄 Types of changes

• Cleanup (refactoring)

---

PR Type

Other

---

Description

• Extracted common logic into private selectByVisibleText method

• Added boolean parameter to toggle partial/exact matching

• Eliminated duplicate code between two selection methods

• Preserved existing public interface and behavior

---

Diagram Walkthrough

flowchart LR
  A["selectByVisibleText(text)"] --> C["selectByVisibleText(text, false)"]
  B["selectByContainsVisibleText(text)"] --> D["selectByVisibleText(text, true)"]
  C --> E["Common Selection Logic"]
  D --> E

Loading

File Walkthrough

Relevant files

Miscellaneous

Select.java Refactor text selection methods to eliminate duplication  java/src/org/openqa/selenium/support/ui/Select.java Replaced duplicate code in selectByVisibleText and selectByContainsVisibleText methods Added private overloaded method with boolean parameter for partial matching Consolidated selection logic into single reusable implementation Maintained existing public API and behavior +69/-96

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavior Parity Ensure the new shared method preserves exact-match semantics for selectByVisibleText and partial-match semantics for selectByContainsVisibleText across all edge cases (whitespace trimming, multi-select early return, and visibility checks) compared to the previous implementations. private void selectByVisibleText(String text, boolean isPartialMatch) { assertSelectIsEnabled(); assertSelectIsVisible(); // try to find the option via XPATH ... List<WebElement> options = element.findElements( By.xpath(".//option[normalize-space(.) = " + Quotes.escape(text) + "]")); for (WebElement option : options) { if (!hasCssPropertyAndVisible(option)) throw new NoSuchElementException("Invisible option with text: " + text); setSelected(option, true); if (!isMultiple()) { return; } } boolean matched = !options.isEmpty(); if (!matched) { String searchText = text.contains(" ") ? getLongestSubstringWithoutSpace(text) : text; List<WebElement> candidates; if (searchText.isEmpty()) { candidates = element.findElements(By.tagName("option")); } else { candidates = element.findElements( By.xpath(".//option[contains(., " + Quotes.escape(searchText) + ")]")); } String trimmed = text.trim(); for (WebElement option : candidates) { boolean isMatchedOptionFound = isPartialMatch ? option.getText().contains(trimmed) : option.getText().trim().equals(trimmed); if (isMatchedOptionFound) { if (!hasCssPropertyAndVisible(option)) throw new NoSuchElementException("Invisible option with text: " + text); setSelected(option, true); if (!isMultiple()) { return; } matched = true; } } } if (!matched) { throw new NoSuchElementException("Cannot locate option with text: " + text); } } Performance The method performs multiple XPath queries and iterates candidates; consider validating that this does not regress performance on large option lists, especially with repeated calls in multi-select scenarios. // try to find the option via XPATH ... List<WebElement> options = element.findElements( By.xpath(".//option[normalize-space(.) = " + Quotes.escape(text) + "]")); for (WebElement option : options) { if (!hasCssPropertyAndVisible(option)) throw new NoSuchElementException("Invisible option with text: " + text); setSelected(option, true); if (!isMultiple()) { return; } } boolean matched = !options.isEmpty(); if (!matched) { String searchText = text.contains(" ") ? getLongestSubstringWithoutSpace(text) : text; List<WebElement> candidates; if (searchText.isEmpty()) { candidates = element.findElements(By.tagName("option")); } else { candidates = element.findElements( By.xpath(".//option[contains(., " + Quotes.escape(searchText) + ")]")); } i18n Matching Verify that contains vs equals(trim) logic maintains correct behavior for non-Latin scripts and whitespace normalization, matching previous behavior for inputs like Korean examples noted in the Javadoc. String trimmed = text.trim(); for (WebElement option : candidates) { boolean isMatchedOptionFound = isPartialMatch ? option.getText().contains(trimmed) : option.getText().trim().equals(trimmed); if (isMatchedOptionFound) { if (!hasCssPropertyAndVisible(option))

[selenium-ci]

Thank you, @vicky-iv for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add input normalization and null checks Normalize text upfront and guard against null/blank to avoid NPEs and incorrect XPath building. Also ensure the XPath exact-match search uses the trimmed value consistently with later comparisons. java/src/org/openqa/selenium/support/ui/Select.java [342-399] private void selectByVisibleText(String text, boolean isPartialMatch) { assertSelectIsEnabled(); assertSelectIsVisible(); + if (text == null || text.trim().isEmpty()) { + throw new NoSuchElementException("Cannot locate option with text: " + text); + } + String trimmed = text.trim(); + // try to find the option via XPATH ... List<WebElement> options = element.findElements( - By.xpath(".//option[normalize-space(.) = " + Quotes.escape(text) + "]")); + By.xpath(".//option[normalize-space(.) = " + Quotes.escape(trimmed) + "]")); for (WebElement option : options) { if (!hasCssPropertyAndVisible(option)) throw new NoSuchElementException("Invisible option with text: " + text); setSelected(option, true); if (!isMultiple()) { return; } } -... - String trimmed = text.trim(); - for (WebElement option : candidates) { - boolean isMatchedOptionFound = - isPartialMatch - ? option.getText().contains(trimmed) - : option.getText().trim().equals(trimmed); - if (isMatchedOptionFound) { - if (!hasCssPropertyAndVisible(option)) - throw new NoSuchElementException("Invisible option with text: " + text); + boolean matched = !options.isEmpty(); + if (!matched) { + // try to find via the various matching strategies + String searchText = trimmed.contains(" ") ? getLongestSubstringWithoutSpace(trimmed) : trimmed; + List<WebElement> candidates; + if ("".equals(searchText)) { + candidates = element.findElements(By.tagName("option")); + } else { + candidates = + element.findElements( + By.xpath(".//option[contains(., " + Quotes.escape(searchText) + ")]")); + } - setSelected(option, true); + for (WebElement option : candidates) { + boolean isMatchedOptionFound = + isPartialMatch + ? option.getText().contains(trimmed) + : option.getText().trim().equals(trimmed); - if (!isMultiple()) { - return; + if (isMatchedOptionFound) { + if (!hasCssPropertyAndVisible(option)) + throw new NoSuchElementException("Invisible option with text: " + text); + + setSelected(option, true); + + if (!isMultiple()) { + return; + } + matched = true; } - matched = true; } + } + + if (!matched) { + throw new NoSuchElementException("Cannot locate option with text: " + text); } } -if (!matched) { - throw new NoSuchElementException("Cannot locate option with text: " + text); -} - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This is a high-impact suggestion that correctly identifies multiple issues: a potential NullPointerException if text is null, and inconsistent use of text vs text.trim() for XPath queries and later comparisons. The proposed changes fix these issues, improving both correctness and robustness.	Medium
	Guard against null or blank input Handle null or empty input to avoid potential NullPointerExceptions and unnecessary tokenization. Return an empty string early when s is null or blank. java/src/org/openqa/selenium/support/ui/Select.java [151-161] private String getLongestSubstringWithoutSpace(String s) { + if (s == null) { + return ""; + } + String trimmedInput = s.trim(); + if (trimmedInput.isEmpty()) { + return ""; + } String result = ""; - StringTokenizer st = new StringTokenizer(s, " "); + StringTokenizer st = new StringTokenizer(trimmedInput, " "); while (st.hasMoreTokens()) { String t = st.nextToken(); if (t.length() > result.length()) { result = t; } } return result; } Apply / Chat Suggestion importance[1-10]: 5 __ Why: This suggestion correctly identifies that getLongestSubstringWithoutSpace can throw a NullPointerException if the input s is null, and proposes a valid fix, making the helper method more robust.	Low
General	Validate partial-match input early Validate the text argument before use to prevent ambiguous matching and surprises. Throw a clear NoSuchElementException when text is null or empty after trim, consistent with other methods' error behavior. java/src/org/openqa/selenium/support/ui/Select.java [146-149] @Override public void selectByContainsVisibleText(String text) { + if (text == null || text.trim().isEmpty()) { + throw new NoSuchElementException("Cannot locate option with text: " + text); + } selectByVisibleText(text, true); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out the lack of input validation for text which could lead to a NullPointerException and adds a necessary null/empty check, improving the robustness of the public API.	Medium
Update

[vicky-iv]

Accidentally created from a wrong branch