Use .remove() instead of .parentNode.removeChild

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on 1 bug)

53 Branch
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(4 attachments)

Posted file xpcshell script
ChildNode.remove() was implemented in bug 856629 for Firefox 23.
removeChild returns the removed node, whereas remove() doesn't return anything. We had only one place in the tree depending on this return value, and it's in a test that can easily be simplified.
Attachment #8831468 - Flags: review?(jaws)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Posted patch eslint ruleSplinter Review
Attachment #8831469 - Flags: review?(jaws)
I had to leave some files unmodified to avoid test failures.

These use the incomplete DOM API implemented in JSDOMParser.js:
- toolkit/components/reader/Readability.js
- toolkit/components/reader/JSDOMParser.js

remove() doesn't seem to work on <option> elements:
- dom/html/test/test_bug394700.html

I intend to file bugs on these 2 issues in the relevant components, and attach in these bugs the patches doing the replacements in these files.
Attachment #8831470 - Flags: review?(jaws)
Comment on attachment 8831470 [details] [diff] [review]
script-generated patch

Review of attachment 8831470 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8831470 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d95649edd01cee9bf502db4a7b07de0f965f49f
Bug 1334831 - Make the printpreview_bug396024_helper.xul test not depend on the removeChild return value, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe16a8a26150a0469177a50e08f090146d2afbe
Bug 1334831 - add an eslint rule to report usage of .parentNode.removeChild when .remove() could be used instead, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d172aa441447396884068a7d01561843cdf596
Bug 1334831 - script-generated patch to use .remove() instead of .parentNode.removeChild, r=jaws.
https://hg.mozilla.org/mozilla-central/rev/5d95649edd01
https://hg.mozilla.org/mozilla-central/rev/cbe16a8a2615
https://hg.mozilla.org/mozilla-central/rev/a9d172aa4414
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1335073
Depends on: 1335150
(In reply to Carsten Book [:Tomcat] from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/a9d172aa4414

Looks like this didn't get upstreamed :(
Flags: needinfo?(jonas.jenwald)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> (In reply to Carsten Book [:Tomcat] from comment #7)
> > https://hg.mozilla.org/mozilla-central/rev/a9d172aa4414
> 
> Looks like this didn't get upstreamed :(

See https://github.com/mozilla/pdf.js/issues/8008
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> (In reply to Carsten Book [:Tomcat] from comment #7)
> > https://hg.mozilla.org/mozilla-central/rev/a9d172aa4414
> 
> Looks like this didn't get upstreamed :(

Initially it was easiest to not upstream that, as discussed in https://github.com/mozilla/pdf.js/issues/8008, since there's issues with cross-browser support for ChildNode.remove().

However, given your comment, it seemed that this issue might continue to come up. Hence I decided to look at this once more, and it turned out that it wasn't actually too difficult to polyfill ChildNode.remove(), so these changes were upstreamed in PR https://github.com/mozilla/pdf.js/pull/8056 (which is included in the latest patch in bug 1338395).
Flags: needinfo?(jonas.jenwald)
Depends on: 1345253
You need to log in before you can comment on or make changes to this bug.