-
Notifications
You must be signed in to change notification settings - Fork 193
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
Editorial: migrate from "context object" to "this" #1582
Conversation
The term "context object" is being removed from DOM: whatwg/dom#973 This also changes a reference to <a> elements to the string "a", since querySelectorAll's argument is a string. This may have unintended side effects of matching <svg:a> and "a" elements in other namespaces.
<var>start node</var>. If this throws an exception, | ||
<li><p>Let <var>elements</var> be the result of calling | ||
<a>querySelectorAll</a> with <var>start node</var> as <a>this</a> | ||
and "<code>a</code>" as the argument. If this throws an exception, |
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.
This and the following similar change could be breaking what is intended by the spec. Unfortunately I can't find any tests in WPT involving SVG, so it would be some work to test if current WebDriver implementations would filter out non-HTMLAnchorElement values as the original text might suggest (but not correctly define).
Leaving this comment to see if others have thoughts before I dig more.
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 would expect it would only consider elements that represent links; getting some foo:a
elements in a made up namespace. However svg:a
is also a link, so that seems like it perhaps ought to be considered? idk what existing implementations do here.
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 tried to find out, and it seems like ChromeDriver implements this using a large snippet of JS from Selenium. I think this is what ends up being used:
https://github.com/SeleniumHQ/selenium/blob/64447d4b03f6986337d1ca8d8b6476653570bcc1/javascript/atoms/locators/link_text.js#L76
So that boils down to root.querySelectorAll("a")
and I can't see anything that tries to filter to HTMLAnchorElement
and SVGAElement
. So I guess what I wrote here ends up matching at least chromedriver better than the original text.
I guess this should be tested though...
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.
https://searchfox.org/mozilla-central/source/testing/marionette/element.js#452 looks like it boils down to the same thing. I'm prepared to say this is stilly an unintentional, but probably not worth changing at this point.
The term "context object" is being removed from DOM:
whatwg/dom#973
This also changes a reference to elements to the string "a", since
querySelectorAll's argument is a string. This may have unintended side
effects of matching svg:a and "a" elements in other namespaces.
Preview | Diff