Closed Bug 1404789 Opened 7 years ago Closed 7 years ago

Stop reconstructing frames for the whole shadow root each time content is inserted in a shadow tree.

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(13 files)

(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/plain
Details
(deleted), text/plain
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
(deleted), text/x-review-board-request
bzbarsky
: review+
Details
Right now we have this hack in tree (and two other similar-looking lines): http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/layout/base/nsCSSFrameConstructor.cpp#7681 As the comment there by Olli says, that's super-inefficient, and causes all sorts of badness, so we should probably fix it if we're being serious about implementing shadow DOM.
Priority: -- → P3
Depends on: 1407152
Depends on: 1407586
My patch is green except for accessible/tests/mochitest/treeupdate/test_bug1276857.html. This is somewhat expected because we're notifying more async of the content insertion I think, but I think a11y should be able to deal with it, so I suspect this uncovers a bug on their end: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f171157db4b36c423ee03295c73d51d80f5c16b7&selectedJob=137267790 Alexander, do you think it's acceptable to skip / mark as failure / adjust this test for now? I'm trying to re-enable Shadow-DOM support on Stylo, and this test fails consistently both with Stylo and without it when I remove this hack, in the same way. I think this is an accessibility bug, but I'd really wouldn't want to block this and bug 1409079 on that test, since it relies on this hack.
Flags: needinfo?(surkov.alexander)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2) > Created attachment 8919004 [details] > Bug 1404789: Remove shadow tree hacks in the frame constructor. > > Turns out that the patches in bug 1398448 fixed most of the layout stuff. I need to bisect more, fwiw, this bug is in the range, but I need to close it...
Blocks: 1409079
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #2) > > Created attachment 8919004 [details] > > Bug 1404789: Remove shadow tree hacks in the frame constructor. > > > > Turns out that the patches in bug 1398448 fixed most of the layout stuff. > > I need to bisect more, fwiw, this bug is in the range, but I need to close > it... Err, disregard this entirely, that bug is not the culprit. I'll try to bisect it because I'm curious (I remember to have tried this exact same patch a while before without success), but I won't block landing this on it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1) > My patch is green except for > accessible/tests/mochitest/treeupdate/test_bug1276857.html. > > This is somewhat expected because we're notifying more async of the content > insertion I think, but I think a11y should be able to deal with it, so I > suspect this uncovers a bug on their end: a11y tree updates are all async, so the missed accessible text points to either the tree is updated twice (aka two reorder events) or we fail to update tree properly. Could you please do some a11y logging before and after the patch (via 'export A11YLOG=tree' env variable setting) to make sure the layout sends all notifications to a11y?
Attached file A11YLOG=tree without patch (deleted) —
Attached file A11YLOG=tree with patch (deleted) —
So it looks like we're getting one less content insertion notification. In general getting less notifications with the patch makes sense, because what the code before this patch did when you inserted content in a shadow tree was to remove all the frames (and notify), and then to reinsert again (and also notify). Now we use the same machinery the rest of Gecko uses...
So, I need to go to sleep for today. I can try to look into the test more in depth tomorrow, but given I have absolutely 0 experience debugging a11y tests, and that wherever this bug is, it is relying on the hack I'm removing (which I'm 100% sure we need to remove), it may take some time for me to find it :(.
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review195252 I don't understand why these changes are OK. What actually makes sure that we redistribute content to new insertion points or whatnot as the shadow tree is mutated? ::: commit-message-7af89:5 (Diff revision 1) > +Bug 1404789: Remove shadow tree hacks in the frame constructor. r?bz > + > +Turns out that the patches in bug 1398448 fixed most of the layout stuff. > + > +There's a GeckoRestyleManager assertion in test_shadowroot_style.html about Well, so... Stylo force-disables shadow DOM. So we don't really have stylo test coverage for this test, it's skip-if(stylo). That means we have no idea what asserts stylo may or may not be hitting here, and so it might be worth understanding whether the assertion that fires corresponds to a problem that stylo would also be subject to.
Attachment #8919004 - Flags: review?(bzbarsky)
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review195252 > Well, so... > > Stylo force-disables shadow DOM. So we don't really have stylo test coverage for this test, it's skip-if(stylo). > > That means we have no idea what asserts stylo may or may not be hitting here, and so it might be worth understanding whether the assertion that fires corresponds to a problem that stylo would also be subject to. Note that the point is re-enabling stylo Shadow DOM in bug 1409079 along these changes, and stylo doesn't hit similar problems in this test. The shadow root changes causes a reframe of the whole binding parent, see the callers of `nsCSSFrameConstructor::DestroyFramesFor` in `Element::CreateShadowRoot`.
This doesn't really block re-enabling web components on Stylo.
No longer blocks: 1409079
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. Re-requesting review because the skip-if for stylo is going away in bug 1409079. Let me know if you still want me to look into it, though I think debugging GeckoRestyleManager stuff at this point for WebComponents stuff is not a good use of my (or anyone's) time. In particular, I expect most of these failures to be variants of bug 1389936, that is, stuff that GeckoRestyleManager assumes about the correspondence between DOM tree and frame tree which aren't true in Shadow DOM.
Attachment #8919004 - Flags: review?(bzbarsky)
Depends on: 1389743
Per IRC conversation, comment 14 just shows that now we construct frames more lazily, so a testcase like this (from <http://searchfox.org/mozilla-central/source/layout/reftests/forms/legend/shadow-dom.html>): function shadow(id) { return document.getElementById(id).createShadowRoot(); } shadow("host3").innerHTML = 'a <content></content>'; manages to pass because we tear down the frames for host3 when calling creataeShadowRoot() and don't recreate until later, when the shadow tree has already been modified. But doing a layout flush inside shadow() after calling createShadowRoot() but before returning the result would make the testcase fail again. This sort of situation is what the code being removed in this bug is trying to handle: dynamic changes to the set of insertion points in the shadow tree...
In particular, after bug 1389743, any test that calls createShadowRoot() but doesn't immediately do at least a frames flush is not testing what it thinks it's testing...
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review195662
Attachment #8919004 - Flags: review?(bzbarsky)
Blocks: 1286419
Comment on attachment 8919709 [details] Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. Cancelling review on this one for now, I've noticed some fishyness I'm trying to address...
Attachment #8919709 - Flags: review?(bzbarsky)
So with all this stuff and https://github.com/servo/servo/pull/18937, Stylo passes all the shadow DOM tests I'm aware of. That accessibility test still fails in both though, and I'm trying to investigate why. All the other shadow-dom related accessibility tests are re-enabled and passing fwiw.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #39) > So with all this stuff and https://github.com/servo/servo/pull/18937, Stylo > passes all the shadow DOM tests I'm aware of. > > That accessibility test still fails in both though, and I'm trying to > investigate why. All the other shadow-dom related accessibility tests are > re-enabled and passing fwiw. This is a bug, and I have a test-case now...
Comment on attachment 8919707 [details] Bug 1404789: Cleanup a bit the ShadowRoot code. https://reviewboard.mozilla.org/r/190614/#review196022 ::: dom/base/ShadowRoot.h:57 (Diff revision 2) > bool ApplyAuthorStyles(); > void SetApplyAuthorStyles(bool aApplyAuthorStyles); > StyleSheetList* StyleSheets(); > > +private: > +>>>>>>> d6e11640eda7... fixup! Bug 1404789: Cleanup a bit the ShadowRoot code. r?bz This line should not be here. ::: dom/base/ShadowRoot.cpp:266 (Diff revision 2) > return; > } > > // Matching may cause the insertion point to drop fallback content. > - if (mInsertionPoints[i]->MatchedNodes().IsEmpty() && > - static_cast<nsINode*>(mInsertionPoints[i])->GetFirstChild()) { > + if (insertionPoint->MatchedNodes().IsEmpty() && > + static_cast<nsINode*>(insertionPoint)->GetFirstChild()) { Why not insertionPoint->HasChildren() instead? I expect you can drop the cast if you do that. ::: dom/base/ShadowRoot.cpp:327 (Diff revision 2) > + } > + > - // Removing the matched node may cause the insertion point to use > + // Removing the matched node may cause the insertion point to use > - // fallback content. > + // fallback content. > - if (mInsertionPoints[i]->MatchedNodes().Length() == 1 && > - static_cast<nsINode*>(mInsertionPoints[i])->GetFirstChild()) { > + if (insertionPoint->MatchedNodes().Length() == 1 && > + static_cast<nsINode*>(insertionPoint)->GetFirstChild()) { Again, drop the cast and HasChildren().
Attachment #8919707 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919708 [details] Bug 1404789: Privatize ShadowRoot methods. https://reviewboard.mozilla.org/r/190616/#review196086 So this just privatizes IsPooledNode, right? r=me
Attachment #8919708 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919709 [details] Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. https://reviewboard.mozilla.org/r/190618/#review196088 ::: dom/base/ShadowRoot.h:76 (Diff revision 2) > * insertion points in this ShadowRoot. > */ > void DistributeAllNodes(); > > + /* > + * Called when we redistribute content in such a way that there may be new I'm not quite sure I follow this. New insertion points coming into existence, or new insertion points in what sense? ::: dom/base/ShadowRoot.cpp:266 (Diff revision 2) > + auto* shell = OwnerDoc()->GetShell(); > + if (!shell) { > + return; > + } > + > + // FIXME(emilio): Rename this to DestroyFramesForAndRestyle? Please. File a followup for that. ::: dom/base/ShadowRoot.cpp:332 (Diff revision 2) > // insertion points of the parent's ShadowRoot. > if (auto* parentShadow = foundInsertionPoint->GetParent()->GetShadowRoot()) { > parentShadow->DistributeSingleNode(aContent); > } > + > + DistributionChanged(); So my question about when DistributionChanged() should be called... This code is called when nodes are added in the _light_ tree (or otherwise change which insertion point they should be in), right? Reframing the shadow host any time a light DOM child is added to it seems pretty heavyweight and really shouldn't be necessary. Can you explain what this patch is trying to do and why?
Attachment #8919709 - Flags: review?(bzbarsky)
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review196090 I don't see why this is safe to do at this point. We're now reframing for light DOM changes to the host's kids (previous patch), but this code was dealing with changes in the shadow DOM itself, no?
Attachment #8919004 - Flags: review?(bzbarsky)
Comment on attachment 8919710 [details] Bug 1404789: Add a few layout flushes to legend/shadow-dom.html that would've caught the first attempt. https://reviewboard.mozilla.org/r/190620/#review196094 r=me, but might be worth having comments on those flushes explaining why they're there. And maybe a version of this test _with_ the flushes and a version without.
Attachment #8919710 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196096 ::: layout/base/nsCSSFrameConstructor.cpp:7271 (Diff revision 2) > #endif > > // For inserts aChild should be valid, for appends it should be null. > // Returns true if this operation can be lazy, false if not. > +// > +// FIXME(emilio): This function assumes that the flattened tree parent of all Worth a bug filed, with a bug number. ::: layout/base/nsCSSFrameConstructor.cpp:7313 (Diff revision 2) > // We can construct lazily; just need to set suitable bits in the content > // tree. > + nsIContent* parent = aChild->GetFlattenedTreeParent(); > + if (!parent) { > + // Not part of the flat tree, nothing to do. > + return true; Hmm. So we'll return true, then set the lazy bits on the node, then they will stick around for a while, because we will never try to actually construct frames for it, right? Is that ok? Seems a bit weird to have those bits hanging around. In particular, can we _become_ part of the flattened tree without something unsetting the lazy FC bits? If we can, that seems bad, if I remember the invariants around when those bits get set correctly.
Attachment #8919711 - Flags: review?(bzbarsky)
Attachment #8919712 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919743 [details] Bug 1404789: Remove <style scoped> test from display-contents-shadow-dom-1.html. https://reviewboard.mozilla.org/r/190630/#review196100 I'd rather we clone the test and modify one of the versions, so we test both options for the moment. r=me with that.
Attachment #8919743 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919742 [details] Bug 1404789: Simplify ShadowRoot::IsPooledNode. https://reviewboard.mozilla.org/r/190628/#review196102 r=me ::: dom/base/ShadowRoot.cpp:480 (Diff revision 2) > // Insertion points never end up in the pool. > return false; > } > > - if (aContainer == aHost && > - nsContentUtils::IsInSameAnonymousTree(aContainer, aContent)) { > + auto* host = GetHost(); > + auto* container = aContent->GetParent(); Hmm. I guess we could remove this arg from ContentAppended/ContentRemoved/ContentInserted, really... Alright.
Attachment #8919742 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919811 [details] Bug 1404789: Be a bit better at detecting distribution changes. https://reviewboard.mozilla.org/r/190738/#review196104 r=me with the nits below ::: dom/base/ShadowRoot.h:61 (Diff revision 2) > private: > /** > * Distributes a single explicit child of the pool host to the content > * insertion points in this ShadowRoot. > + * > + * Returns the new insertion point, if any. It's not very clear that the "new" insertion point may be the one the element was in to start with.... Maybe we should say that it returns the insertion point the element is distributed to after this call, and note that it may already have been distributed there? ::: dom/base/ShadowRoot.h:63 (Diff revision 2) > * Distributes a single explicit child of the pool host to the content > * insertion points in this ShadowRoot. > + * > + * Returns the new insertion point, if any. > + * > + * Note that this doesn't handle removing the node in the insertion point I don't quite follow this comment. Which "the insertion point" do we need to get removed from and why? Or did you mean to say "adding" instead of "removing" or so? ::: dom/base/ShadowRoot.h:74 (Diff revision 2) > * Removes a single explicit child of the pool host from the content > * insertion points in this ShadowRoot. > + * > + * Returns the old insertion point, if any. > + * > + * Note that this doesn't handle removing the node in the insertion point Here I assume "the insertion point" means the returned insertion point? ::: dom/base/ShadowRoot.cpp:289 (Diff revision 2) > static_cast<nsINode*>(insertionPoint)->GetFirstChild()) { > // This match will cause the insertion point to drop all fallback > // content and used matched nodes instead. Give up on the optimization > // and just distribute all nodes. > DistributeAllNodes(); > - return; > + return insertionPoint; Should we assert that after the DistributeAllNodes() call aContent is in insertionPoint->MatchedNodes()? Seems like we should. ::: dom/base/ShadowRoot.cpp:499 (Diff revision 2) > return; > } > > // Attributes may change insertion point matching, find its new distribution. > - RemoveDistributedNode(aElement); > - DistributeSingleNode(aElement); > + // > + // FIXME(emilio): What about state changes? State changes can't affect distribution. The shadow DOM version we implement so far restricts the selectors allowed to affect `<content>` matching; see IsValidContentSelectors in HTMLContentElement.cpp. In particular, pseudo-element selectors, pseudo-class selectors, and combinators are all excluded. That leaves selection by namespace, localname, class, id, and attributes. ::: dom/base/ShadowRoot.cpp:504 (Diff revision 2) > - DistributeSingleNode(aElement); > + // FIXME(emilio): What about state changes? > + if (!RedistributeElement(aElement)) { > + return; > -} > + } > > -void > + auto* shell = OwnerDoc()->GetShell(); Why do we want to do this if we're not in the doc? ::: dom/base/ShadowRoot.cpp:505 (Diff revision 2) > + if (!RedistributeElement(aElement)) { > + return; > -} > + } > > -void > -ShadowRoot::ContentAppended(nsIDocument* aDocument, > + auto* shell = OwnerDoc()->GetShell(); > + shell->DestroyFramesFor(aElement); shell could be null here... Needs tests. ::: dom/base/ShadowRoot.cpp (Diff revision 2) > return; > } > > - // Watch for new nodes added to the pool because the node > - // may need to be added to an insertion point. > - if (IsPooledNode(aChild)) { Why do we not need this check around the aChild->DestInsertionPoints().AppendElement() bit anymore? ::: dom/base/ShadowRoot.cpp:581 (Diff revision 2) > - DistributeSingleNode(aChild); > + // Watch for new nodes added to the pool because the node > + // may need to be added to an insertion point. > + if (IsPooledNode(aChild)) { > + auto* insertionPoint = DistributeSingleNode(aChild); > + while (insertionPoint) { > + // Handle the case where the parent of the insertion point has a ShadowRoot. Comments like this in the other places where you have this sort of loop would be useful.
Attachment #8919811 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919843 [details] Bug 1404789: Get the insertion point right when reconstructing direct children of a shadow root. https://reviewboard.mozilla.org/r/190788/#review196106 ::: layout/base/nsCSSFrameConstructor.cpp:7788 (Diff revision 2) > // We use the provided tree match context, or create a new one on the fly > // otherwise. > Maybe<TreeMatchContext> matchContext; > if (!aProvidedTreeMatchContext && !aContainer->IsStyledByServo()) { > matchContext.emplace(mDocument, TreeMatchContext::ForFrameConstruction); > - matchContext->InitAncestors(aContainer->AsElement()); > + matchContext->InitAncestors(aFirstNewContent->GetParentElementCrossingShadowRoot()); This could use a comment explaining why we're not using aContainer, because it's not at all clear. ::: layout/base/nsCSSFrameConstructor.cpp:8258 (Diff revision 2) > } > > Maybe<TreeMatchContext> matchContext; > if (!aProvidedTreeMatchContext && !aContainer->IsStyledByServo()) { > matchContext.emplace(mDocument, TreeMatchContext::ForFrameConstruction); > - matchContext->InitAncestors(aContainer ? aContainer->AsElement() : nullptr); > + matchContext->InitAncestors(aStartChild->GetParentElementCrossingShadowRoot()); Again, document. ::: layout/reftests/webcomponents/reframe-shadow-child-1.html:15 (Diff revision 2) > +<script> > + let shadowRoot = document.getElementById("host").createShadowRoot(); > + let tmpl = document.getElementById("tmpl"); > + shadowRoot.appendChild(document.importNode(tmpl.content, true)); > + document.body.offsetTop; > + shadowRoot.firstElementChild.querySelector("span").remove(); Did we need to use anon table bits here? Could we have used an explicit display change on the shadow root child?
Attachment #8919843 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919811 [details] Bug 1404789: Be a bit better at detecting distribution changes. https://reviewboard.mozilla.org/r/190738/#review196174 ::: dom/base/ShadowRoot.cpp (Diff revision 2) > return; > } > > - // Watch for new nodes added to the pool because the node > - // may need to be added to an insertion point. > - if (IsPooledNode(aChild)) { Because it's wrong, and it wasn't there in the `ContentAppended` calls.
Comment on attachment 8919709 [details] Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. https://reviewboard.mozilla.org/r/190618/#review196176 ::: dom/base/ShadowRoot.h:76 (Diff revision 2) > * insertion points in this ShadowRoot. > */ > void DistributeAllNodes(); > > + /* > + * Called when we redistribute content in such a way that there may be new Right, new insertion points come into existence and we need to shuffle around content. Will tweak the comment. ::: dom/base/ShadowRoot.cpp:332 (Diff revision 2) > // insertion points of the parent's ShadowRoot. > if (auto* parentShadow = foundInsertionPoint->GetParent()->GetShadowRoot()) { > parentShadow->DistributeSingleNode(aContent); > } > + > + DistributionChanged(); Indeed, this is heavyweight and goes away in the end of the series.
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review196090 When `<content>` is bound / unbound from the tree, it calls `DistributeAllNodes` in the containing shadw, which calls `DistributionChanged`, which reframes the host, so this can go away. When stuff is rebound, we rely on `DistributeSingleNode` / `RemoveDistributedNodes` to call that if there's a change in the shadow tree.
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196182 ::: layout/base/nsCSSFrameConstructor.cpp:7313 (Diff revision 2) > // We can construct lazily; just need to set suitable bits in the content > // tree. > + nsIContent* parent = aChild->GetFlattenedTreeParent(); > + if (!parent) { > + // Not part of the flat tree, nothing to do. > + return true; What? I don't get this. We return true, so there's no bits set in any case. The reason the code is here is pretty intentional, and it's so we don't set the `NODE_NEEDS_FRAME` bits, which is what the code until now means. Maybe I'm misreading your question?
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196096 > Worth a bug filed, with a bug number. Will do
Comment on attachment 8919709 [details] Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. Re-requesting review on this one because even though I agree this is super-heavyweight, it does handle shadow tree changes, and the heavyweight parts go away at the end of the series.
Attachment #8919709 - Flags: review?(bzbarsky)
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. Re-requesting review on this one because we return before setting the bits. Still need to address the other comments here and everywhere else.
Attachment #8919711 - Flags: review?(bzbarsky)
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. Aand, re-requesting review on this one because the patch right above it is what handles the shadow tree changes. I'll go address the comments and write some tests for them now.
Attachment #8919004 - Flags: review?(bzbarsky)
Flags: needinfo?(surkov.alexander)
Blocks: 1410020
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196096 > Will do Filed bug 1410020.
Comment on attachment 8919811 [details] Bug 1404789: Be a bit better at detecting distribution changes. https://reviewboard.mozilla.org/r/190738/#review196174 > Because it's wrong, and it wasn't there in the `ContentAppended` calls. Ah, so per IRC discussion the key part is that we do want to test IsPooledNode() before distributing into insertion points, but not before doing the fallback content bits. As in, you're moving the check, not removing it.
Comment on attachment 8919709 [details] Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. https://reviewboard.mozilla.org/r/190618/#review196430 ::: commit-message-4ae34:2 (Diff revision 3) > +Bug 1404789: When the shadow tree distribution changes, post a restyle + reframe. r?bz > + So for future reference, what this commit message should say in the non-summary section is something like: Whenever insertion points are added or removed, we want to reframe the shadow host, by calling DistributionChanged() from the DistributeAllNodes() triggered by insertion point changes. When nodes are distributed into existing insertion points, we also reframe the shadow host, for now. This will be improved in later changesets in this series.
Attachment #8919709 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919004 [details] Bug 1404789: Remove shadow tree hacks in the frame constructor. https://reviewboard.mozilla.org/r/189890/#review196432
Attachment #8919004 - Flags: review?(bzbarsky) → review+
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196182 > What? I don't get this. We return true, so there's no bits set in any case. > > The reason the code is here is pretty intentional, and it's so we don't set the `NODE_NEEDS_FRAME` bits, which is what the code until now means. > > Maybe I'm misreading your question? No, I was mireading the code, or rather not reading enough context. So this adds a new mode to MaybeConstructLazily, where we return true but don't set the flags. That seems reasonable, ok.
Comment on attachment 8919711 [details] Bug 1404789: Do not assume that we're part of the flattened tree when we get notified in the frame constructor. https://reviewboard.mozilla.org/r/190622/#review196438
Attachment #8919711 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7a1a307de169 Cleanup a bit the ShadowRoot code. r=bz https://hg.mozilla.org/integration/autoland/rev/c097b5cc5c07 Privatize ShadowRoot methods. r=bz https://hg.mozilla.org/integration/autoland/rev/f5345c4fda8c When the shadow tree distribution changes, post a restyle + reframe. r=bz https://hg.mozilla.org/integration/autoland/rev/4fae76403e5d Remove shadow tree hacks in the frame constructor. r=bz https://hg.mozilla.org/integration/autoland/rev/86eda2115d75 Add a few layout flushes to legend/shadow-dom.html that would've caught the first attempt. r=bz https://hg.mozilla.org/integration/autoland/rev/45e439db72eb Do not assume that we're part of the flattened tree when we get notified in the frame constructor. r=bz https://hg.mozilla.org/integration/autoland/rev/c4f26703a9d0 Update test expectations. r=bz https://hg.mozilla.org/integration/autoland/rev/c9a117613499 Remove <style scoped> test from display-contents-shadow-dom-1.html. r=bz https://hg.mozilla.org/integration/autoland/rev/1fff04170822 Simplify ShadowRoot::IsPooledNode. r=bz https://hg.mozilla.org/integration/autoland/rev/fff75157a8cd Be a bit better at detecting distribution changes. r=bz https://hg.mozilla.org/integration/autoland/rev/e214368792a2 Get the insertion point right when reconstructing direct children of a shadow root. r=bz
Blocks: 1410259
Blocks: 1410212
Blocks: 1411754
Blocks: 1409086
Blocks: 1409136
Assignee: nobody → emilio
Depends on: 1414692
Depends on: 1414902
Depends on: 1415353
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: