Use .remove() instead of .parentNode.removeChild

RESOLVED FIXED in Firefox 54

Status

()

Firefox
General
RESOLVED FIXED
7 months ago
6 months 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)

(Assignee)

Description

7 months ago
Created attachment 8831467 [details]
xpcshell script

ChildNode.remove() was implemented in bug 856629 for Firefox 23.
(Assignee)

Comment 1

7 months ago
Created attachment 8831468 [details] [diff] [review]
Test adapted by hand

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)

Updated

7 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 months ago
Created attachment 8831469 [details] [diff] [review]
eslint rule
Attachment #8831469 - Flags: review?(jaws)
(Assignee)

Comment 3

7 months ago
Created attachment 8831470 [details] [diff] [review]
script-generated patch

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)
(Assignee)

Comment 4

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e7d6702d143
Attachment #8831468 - Flags: review?(jaws) → review+
Attachment #8831469 - Flags: review?(jaws) → review+
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+
(Assignee)

Comment 6

7 months ago
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.

Comment 7

7 months ago
bugherder
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
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Updated

7 months ago
Depends on: 1335073
(Assignee)

Updated

7 months ago
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)
(Assignee)

Comment 9

6 months ago
(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)
(Assignee)

Updated

6 months ago
Depends on: 1345253
You need to log in before you can comment on or make changes to this bug.