Closed Bug 911477 Opened 11 years ago Closed 8 years ago

Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith()

Categories

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

26 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: crimsteam, Assigned: yrliou, Mentored)

References

Details

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

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

When we can expect impelmentacji new methods from DOM4:

- ParentNode.prepend()
- ParentNode.append()
- ChildNode.before()
- ChildNode.after()
- ChildNode.replace()

At this moment we have only ChildNode.remove().


Actual results:

They're not implemented.


Expected results:

They should be implemented.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Version: 23 Branch → 26 Branch
Last I had checked, the spec for preped/append/before/after was still in flux in terms of what the arguments are and what they mean.

Has it stabilized since then?  I'd rather not spend too much time dealing with this if it's just going to change some more....
Flags: needinfo?(annevk)
Add this momen it's look like stable:

void prepend((Node or DOMString)... nodes);

All algorithms for this method are complete. So I suppose we cane make sth like this:

node.prepend(refNode1, refNode2, refNode3);
or
node.prepend("textNode1", "textNode2", "textNode3");

But for clear I will ask someone editors specification for comment on this case.
I already asked the spec editor for a response; see the needinfo request.
I'm not aware of them being in flux. We might need the new @@unscopable JavaScript feature though to make sure they're not causing trouble in event handlers.
Flags: needinfo?(annevk)
Luke, what would it take to expose something like limited forms of With objects here or some other equivalent that would give us @@unscopable?
Flags: needinfo?(luke)
Let me see if I understand what you need: to be able to create an array of blacklisted names and attach this to the prototype of an object with a magic name (@@unscopeable) such that, if the object is used as a with object, the blacklisted names will be ignored by name lookup within the with objects scope.

Is that right?  If so, are you thinking some JSAPI JS_SetUnscopeableProperty(obj, v) which defines obj[@@unscopeable] = v?
Flags: needinfo?(luke)
Luke, see @@unscopables in http://people.mozilla.org/~jorendorff/es6-draft.html to find what we need. (Although we do indeed need it for a different object than Array.prototype.)
I don't have a preconception of what I need.  The real hard requirement for the DOM case is being able to construct a scopechain that blacklists exposing certain properties.
Ah, so is this the scopechain of event handlers we are talking about?  If 'yes', then would the first step be to convert event handlers to do the selfhost+with strategy we discussed earlier and then, when the JS engine implements @@unscopeables, using this to do the necessary blacklisting?

Another question: since setting @@unscopeables on the DOM objects in the scope chain would be generally observable to content, does the DOM spec mandate this setting of @@unscopeables?
> Ah, so is this the scopechain of event handlers we are talking about?

Yes, indeed.

> would the first step be to convert event handlers to do the selfhost+with strategy

That seems like it might be simplest, yes...

> does the DOM spec mandate this setting of @@unscopeables?

I think it would end up doing so.
(In reply to Boris Zbarsky [:bz] from comment #10)
> > does the DOM spec mandate this setting of @@unscopeables?
> 
> I think it would end up doing so.

That would make sense since the other browsers self-host event handlers using with (right?).

Ok, well, I'm not too sure about when @@unscopeables will be added, but it doesn't seem difficult (we put a proxy-like thing on the scope chain for a with object anyway, so easy enough to hook in at just the right place w/o harming perf).  Mostly the question is the 'right' way to name this @@unscopeables property.  Do you know Waldo?
Flags: needinfo?(jwalden+bmo)
Chrome kinda-self-hosts using something kinda-like-with.  I don't know about other implementations.
The "right" way is, um.  @@unscopeable is a symbol.  Those are totally under-specified and under-defined and un-certain right now.  If we knew how to represent @@unscopeable the "right" way, we'd have used it for @@iterator in bug 907077.  Instead, we're using a magical string value, which I'm hoping we can keep changing frequently enough from release to release that nobody will prematurely start using it.  (Or at least make the string obnoxious enough and clear that nobody will use it.)

We could do the same here if we really really really wanted this by yesterday.  I'm not convinced the need for this is so pressing that we should run ahead of TC39 here with some hackaround alternative, even if we managed to discourage web content from using it before its complete standardization.  So while these methods themselves might be stable, I think their relying on the very-unstable @@unscopeable thing means we shouldn't consider them a stable thing yet.
Flags: needinfo?(jwalden+bmo)
I think what Jeff says is fair. I think TC39 wants to have this sorted within the next couple of months so we'll just have to delay a little unless we want to resort to "hacks".
What's the status for this in 2015?
I think other work (like classes) is higher priority for the JS team right now than @@unscopables.  It's not clear what timeframe that's on, or whether it's even on the radar.

In any case, clearly nothing is going to happen here until that's fixed.
Depends on: 1104955
There's really not much point commenting here.  This bug is blocked on bug 1054759 getting fixed in SpiderMonkey...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OK, once bug 1104955 lands (I expect a few days), this can be worked on.  Happy to mentor.

As far as comment 17 goes, Blink backed it out and has not relanded.

As far as comment 18 goes, WebKit nightlies support this, but shipping Safari does not.  And WebKit nightlies don't have it unscopable, for what it's worth.
Mentor: bzbarsky
Whiteboard: [tw-dom]
Summary: Implement DOM4 methods: prepend(), append(), before(), after() and replace() → Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith()
I would like to try on this bug.
Assignee: nobody → joliu
Great!  I looked over the spec, and it seems fairly reasonable.  Do let me know if you run into any issues.
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Hi Boris,

I have a patch implementing these methods, and can pass web-platform-tests, I will submit a review request to you later.

Besides the implementation, I didn't quite get the idea that why it would be problematic without [unscopeable].
I saw there were some discussion in previous comments talking about the scopechain of event handlers.
Could you help to explain the problem with some concrete examples?

Thanks,
Jocelyn
Flags: needinfo?(bzbarsky)
Sure.  Consider this web page:

  <script>
    function append(itemId) {
      /* do something with itemId */
      console.log("Append ", itemId);
    }
  </script>
  <input type="button" value="Append a thing" onclick="append(1)">

Without Element.append supported, clicking the button calls the "append" function defined in the <script> tag.

With Element.append supported, if it's not [Unscopable] then clicking the button would call the Element.append method, with the button as `this`.  Hence the need for [Unscopable] to prevent compat issues arising as a result.

Note that we had precisely this problem with ChildNode.remove; see bug 938799.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #24)
> Sure.  Consider this web page:
> 
>   <script>
>     function append(itemId) {
>       /* do something with itemId */
>       console.log("Append ", itemId);
>     }
>   </script>
>   <input type="button" value="Append a thing" onclick="append(1)">
> 
> Without Element.append supported, clicking the button calls the "append"
> function defined in the <script> tag.
> 
> With Element.append supported, if it's not [Unscopable] then clicking the
> button would call the Element.append method, with the button as `this`. 
> Hence the need for [Unscopable] to prevent compat issues arising as a result.
> 
> Note that we had precisely this problem with ChildNode.remove; see bug
> 938799.

Thanks a lot, it's really helpful! :)
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

https://reviewboard.mozilla.org/r/46637/#review43403

r=me, with the ErrorResult checking improved and either the hashset thing I describe below implemented or a followup bug filed for it.

::: dom/base/nsINode.h:1798
(Diff revision 1)
>      mozilla::ErrorResult rv;
>      parent->RemoveChild(*this, rv);
>      return rv.StealNSResult();
>    }
>  
> +private:

Ah, this needs to be a member method because you use the owner document?

This needs a spec issue: the spec doesn't say how the Text nodes in question (and the DocumentFragment, but that's less important) should get created.  Filed https://github.com/whatwg/dom/issues/224

Also, this could probably live down with the other private stuff.  Not sure I have a strong opinion on that, but it's not part of the API, while everything else around it here is.

::: dom/base/nsINode.h:1814
(Diff revision 1)
> +
>    // ChildNode methods
>    mozilla::dom::Element* GetPreviousElementSibling() const;
>    mozilla::dom::Element* GetNextElementSibling() const;
> +
> +  void Before(const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,

Could add a typedef for mozilla::dom::Sequence too.  Either way.

::: dom/base/nsINode.cpp:1661
(Diff revision 1)
>    return nullptr;
>  }
>  
> +already_AddRefed<nsINode>
> +nsINode::ConvertNodesOrStringsIntoNode(
> +  const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,

There's a "using namespace mozilla::dom" at the top of this file, so there is no need to have "mozilla::dom" on Sequence, or anything else.  Just commenting it here, but applies various places in the patch.

::: dom/base/nsINode.cpp:1664
(Diff revision 1)
> +already_AddRefed<nsINode>
> +nsINode::ConvertNodesOrStringsIntoNode(
> +  const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> +  ErrorResult& aRv)
> +{
> +  nsTArray<nsCOMPtr<nsINode>> nodes;

I think it would be better to avoid the extra array allocations here by factoring out the "is this a node or string" bit into a static helper returning `already_AddRefed<nsINode>` and then checking aNodes.Length() up front.  If it's 1, return the helper applied to the one element.  Otherwise, iterate aNodes, convert each one into a single stack nsCOMPtr, and immediately add it to the document fragment.

::: dom/base/nsINode.cpp:1682
(Diff revision 1)
> +  if (nodes.Length() == 1) {
> +    return nodes[0].forget();
> +  } else {
> +    nsCOMPtr<nsINode> fragment = OwnerDoc()->CreateDocumentFragment();
> +    for (const auto& node : nodes) {
> +      fragment->AppendChild(*node, aRv);

If aRv.Failed() after this call, you should just immediately return nullptr.  Do NOT keep doing more calls that might scribble on aRv.

::: dom/base/nsINode.cpp:1713
(Diff revision 1)
> +  }
> +
> +  nsINode* viablePreviousSibling = nullptr;
> +  for (nsINode* sibling = GetPreviousSibling(); sibling;
> +       sibling = sibling->GetPreviousSibling()) {
> +    if (!IsNodeInNodes(sibling, aNodes)) {

This could have pretty pathological behavior if we have a long sibling list and all our previous siblings are in aNodes.

It might be worth thinking about constructing a temporary hashset here if aNodes is large enough.. or even in general.  It would be a slight slowdown for small aNodes, probably, but may be worth it to avoid the pathology.  Especially if you pre-size it to some sane size like 8 or 16 or whatnot to handle common small-number-of-arguments cases quickly.

::: dom/base/nsINode.cpp:1719
(Diff revision 1)
> +      viablePreviousSibling = sibling;
> +      break;
> +    }
> +  }
> +
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);

Right after this call, you need to return if aRv.Failed(), instead of doing more stuff.

::: dom/base/nsINode.cpp:1735
(Diff revision 1)
> +  nsCOMPtr<nsINode> parent = GetParentNode();
> +  if (!parent) {
> +    return;
> +  }
> +
> +  nsINode* viableNextSibling = nullptr;

This code is duplicated in After and ReplaceWith.  Might be worth factoring it out into a helper method.  And, again, using a hashset.

::: dom/base/nsINode.cpp:1744
(Diff revision 1)
> +      viableNextSibling = sibling;
> +      break;
> +    }
> +  }
> +
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);

Need to return immediately if aRv.Failed().

::: dom/base/nsINode.cpp:1766
(Diff revision 1)
> +      viableNextSibling = sibling;
> +      break;
> +    }
> +  }
> +
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);

Need to return immediately if aRv.Failed().

::: dom/base/nsINode.cpp:1821
(Diff revision 1)
>  
>  void
> +nsINode::Prepend(const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> +                 ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);

Need to return immediately if aRv.Failed().

::: dom/base/nsINode.cpp:1829
(Diff revision 1)
> +
> +void
> +nsINode::Append(const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> +                 ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);

Need to return if aRv.Failed().

::: dom/base/nsINode.cpp:1830
(Diff revision 1)
> +void
> +nsINode::Append(const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> +                 ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);
> +  this->AppendChild(*node, aRv);

Why is the `this->` part needed?  I expect it's not...
Attachment #8741616 - Flags: review?(bzbarsky) → review+
Oh, and please send an intent to ship for this!
https://reviewboard.mozilla.org/r/46637/#review43403

> Ah, this needs to be a member method because you use the owner document?
> 
> This needs a spec issue: the spec doesn't say how the Text nodes in question (and the DocumentFragment, but that's less important) should get created.  Filed https://github.com/whatwg/dom/issues/224
> 
> Also, this could probably live down with the other private stuff.  Not sure I have a strong opinion on that, but it's not part of the API, while everything else around it here is.

Yes, it was the reason.
Thanks for firing the spec issue, I didn't notice that bit.

With the updated algo in the spec, I will pass the node document as an argument and remove it from member functions.

> Could add a typedef for mozilla::dom::Sequence too.  Either way.

Should we use type alias here since it's a template class?

> There's a "using namespace mozilla::dom" at the top of this file, so there is no need to have "mozilla::dom" on Sequence, or anything else.  Just commenting it here, but applies various places in the patch.

Didn't notice that, thanks for pointing it out!

> Why is the `this->` part needed?  I expect it's not...

Yes, that's redudant, sorry about that.
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46637/diff/1-2/
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

Hi Boris,

Would you mind take a look again since there are many new static helpers due to refactoring?
Just want to make sure that I didn't misunderstood your review comments.

I will send an intent to ship after r+ and passing tests on try server.

Thanks,
Jocelyn
Attachment #8741616 - Flags: review+ → review?(bzbarsky)
> Should we use type alias here since it's a template class?

Ah, yes, it would have to be that.  Again, either way is fine.
Forgot to remove related entries in interfaces.html.ini, will remove them in the next revision.
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

https://reviewboard.mozilla.org/r/46637/#review43703

::: dom/base/nsINode.cpp:1661
(Diff revisions 1 - 2)
>    return nullptr;
>  }
>  
> -already_AddRefed<nsINode>
> -nsINode::ConvertNodesOrStringsIntoNode(
> -  const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> +static already_AddRefed<nsINode>
> +GetNodeFromNodeOrString(const OwningNodeOrString& aNode,
> +                        const nsIDocument* aDocument)

Just nsIDocument*, please.  There's nothing terribly const about it.

::: dom/base/nsINode.cpp:1666
(Diff revisions 1 - 2)
> -  const mozilla::dom::Sequence<OwningNodeOrString>& aNodes,
> -  ErrorResult& aRv)
> -{
> -  nsTArray<nsCOMPtr<nsINode>> nodes;
> -  for (const auto& node : aNodes) {
> -    if (node.IsNode()) {
> +                        const nsIDocument* aDocument)
> +{
> +  if (aNode.IsNode()) {
> +    nsCOMPtr<nsINode> node = aNode.GetAsNode();
> +    return node.forget();
> +  } else if (aNode.IsString()){

No else after return, please.  So:

  if (aNode.IsNode()) {
    // stuff
    return node.forget();
  }
  
  if (aNode.IsString()) {
    // whatever
  }

::: dom/base/nsINode.cpp:1682
(Diff revisions 1 - 2)
> + * https://dom.spec.whatwg.org/#converting-nodes-into-a-node for |prepend()|,
> + * |append()|, |before()|, |after()|, and |replaceWith()| APIs.
> + */
> +static already_AddRefed<nsINode>
> +ConvertNodesOrStringsIntoNode(const Sequence<OwningNodeOrString>& aNodes,
> +                              const nsIDocument* aDocument,

No const here, please.

::: dom/base/nsINode.cpp:1687
(Diff revisions 1 - 2)
> +                              const nsIDocument* aDocument,
> +                              ErrorResult& aRv)
> +{
> +  if (aNodes.Length() == 1) {
> +    return GetNodeFromNodeOrString(aNodes[0], aDocument);
>    } else {

Again, no else after return.

::: dom/base/nsINode.cpp:1808
(Diff revisions 1 - 2)
> -  nsCOMPtr<nsINode> node = ConvertNodesOrStringsIntoNode(aNodes, aRv);
> -  if (parent == GetParentNode()) {
> +  nsCOMPtr<nsINode> node =
> +    ConvertNodesOrStringsIntoNode(aNodes, OwnerDoc(), aRv);
> +
> +  if (aRv.Failed()) {
> +    return;
> +  } else if (parent == GetParentNode()) {

No else after return.

r=me; this looks fantastic.
Attachment #8741616 - Flags: review?(bzbarsky) → review+
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46637/diff/2-3/
Comment on attachment 8741616 [details]
MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(), before(), after() and replaceWith(). r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46637/diff/3-4/
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #36)
> Comment on attachment 8741616 [details]
> MozReview Request: Bug 911477 - Implement DOM4 methods: prepend(), append(),
> before(), after() and replaceWith(). r?bz
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/46637/diff/3-4/

* rebase and update some ini files which I missed in the first place.
* submit a full try again at https://treeherder.mozilla.org/#/jobs?repo=try&revision=780cb3d53d87

I will ship it into nightly channel (FF 49) if try looks good.
https://hg.mozilla.org/mozilla-central/rev/def3cd7babe6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Depends on: CVE-2016-9069
Depends on: CVE-2016-9067
You need to log in before you can comment on or make changes to this bug.