Closed
Bug 1074738
Opened 10 years ago
Closed 10 years ago
GetCurrentDoc fixes in content/base
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: smaug, Assigned: smaug)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
47.21 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
49.34 KB,
patch
|
Details | Diff | 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)
Assignee | ||
Comment 1•10 years ago
|
||
Hmm, that breaks something :/
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
I somehow missed these.
Attachment #8497755 -
Flags: review?(wchen)
Comment 4•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8497755 -
Flags: review?(wchen) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(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 6•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(wchen)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/929d78de0f8a
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/929d78de0f8a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•