-
-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update xpath selector selection logic yet again #1135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the edge case in my other comment, looks good!
app/renderer/util.js
Outdated
// even if this node name is unique, if it's the root node, we don't want to refer to it using | ||
// '//' but rather '/' | ||
if (!domNode?.parentNode?.tagName) { | ||
xpath = `/${domNode.tagName}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how useful this condition is. From the apps I checked, Android always has two android.widget.FrameLayout
as the roots, so isUnique
will be false anyway; and iOS has XCUIElementTypeApplication
with filled out name
and label
fields, so the xpath generator will rely on those, and not even reach this base case. So currently I haven't found a usecase for this.
Also, CI is failing here because we can't use optional chaining (Parcel v1 limitation). It probably needs to be something like if (!(domNode.parentNode && domNode.parentNode.tagName))
.
Finally, if we really must keep the condition, it should probably be moved inside the isUnique
block on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really matters for our tests, so I don't have to change a million lines of test code, and I think it might matter for some other hierarchies that come from other platforms. But I'll fix the optional chaining issue.
106b61e
to
eb01e75
Compare
this PR primarily removes class/type from xpath uniqueness attributes. instead adds logic to look for unique node names and refer to those if necessary (since class/type are always the same as node names)