Replace element.isDisconnected with Node#isConnected

RESOLVED FIXED in Firefox 57

Status

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks: 1 bug)

Version 3
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Depends on: 1393812
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8902678 - Flags: review?(dburns)
(Assignee)

Comment 2

a year ago
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).
Blocks: 1311041
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1392854
Comment hidden (mozreview-request)
Blocks: 1393173
(Assignee)

Updated

a year ago
Priority: -- → P1

Comment 5

a year ago
mozreview-review
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)
Attachment #8902678 - Flags: review?(dburns) → review+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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

a year ago
mozreview-review-reply
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

a year ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d73e79c0d9d4
Use Node.isConnected for web element staleness check. r=automatedtester
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.
https://hg.mozilla.org/mozilla-central/rev/d73e79c0d9d4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1396231
Depends on: 1399018
Depends on: 1405018
You need to log in before you can comment on or make changes to this bug.