Fix directional relative selectors to prefer closest element over deepest#16
Open
gdealmeida1885 wants to merge 5 commits intodevicelab-dev:mainfrom
Open
Conversation
Contributor
|
@gdealmeida1885 Thank you, There is conflict, if you can resolve it great else I ll do |
CheckCondition() was passing when condition selectors (visible/notVisible)
directly to the driver without expanding variables. Expressions like
${output.homeScreen.buttons.profile} were sent as literal strings,
causing conditions to always evaluate as false and silently skip
conditional blocks.
Three changes:
- CheckCondition(): expand variables in visible/notVisible selectors
and platform field before evaluating against the driver
- ExpandStep(): add RunFlowStep case to expand variables in File,
When condition fields, and Env values
- executeNestedStep(): call ExpandStep before executeRunFlow for
nested RunFlowStep execution
- TestExpandStep_RunFlowStep: verifies ExpandStep expands variables in File, When.Visible, When.NotVisible, When.Script, When.Platform, and Env map values - TestExpandStep_RunFlowStep_NilWhen: verifies nil When doesn't panic - TestCheckCondition_ExpandsVisibleSelectorVariables: verifies expanded selector ID reaches the driver - TestCheckCondition_ExpandsNotVisibleSelectorVariables: same for NotVisible text selector - TestCheckCondition_ExpandsPlatformVariable: verifies platform string expansion before comparison
Address PR review feedback: CheckCondition() should only evaluate conditions, not expand variables. ExpandStep() already handles all variable expansion before CheckCondition() is called, so the expansion in CheckCondition() was a redundant second pass. - Revert CheckCondition() to use condition fields directly - Remove CheckCondition expansion tests and mockConditionDriver - Keep ExpandStep RunFlowStep case (the actual fix)
…pest - Change below/above/leftOf/rightOf selection from DeepestMatchingElement to candidates[0] (closest by distance, clickable-preferred) - Keep deepest element behavior for non-directional filters (childOf, etc.) - Add regression tests for all three drivers (WDA, Appium, UIAutomator2)
f271944 to
d40f3ac
Compare
Contributor
Author
|
@omnarayan I've rebased this onto the latest main and fixed the changelog conflict. Should be good to merge now! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Directional relative selectors (
below:,above:,leftOf:,rightOf:) incorrectly pick the deepest DOM element instead of the closest by distance. This causes taps to land on the wrong element when a deeply-nested node exists further from the anchor than a shallower, closer match.Type of Change
How We Found This Bug
Our app uses an Expo auth WebView (via
expo-auth-session) that doesn't exposetestIDattributes on its elements. To automate the login flow, we rely on relative position selectors to locate input fields — e.g.,tapOn: below: "Email Address"to tap the email input.This worked correctly with standard Maestro, but with maestro-runner, the selector consistently tapped a deeply-nested link far below the anchor instead of the input field directly beneath it.
Real-World Impact
Any screen where elements lack
testIDs (WebViews, third-party auth pages, embedded browsers) and relative selectors are used will tap the wrong element when a deeper DOM node exists further from the anchor. This makes auth flows and similar WebView interactions unreliable.Root Cause Analysis
Maestro vs maestro-runner Comparison
.firstOrNull()— picks closestDeepestMatchingElement()— picks deepest DOM depthThe pipeline in maestro-runner:
FilterBelow()→ returns candidates sorted by distance (closest first) ✅SortClickableFirst()→ moves clickable elements first, preserving order within groups ✅DeepestMatchingElement()→ discards distance ordering, picks highest DOM depth ❌Concrete Example
DeepestMatchingElementpicked the Link (depth 5) over the TextField (depth 2), ignoring that the TextField was 22x closer.Changes Made
Fix (4 locations across 3 drivers)
When a directional filter (
below/above/leftOf/rightOf) was applied, usecandidates[0](closest element, clickable-preferred) instead ofDeepestMatchingElement(). Non-directional filters (childOf/containsChild/insideOf) continue using depth-based selection since they don't sort by distance.resolveRelativeSelectorpkg/driver/wda/driver.gofindElementRelativeWithElementspkg/driver/appium/driver.goresolveRelativeSelectorpkg/driver/uiautomator2/driver.gofindElementRelativeWithElementspkg/driver/uiautomator2/driver.goTests (3 new test cases)
Each driver gets a regression test that creates a page source with:
Verifies the closest element is selected, not the deepest.
Testing
go test ./pkg/driver/...)tapOn: below: "Email Address"now taps the input field, not the linkChecklist
Additional Notes
This fix is intentionally scoped to directional filters only. Containment-based filters (
childOf,containsChild,insideOf) don't sort by distance, soDeepestMatchingElementremains the correct selection strategy for those.