Repository navigation
[Group review] feat(locator): press, focus, doubleClick, rightClick - #3
crystalmountain666 wants to merge 5 commits into
Conversation
|
Thanks @crystalmountain666 and @TONGdamy, this looks good. The diff only touches the files the issue needs. Happy to approve. Just make sure the Verification section is filled in with the test numbers before opening the upstream PR. Minor note: could remove the |
Lisr-iiil
left a comment
There was a problem hiding this comment.
Thanks for putting this together. I reviewed the implementation, unit tests, Javadoc, and documentation against seleniumboot#134 and the maintainer’s guidance.
The four methods follow the existing terminal-action contract through whenReady() / whenVisible(): the element is resolved again on every poll, and missing or stale elements are retried until timeouts.explicit. Action execution remains inside the retry loop, so staleness during the action is also retried.
The tests cover eight scenarios for each action, plus the enabled-state check for doubleClick(). The cases for staleness during an action and persistent staleness address the maintainer’s concern about stale references being treated as success. The pointer tests also check the target element, mouse button, and click count. The Javadoc, guide, and changelog are consistent with the implementation.
This review is based on inspecting the code, tests, and documentation. I have not independently run the test suite or performed local browser verification.
I have two questions about the verification notes:
- Issue seleniumboot#134 mentions a real-page case in the sample project if browser testing is needed. The mock tests check the retry logic and action construction, but actual focus changes, keyboard behaviour, and mouse events depend on the browser. Could you clarify whether a real-page case was also run and include its assertions and results?
- CONTRIBUTING.md describes running the official consumer project against the local framework build. Could you confirm whether this was completed and add the results to the notes?
I couldn’t find these details in the current description, so I’m asking for clarification rather than assuming the checks haven’t been done.
This internal PR should remain unmerged as requested.
BasePage already offers doubleClick/rightClick, but Locator did not, so users had to drop out of the Locator API for these gestures. Both methods perform the gesture with Selenium's Actions API and go through the existing terminal-action helpers, so they follow the same contract as click() and hover() (seleniumboot#67/seleniumboot#68): the element is re-resolved from scratch on every poll, and stale or not-yet-present elements are retried until timeouts.explicit. A StaleElementReferenceException thrown while the gesture is performed is retried as well, rather than being treated as success. - doubleClick() waits for the element to be visible and enabled, like click(). - rightClick() waits for the element to be visible, like hover(). Each method has Javadoc with a usage example. Tests follow in the next commit. Refs seleniumboot#134
…eleniumboot#134) Add LocatorActionsTest, which checks that doubleClick() and rightClick() honour the same terminal-action contract as click() (seleniumboot#67/seleniumboot#68). The tests mock the driver as Interactive so the Actions sequences can be captured, and set timeouts.explicit to 1s to keep the timeout cases fast. Each pointer action is run through a shared data provider covering: - a ready element: the gesture is performed once on one lookup, and the element is never cleared or plain-clicked - an element that appears later: the locator re-resolves first - an element that is hidden at first: the action waits for it to become visible - an element that goes stale during the readiness check: a fresh element is resolved and used - an element that goes stale during the gesture: the gesture is retried on a freshly resolved element - an element that never appears: LocatorException with a TimeoutException cause, and no gesture is performed - an element that stays stale during the gesture: the action times out instead of reporting success - an element that never becomes visible: times out without acting A separate test checks that doubleClick() waits until the element is enabled. The successful cases also decode the captured gesture to check the target element, the number of press/release pairs (2 for doubleClick, 1 for rightClick) and the mouse button (left for doubleClick, right for rightClick). The data provider is shared so that the keyboard and focus actions can be added to the same matrix. Refs seleniumboot#134
754f2af to
e433f47
Compare
Internal group review before opening the PR to seleniumboot/selenium-boot. Please approve or leave comments. This PR should not be merged into
master, so thatmasterstays in sync with upstream.The branch is rebased onto the current upstream
masterand contains the five commits already reviewed in the internal PRs #1 (pointer actions) and #2 (keyboard actions and docs). The draft upstream PR description is below.@May-Mei6 @Lisr-iiil could you review this when you have time? Thanks!
What & why
Locatorhad nodoubleClick(),rightClick(),focus()orpress(Keys...), so tests had to drop back toBasePageor raw WebDriver for these interactions. This PR adds all four so theLocatorAPI is the only place users need to look.All four are terminal actions and follow the contract from seleniumboot#67/seleniumboot#68: the locator is re-resolved from scratch on every poll, and stale or not-yet-present elements are retried until
timeouts.explicit. AStaleElementReferenceExceptionthrown while the action is performed is retried as well, so a stale reference is never treated as success.doubleClick()waits for the element to be visible and enabled, likeclick()andBasePage.doubleClick().rightClick()waits for the element to be visible, likehover()andBasePage.rightClick().press(Keys...)sends the keys to the element without clearing it.focus()moves focus to the element with JavaScript.Closes seleniumboot#134.
Type of change
Checklist
mvn clean verify) (see Verification)@SeleniumBootApipublic APIs (or a major version bump is justified below)docs-site/)Notes for reviewers
Commits. The change is split into five commits:
doubleClick()/rightClick(), their tests,press(Keys...)/focus(), their tests, and the docs (changelog entry indocs-site/docs/changelog.mdand one line in the locators guide).Tests. The new
LocatorActionsTestruns all four actions through a shared data provider covering eight scenarios: a ready element, an element that appears later, an element that is hidden at first, staleness during the readiness check, staleness during the action, an element that never appears, an element that stays stale (must time out instead of succeeding), and an element that never becomes visible. A separate test checks thatdoubleClick()waits until the element is enabled. For the pointer actions, the tests also decode the captured gesture to check the target element, the number of press/release pairs and the mouse button. The tests are in their own file, followingLocatorDragToTest.Verification.
mvn clean verifyon the rebased branch (macOS, IntelliJ): [TOTAL] tests, [FAILURES] failures. [Describe the failures here, or delete this sentence if there are none.] AllLocatorActionsTesttests pass.Co-authored by @crystalmountain666 and @TONGdamy.