1394881 - Replace element.isDisconnected with Node#isConnected

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

It turns out that Node#isConnected (described in
https://dom.spec.whatwg.org/#dom-node-isconnected) handles an
element’s shadow root, which is exactly the reason why we wrote
element.isDisconnected in the first place.

element.isDisconnected is long and error-prone and can be removed
entirely in favour of this web platform API.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1394881 - Use Node.isConnected for web element staleness check.

It turns out that Node.isConnected (described in
https://dom.spec.whatwg.org/#dom-node-isconnected) handles an element’s
shadow root, which element.isDisconnected tries to replicate.

element.isDisconnected and element.isStale are both long and error-prone
and can be removed entirely in favour of this web platform API.

The relevant change to the WebDriver specification landed in
https://github.com/w3c/webdriver/commit/32a477b023d155d0ee348cd87bcbc3e2b65eda1b.

Review commit: https://reviewboard.mozilla.org/r/174352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174352/

Comment 2

[Andreas Tolfsen ❲:ato❳]

One of the headaches of improving window tracking in Marionette has
been how to deal element.Store#get taking a container object with
references to WindowProxy and the shadowRoot element.  By using
isConnected on the element reference itself, we can do away with
passing these into the known element store (or store references to
them internally in the store).

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8902678 [details]
Bug 1394881 - Use Node.isConnected for web element staleness check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174352/diff/1-2/

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8902678 [details]
Bug 1394881 - Use Node.isConnected for web element staleness check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174352/diff/2-3/

Comment 5

[David Burns :automatedtester]

Comment on attachment 8902678 [details]
Bug 1394881 - Use Node.isConnected for web element staleness check.

https://reviewboard.mozilla.org/r/174352/#review179710

::: testing/marionette/element.js:171
(Diff revision 3)
>     *     the DOM provided in the container.
>     */
> -  get(uuid, container) {
> +  get(uuid) {
>      let el = this.els[uuid];
>      if (!el) {
> -      throw new NoSuchElementError(`Element reference not seen before: ` +
> +      throw new NoSuchElementError("Element reference not seen before: " + uuid);

string templates would be neater here and matches the other errors (even though they have pprint)

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8902678 [details]
Bug 1394881 - Use Node.isConnected for web element staleness check.

https://reviewboard.mozilla.org/r/174352/#review179710

> string templates would be neater here and matches the other errors (even though they have pprint)

I will address this in a follow-up as it’s quite crucial for my progress on the window tracking bug to get this landed on central.  Assuming this is OK with you.

Comment 7

[David Burns :automatedtester]

Comment on attachment 8902678 [details]
Bug 1394881 - Use Node.isConnected for web element staleness check.

https://reviewboard.mozilla.org/r/174352/#review179710

> I will address this in a follow-up as it’s quite crucial for my progress on the window tracking bug to get this landed on central.  Assuming this is OK with you.

Thats fine with me

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d73e79c0d9d4
Use Node.isConnected for web element staleness check. r=automatedtester

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

This patch doesn't fix the issues with test_submit_unencrypted_info_warning.py, because the try build still doesn't contain any fx-ui tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9cd8b24b175&selectedJob=127101165

I filed bug 1395469 for the new signature.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d73e79c0d9d4