Closed Bug 1074738 Opened 5 years ago Closed 5 years ago

GetCurrentDoc fixes in content/base

Categories

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

34 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch content_base_getcurrentdoc.diff (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=14543724ba2a

There are few unclear cases, but obviously the spec isn't quite clear either.
Attachment #8497391 - Flags: review?(wchen)
Hmm, that breaks something :/
Attached patch v2Splinter Review
One silly mistake in the previous patch, and one existing issue fixed, where
nsGenericDOMDataNode::UnbindFromTree behaves differently to Element::UnbindFromTree

https://tbpl.mozilla.org/?tree=Try&rev=75b97d83ccd9
Assignee: nobody → bugs
Attachment #8497391 - Attachment is obsolete: true
Attachment #8497391 - Flags: review?(wchen)
Attachment #8497579 - Flags: review?(wchen)
Attached patch additional patchSplinter Review
I somehow missed these.
Attachment #8497755 - Flags: review?(wchen)
Comment on attachment 8497579 [details] [diff] [review]
v2

Review of attachment 8497579 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/Element.cpp
@@ +1431,5 @@
> +  // tear down the binding. Only tear down the binding if the element
> +  // is still no longer in the DOM. nsXBLService::LoadBinding tears
> +  // down the old binding if the element is inserted back into the
> +  // DOM and loads a different binding.
> +  if (!mContent->IsInDoc()) {

It seems to me this should be IsInComposedDoc() since we also run this when the content is detatched from a shadow tree.

::: content/base/src/Link.cpp
@@ +76,5 @@
>    Element *element = self->mElement;
>  
>    // If we have not yet registered for notifications and need to,
>    // due to our href changing, register now!
> +  if (!mRegistered && mNeedsRegistration && element->IsInComposedDoc()) {

The "link" element is specified to be inert inside the shadow tree [1]. We need to also ensure that the current element is not a HTMLLinkElement, or make sure all the callers in HTMLLinkElement don't call when in a shadow tree. This goes for the other method changed in this class.

[1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements

::: content/base/src/nsDocument.cpp
@@ +6051,5 @@
>          }
>        }
>  
>        EnqueueLifecycleCallback(nsIDocument::eCreated, elem, nullptr, definition);
> +      //XXXsmaug It is unclear if we should use GetComposedDoc() here.

According to the spec [2], we should be using the uncomposed document here. Although, it feels like it should be spec'ed to use the composed document once it's defined.

[2] http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks

::: content/base/src/nsGenericDOMDataNode.cpp
@@ +591,5 @@
> +    // anonymous content that the document is changing.
> +    if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +      nsContentUtils::AddScriptRunner(
> +        new RemoveFromBindingManagerRunnable(document->BindingManager(), this,
> +                                             document));

I'm not sure it's a good idea to use a runnable here if we don't have to. From what I understand the reason why we use a runnable for elements is because they may have a binding and removing the binding may run destructors. I don't think there is a way to attach bindings to data nodes so all we are doing here is nulling the insertion parent and this makes it potentially run asyncronously, so we could end up with a period of time when the node is detached but still have an insertion parent.
Attachment #8497579 - Flags: review?(wchen) → review-
Attachment #8497755 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #4)
> Comment on attachment 8497579 [details] [diff] [review]
> v2
> 
> Review of attachment 8497579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/Element.cpp
> @@ +1431,5 @@
> > +  // tear down the binding. Only tear down the binding if the element
> > +  // is still no longer in the DOM. nsXBLService::LoadBinding tears
> > +  // down the old binding if the element is inserted back into the
> > +  // DOM and loads a different binding.
> > +  if (!mContent->IsInDoc()) {
> 
> It seems to me this should be IsInComposedDoc() since we also run this when
> the content is detatched from a shadow tree.
Indeed.


> The "link" element is specified to be inert inside the shadow tree [1]. We
> need to also ensure that the current element is not a HTMLLinkElement, or
> make sure all the callers in HTMLLinkElement don't call when in a shadow
> tree. This goes for the other method changed in this class.
> 
> [1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements
> 
I disagree. These Get/IsIn*Doc calls are about link state and if the link is visible we should update their state.



> ::: content/base/src/nsDocument.cpp
> @@ +6051,5 @@
> >          }
> >        }
> >  
> >        EnqueueLifecycleCallback(nsIDocument::eCreated, elem, nullptr, definition);
> > +      //XXXsmaug It is unclear if we should use GetComposedDoc() here.
> 
> According to the spec [2], we should be using the uncomposed document here.
> Although, it feels like it should be spec'ed to use the composed document
> once it's defined.
> 
> [2] http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks
Yeah, looks like custom element spec is behaving only with composed tree.
We need spec changes there.



> ::: content/base/src/nsGenericDOMDataNode.cpp
> @@ +591,5 @@
> > +    // anonymous content that the document is changing.
> > +    if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> > +      nsContentUtils::AddScriptRunner(
> > +        new RemoveFromBindingManagerRunnable(document->BindingManager(), this,
> > +                                             document));
> 
> I'm not sure it's a good idea to use a runnable here if we don't have to.
We really should be consistent with Element::UnbindFromTree.
Consistency as such is already a good enough reason to use runnable.


r+ with Element.cpp changed to use IsInComposedTree() ?
Flags: needinfo?(wchen)
Comment on attachment 8497579 [details] [diff] [review]
v2

Review of attachment 8497579 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Olli Pettay [:smaug] from comment #5)
> > The "link" element is specified to be inert inside the shadow tree [1]. We
> > need to also ensure that the current element is not a HTMLLinkElement, or
> > make sure all the callers in HTMLLinkElement don't call when in a shadow
> > tree. This goes for the other method changed in this class.
> > 
> > [1] http://w3c.github.io/webcomponents/spec/shadow/#inert-html-elements
> > 
> I disagree. These Get/IsIn*Doc calls are about link state and if the link is
> visible we should update their state.

<link> elements generally don't get a frame and aren't visible. I'm OK with this if it doesn't affect the "inertness" of <link> in the shadow DOM, but it seems like we should be using the uncomposed document in the case of <link> and other elements like <a> should use the composed document as the spec says that <link> should behave as if not part of the document tree (although, that part of the spec is one of the sections that needs some work).

> r+ with Element.cpp changed to use IsInComposedTree() ?

OK
Attachment #8497579 - Flags: review- → review+
Flags: needinfo?(wchen)
Attached patch combinedSplinter Review
https://hg.mozilla.org/mozilla-central/rev/929d78de0f8a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.