Florian Bender Reporter
	Description • 12 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

	Florian Bender Reporter
	Comment 1 • 12 years ago

See discussion in Bug 199191 Comment 11 and later.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 12 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.

	Henri Sivonen (:hsivonen)
	Comment 3 • 12 years ago

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.”

	Josh Matthews [:jdm]
	Comment 4 • 10 years ago

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

	Florian Bender Reporter
	Comment 5 • 10 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).

	Adam Barth
	Comment 6 • 10 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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 10 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 8 years ago

Now in the spec, sadly: https://github.com/whatwg/dom/commit/8fa7ac749d1b612504184aad2c20808a1785b370

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 9 • 8 years ago

I would like to try on this bug.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 8 years ago

You might want to look at Bug 1256338 too. Should be very similar.
I think wpt doesn't have tests yet for either one.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 12 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/42775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42775/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 13 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/42777/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42777/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 8 years ago

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

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 16 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 17 • 8 years 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/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 8 years ago

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.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 20 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 21 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 22 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 23 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 24 • 8 years 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.

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 25 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 26 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/43525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43525/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 27 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 28 • 8 years 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+.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 30 • 8 years ago

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

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 31 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 32 • 8 years 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/

	Jocelyn Liu [:jocelyn] [:joliu] Assignee
	Comment 33 • 8 years ago

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

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

	Pulsebot
	Comment 34 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 35 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/85d6982a515a
https://hg.mozilla.org/mozilla-central/rev/3d44a2dab529

	Chris Mills [:cmills]
	Comment 36 • 8 years ago

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!