Last Comment Bug 811259 - implement Element.insertAdjacentText and Element.insertAdjacentElement
: implement Element.insertAdjacentText and Element.insertAdjacentElement
Status: RESOLVED FIXED
[parity-ie] [parity-chrome] [parity-o...
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: mozilla48
Assigned To: Jocelyn Liu [:jocelyn] [:joliu]
:
:
Mentors:
: 1256338 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-13 03:21 PST by Florian Bender
Modified: 2016-05-09 10:44 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r?smaug (58 bytes, text/x-review-board-request)
2016-03-28 05:29 PDT, Jocelyn Liu [:jocelyn] [:joliu]
bugs: review+
Details | Review
MozReview Request: Bug 811259 - Patch2: Add web-platform tests for insertAdjacentText and insertAdjacentElement. r=smaug (58 bytes, text/x-review-board-request)
2016-03-28 05:29 PDT, Jocelyn Liu [:jocelyn] [:joliu]
bugs: review+
Details | Review
MozReview Request: Bug 811259 - Patch1: Implement Element.insertAdjacentText and Element.insertAdjacentElement. r=smaug (58 bytes, text/x-review-board-request)
2016-03-31 06:11 PDT, Jocelyn Liu [:jocelyn] [:joliu]
bugs: review+
Details | Review

Description Florian Bender 2012-11-13 03:21:04 PST
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
Comment 1 Florian Bender 2012-11-13 08:11:11 PST
See discussion in Bug 199191 Comment 11 and later.
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-11-13 08:28:18 PST
Henri, perhaps you have some comments about this.
(IIRC you implemented insertAdjacentHTML.)

But the new methods in DOM Core are probably more useful.
Comment 3 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-11-14 00:54:01 PST
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.”
Comment 4 Josh Matthews [:jdm] 2014-02-03 05:21:07 PST
Blink is considering removing this: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9oeB2yR4thg
Comment 5 Florian Bender 2014-02-03 09:17:46 PST
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 Adam Barth 2014-02-03 10:45:06 PST
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.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2014-02-03 11:24:10 PST
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.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2016-03-14 08:36:15 PDT
Now in the spec, sadly: https://github.com/whatwg/dom/commit/8fa7ac749d1b612504184aad2c20808a1785b370
Comment 9 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-15 01:06:38 PDT
I would like to try on this bug.
Comment 10 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-15 07:23:02 PDT
You might want to look at Bug 1256338 too. Should be very similar.
I think wpt doesn't have tests yet for either one.
Comment 11 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-28 05:20:25 PDT
*** Bug 1256338 has been marked as a duplicate of this bug. ***
Comment 12 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-28 05:29:41 PDT
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/
Comment 13 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-28 05:29:41 PDT
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 14 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-28 06:30:16 PDT
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.
Comment 15 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-28 06:45:22 PDT
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());
Comment 16 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 04:59:18 PDT
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/
Comment 17 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 04:59:18 PDT
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 18 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-30 09:34:08 PDT
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
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-30 09:49:00 PDT
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.
Comment 20 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 13:21:29 PDT
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/
Comment 21 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 13:21:29 PDT
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/
Comment 22 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 13:39:05 PDT
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/
Comment 23 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 13:39:05 PDT
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/
Comment 24 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-30 13:49:30 PDT
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.
Comment 25 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-31 06:04:24 PDT
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/
Comment 26 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-31 06:11:10 PDT
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/
Comment 27 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-31 06:11:10 PDT
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/
Comment 28 Jocelyn Liu [:jocelyn] [:joliu] 2016-03-31 06:15:19 PDT
(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 29 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-31 06:27:42 PDT
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
Comment 30 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2016-03-31 06:30:28 PDT
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
Comment 31 Jocelyn Liu [:jocelyn] [:joliu] 2016-04-06 19:44:54 PDT
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/
Comment 32 Jocelyn Liu [:jocelyn] [:joliu] 2016-04-06 19:44:54 PDT
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/
Comment 33 Jocelyn Liu [:jocelyn] [:joliu] 2016-04-06 19:50:36 PDT
* rebase and submit another try build since it has been a week.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbfcf81faf1
Comment 36 Chris Mills (Mozilla, MDN editor) [:cmills] 2016-05-09 10:44:03 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.