w3c / webdriver

Remote control interface that enables introspection and control of user agents.
https://w3c.github.io/webdriver/
Other
676 stars 190 forks source link

Should HTMLDocument nodes be serialized as web element reference? #1687

Closed whimboo closed 1 year ago

whimboo commented 1 year ago

Right now the WebDriver specification creates a web element reference in JSON clone when the value is of instance Element, which basically maps to Element nodes in the HTML spec.

But when you check the Node interface you can see that Element nodes have a type of 1 and Document nodes a type of 9. That means that HTMLDocument nodes are not Elements for WebDriver and as such shouldn't be serialized as such.

But recently when investigating a test failure in Firefox I noticed that Chrome is actually passing this test and returns the HTMLDocument as web element reference. This is against the WebDriver specification.

The question is now if Chrome should fix that broken behavior (not sure if this could cause regressions for existing tests) or if we should allow the serialization of HTMLDocument. Maybe the WebDriver BiDi specification could benefit from that as well?

CC'ing @sadym-chromium and @jgraham for now, and I'll put it onto our monthly WebDriver agenda for this week.

whimboo commented 1 year ago

Note that Document nodes do not have the full feature set as Element nodes have, eg. Get Element Attribute and others will / might not work.

css-meeting-bot commented 1 year ago

The Browser Testing and Tools Working Group just discussed Should HTMLDocument nodes be serialized as web element reference.

The full IRC log of that discussion <jgraham> Topic: Should HTMLDocument nodes be serialized as web element reference
<jgraham> github: https://github.com/w3c/webdriver/issues/1687
<foolip> q+
<jgraham> whimboo: I started to work on serializing web elements and looking at the existing tests, I found one that gets the Document Node and Chrome will return an WebElement. This doesn't match the spec. I changed Firefox so it fails (as per spec, because the Document node is cyclic). Do we have data about whether this in use? Several commands in WebDriver don't work e.g. getAttribute. Would be clearer to say
<jgraham> we don't support this.
<jgraham> ack foolip
<jgraham> foolip: Is this classic or bidi?
<jgraham> whimboo: Classic?
<sadym> q+
<jgraham> foolip: Then I don't know. Classic has Element and Window handles?
<jgraham> whimboo: Element, Window, Frame and ShadowRoot
<jgraham> foolip: So it's a spec violation? [yes]
<jgraham> foolip: Is there a use case?
<jgraham> whimboo: We just discovered this when fixing another bug
<jgraham> foolip: File a chromedriver bug? Seems weird to support window and not document
<jgraham> simon: Window is used to switch to window
<sadym> q-
<jgraham> simon: document.documentElent would be the best thing to use. Classic doesn't do anything sophisticated here
whimboo commented 1 year ago

@sadym-chromium your feedback would be welcome. Thanks.

sadym-chromium commented 1 year ago

It looks like the spec tells us throwing "circular reference" exception when document is serialized, because of:

Removing the field id will be a breaking change, while adding new document-specific fields to serialization seems fine.

sadym-chromium commented 1 year ago

@whimboo sorry for the late reply, it required some research.

whimboo commented 1 year ago

Thank you @sadym-chromium. I'm going to land my wdspec test change on mozilla-central, which will be synced soon via https://github.com/web-platform-tests/wpt/pull/36619 so that we expect a failure and are compliant with the WebDriver specification.

If it turns out that consumers see regressions we might have to further discuss how to handle documents in detail. Until then I'll leave this issue open and will come to it once Firefox 108 is released and / or we got user feedback.

gsnedders commented 1 year ago

see also https://github.com/web-platform-tests/wpt/issues/30157 where I seem to have said that ad-hoc testing showed non-element node types were far from interoperable; I was sure there was some issue in this repo but I can't find it

whimboo commented 1 year ago

So far I haven't seen any negative feedback about this particular change. As such I would say that we close this issue as done.