Repository navigation
fix(page-controller): match select options by value as well as text - #750
Li-john1021 wants to merge 1 commit into
Conversation
selectOptionElement only compared trimmed textContent, so i18n and value≠label dropdowns failed when the agent passed the option value shown in dehydrated DOM (value is in DEFAULT_INCLUDE_ATTRIBUTES). Prefer exact textContent (unchanged), then fall back to option.value. Update select_dropdown_option tool copy and add happy-dom coverage. Fixes alibaba#749
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Li-john1021 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c3c6e5448
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // as values while showing localized textContent — agents pass either. | ||
| const option = | ||
| options.find((opt) => (opt.textContent?.trim() ?? '') === needle) ?? | ||
| options.find((opt) => opt.value === needle || opt.value === optionText) |
There was a problem hiding this comment.
When optionText has surrounding whitespace and a select contains value="foo" before value=" foo " (with unrelated labels), this find accepts the first option via needle before it ever tests the later exact raw value. The advertised exact value selection therefore chooses the wrong option; use separate passes that prefer opt.value === optionText before any trimmed/normalized fallback.
AGENTS.md reference: AGENTS.md:L152-L152
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
What
selectOptionElementpreviously matched options only via exacttextContent. That breaks common cases whereoption.valuediffers from the visible label (i18n, codes likeapple/international), even though dehydrated DOM already exposesvalue=...and agents often pass that value.This change keeps exact textContent first (same behavior as today for ambiguous needles), then falls back to exact
option.value. Theselect_dropdown_optiontool description is updated accordingly. Happy-dom regression tests cover text, value/i18n, reject path, and text-first ambiguity.Closes #749
Related (orthogonal — they change setter/events, not matching): #704, #686, #718.
Type
Testing
npm run cipassespackages/page-controller)Repro before fix:
<option value="apple">苹果</option>+selectOptionElement(select, 'apple')→ not found. After fix → selectsapple.Requirements / 要求
Thank you for reviewing — happy to adjust the match order or wording if you prefer a different approach.