The default bug view has changed. See this FAQ.

implement Element.insertAdjacentText and Element.insertAdjacentElement

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: Florian Bender, Assigned: jocelyn)

Tracking

({dev-doc-complete})

17 Branch
mozilla48
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [parity-ie] [parity-chrome] [parity-opera][tw-dom][btpp-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
As insertAdjacentElement, insertAdjacentText is not yet implemented in Firefox. There is no standard definition yet, but it was implemented first by Internet Explorer [1]. Now Webkit supports both methods and Opera (probably) too. Both functions should resemble insertAdjacentHTML. 

They are quite handy not only if you are used to writing insertAdjacentHTML but also for code readability. They're mostly "DOM sugar" for what is already possible using other methods (thus insertAdjacentText/Element should be easily polyfill-able). However I think it's still worth-while to implement those methods. 

[1] http://msdn.microsoft.com/en-us/library/ms536453%28v=vs.85%29.aspx
(Reporter)

Comment 1

4 years ago
See discussion in Bug 199191 Comment 11 and later.
See Also: → bug 199191

Comment 2

4 years ago
Henri, perhaps you have some comments about this.
(IIRC you implemented insertAdjacentHTML.)

But the new methods in DOM Core are probably more useful.
Seems reasonable if Ms2ger, Anne or Aryeh specs insertAdjacentText and insertAdjacentElement in http://dom.spec.whatwg.org/

We probably shouldn’t spec or implement this peculiarity: “You cannot insert text while the document loads. Wait for the onload event to fire before attempting to call this method.”
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

4 years ago
Whiteboard: [parity-ie] [parity-chrome] [parity-opera]
Keywords: dev-doc-needed

Comment 4

3 years ago
Blink is considering removing this: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9oeB2yR4thg
(Reporter)

Comment 5

3 years ago
I'm happy with WONTFIXing if/as soon as Webkit and/or Blink remove this, and there is no plan to spec this (I still find it useful, though).

Comment 6

3 years ago
Do you all have a preference whether we should keep this API in the web platform?  I believe Firefox is the last browser that hasn't implemented the API.  Blink's measurements indicate that the API isn't widely used, so we're amenable to removing it as well.  In either case, we should match behaviors and make sure the spec reflects that consensus behavior.
Given that there's barely any use on the web, I don't see a particular reason to implement it; the API isn't nice enough to warrant the spec/test/implementation work, IMO. Also, beforeBegin/afterEnd are, I think, equivalent to before()/after(), as specced in DOM; the others aren't that much harder to do.
Now in the spec, sadly: https://github.com/whatwg/dom/commit/8fa7ac749d1b612504184aad2c20808a1785b370

Updated

a year ago
Whiteboard: [parity-ie] [parity-chrome] [parity-opera] → [parity-ie] [parity-chrome] [parity-opera][tw-dom]

Updated

a year ago
See Also: → bug 1256338
(Assignee)

Comment 9

a year ago
I would like to try on this bug.
Assignee: nobody → joliu
You might want to look at Bug 1256338 too. Should be very similar.
I think wpt doesn't have tests yet for either one.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1256338
(Assignee)

Updated

a year ago
Summary: implement insertAdjacentText → implement Element.insertAdjacentText and Element.insertAdjacentElement
(Assignee)

Comment 12

a year ago
Created attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

Review commit: https://reviewboard.mozilla.org/r/42775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42775/
Attachment #8735429 - Flags: review?(bugs)
Attachment #8735430 - Flags: review?(bugs)
(Assignee)

Comment 13

a year ago
Created attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review commit: https://reviewboard.mozilla.org/r/42777/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42777/
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

https://reviewboard.mozilla.org/r/42777/#review39205

Looking good, but I'd like to see couple of more tests.

::: testing/web-platform/tests/dom/nodes/Element-insertAdjacentText.html:23
(Diff revision 1)
> +
> +test(function() {
> +  target.insertAdjacentText("beforebegin", "test1");
> +  assert_equals(target.previousSibling.nodeValue, "test1");
> +}, "Inserted element should be target element's previous sibling for 'beforebegin' case")
> +

I think it would be good to have test also for the case when target doesn't have any sibling before calling insertAdjacent*.
So perhaps add "target2" too and make it to be the only child of some other element.

And also test how the API works with document.documentElement (I assume at least insertAdjacentElement should throw in some cases here, since document can't have more than one element as a child.)


This comment applies to both insertAdjacentText and insertAdjacentElement.
Attachment #8735430 - Flags: review?(bugs)

Updated

a year ago
Attachment #8735430 - Flags: review-
Comment on attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

https://reviewboard.mozilla.org/r/42775/#review39207

::: dom/base/Element.cpp:3654
(Diff revision 1)
> +  nsIContent* parent = GetParent();
> +

What if 'this' Element is document element (document.documentElement). Then GetParent() returns null. 
You probably want GetParentNode(); here.
And the return value needs to be kept alive using a strong pointer (otherwise some mutation event might cause the parent to be deleted too early).
So
nsCOMPtr<nsINode> parent = GetParentNode();
And I'd move that call to inside "beforebegin" and 
"afterend" right before if (!parent), since "afterbegin" and "beforeend" don't need the parent.

::: dom/base/Element.cpp:3675
(Diff revision 1)
> +  } else {
> +    aError.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +    return nullptr;
> +  }
> +
> +  return aNode;

I think I'd prefer to return null if insertion didn't succeed, in other words if aError.Failed().
That way some potential C++ would effectively get the same behavior as JS caller.
(in JS failing ErrorResult is anyhow converted to an exception)

::: dom/base/Element.cpp:3682
(Diff revision 1)
> +
> +Element*
> +Element::InsertAdjacentElement(const nsAString& aWhere,
> +                               Element& aElement,
> +                               ErrorResult& aError) {
> +  nsINode* newNode = InsertAdjacent(aWhere, &aElement, aError);

Perhaps add a MOZ_ASSERT here
MOZ_ASSERT(!newNode || newNode->IsElement());
Attachment #8735429 - Flags: review?(bugs)
(Assignee)

Comment 16

a year ago
Comment on attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42775/diff/1-2/
Attachment #8735429 - Flags: review?(bugs)
Attachment #8735430 - Flags: review- → review?(bugs)
(Assignee)

Comment 17

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/1-2/
Comment on attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

https://reviewboard.mozilla.org/r/42775/#review39839
Attachment #8735429 - Flags: review?(bugs) → review+
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

https://reviewboard.mozilla.org/r/42777/#review39845

::: testing/web-platform/tests/dom/nodes/Element-insertAdjacentElement.html:89
(Diff revision 2)
> +
> +  assert_throws("HierarchyRequestError", function() {
> +    var el = docElement.insertAdjacentElement("afterend", document.getElementById("test4"));
> +    assert_equals(el, null);
> +  });
> +}, "Adding more than one child to document.documentElement should cause a HierarchyRequestError exception")

It is adding child to document which throws, not adding child to document.documentElement. Same also in the other test.
Attachment #8735430 - Flags: review?(bugs) → review+
(Assignee)

Comment 20

a year ago
Comment on attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42775/diff/2-3/
(Assignee)

Comment 21

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/2-3/
(Assignee)

Comment 22

a year ago
Comment on attachment 8735429 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42775/diff/3-4/
(Assignee)

Comment 23

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/3-4/
(Assignee)

Comment 24

a year ago
Sorry for lots of request updated noise here.
I wasn't aware that my local commits without review items would also be pushed to try server...
Just pushed again to get rid of that.
I only did two lines of change to address Comment 19 and rebased.
(Assignee)

Comment 25

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/4-5/
(Assignee)

Updated

a year ago
Attachment #8735429 - Attachment is obsolete: true
(Assignee)

Comment 26

a year ago
Created attachment 8736690 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r=smaug

Review commit: https://reviewboard.mozilla.org/r/43525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43525/
Attachment #8735430 - Attachment description: MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r?smaug → MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug
Attachment #8736690 - Flags: review?(bugs)
(Assignee)

Comment 27

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/5-6/
(Assignee)

Comment 28

a year ago
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #26)
> Created attachment 8736690 [details]
> MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText
> and Element.insertAdjacentElement. r=smaug
> 
> Review commit: https://reviewboard.mozilla.org/r/43525/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/43525/

...apparently we couldn't update only one patch here using mozreview, otherwise patches would be discarded...
smaug, I'm really sorry for letting it re-trigger a review request for patch1, could you r+ again?
I didn't make any changes for patch1 since you r+.
Comment on attachment 8736690 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r=smaug

https://reviewboard.mozilla.org/r/43525/#review40055
Attachment #8736690 - Flags: review?(bugs) → review+
yeah, mozreview is silly. If there are no changes, just rebasing or similar, feel free to mark the patch r+ in bugzilla or something.
I think there is mozreview bug open for this.

#mozreview irc channel is good for any questions about it.
And to file mozreview bugs, https://bugzilla.mozilla.org/enter_bug.cgi?product=MozReview
Whiteboard: [parity-ie] [parity-chrome] [parity-opera][tw-dom] → [parity-ie] [parity-chrome] [parity-opera][tw-dom][btpp-active]
(Assignee)

Comment 31

a year ago
Comment on attachment 8736690 [details]
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43525/diff/1-2/
(Assignee)

Comment 32

a year ago
Comment on attachment 8735430 [details]
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42777/diff/6-7/
(Assignee)

Comment 33

a year ago
* rebase and submit another try build since it has been a week.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbfcf81faf1

Comment 34

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d6982a515a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d44a2dab529

Comment 35

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85d6982a515a
https://hg.mozilla.org/mozilla-central/rev/3d44a2dab529
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I've documented these methods:

https://developer.mozilla.org/en-US/docs/Web/API/Element
https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentElement
https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentText

And made sure it is mentioned in the relevant release notes (Jean-Yves already did it):

https://developer.mozilla.org/en-US/Firefox/Releases/48#DOM_HTML_DOM

A tech review would be nice - cheers!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.