Closed Bug 811259 Opened 8 years ago Closed 4 years ago

implement Element.insertAdjacentText and Element.insertAdjacentElement

Categories

(Core :: DOM: Core & HTML, defect)

17 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: fb+mozdev, Assigned: yrliou)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-ie] [parity-chrome] [parity-opera][tw-dom][btpp-active])

Attachments

(2 files, 1 obsolete file)

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
See discussion in Bug 199191 Comment 11 and later.
See Also: → 199191
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
Whiteboard: [parity-ie] [parity-chrome] [parity-opera]
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).
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.
Whiteboard: [parity-ie] [parity-chrome] [parity-opera] → [parity-ie] [parity-chrome] [parity-opera][tw-dom]
See Also: → 1256338
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.
Duplicate of this bug: 1256338
Summary: implement insertAdjacentText → implement Element.insertAdjacentText and Element.insertAdjacentElement
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)
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)
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)
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+
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 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 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 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/
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 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/
Attachment #8735429 - Attachment is obsolete: true
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)
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/
(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]
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 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/
* rebase and submit another try build since it has been a week.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbfcf81faf1
https://hg.mozilla.org/mozilla-central/rev/85d6982a515a
https://hg.mozilla.org/mozilla-central/rev/3d44a2dab529
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.