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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla58
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.
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
(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...
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
(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?
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
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...
Assignee | ||
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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`.
Assignee | ||
Comment 12•7 years ago
|
||
This doesn't really block re-enabling web components on Stylo.
No longer blocks: 1409079
Assignee | ||
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
So, I'm just done bisecting this. Before bug 1389743:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=146444beeb6612487a5300f4978b428e8b7eb407
After bug 1389743:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=677de290fd241ab676b1f576b0f6024faa445661
Which makes sense.
Comment 15•7 years ago
|
||
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...
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•7 years ago
|
||
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.
Assignee | ||
Comment 40•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 44•7 years ago
|
||
mozreview-review |
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 45•7 years ago
|
||
mozreview-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 46•7 years ago
|
||
mozreview-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 47•7 years ago
|
||
mozreview-review |
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 48•7 years ago
|
||
mozreview-review |
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 49•7 years ago
|
||
mozreview-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)
Comment 50•7 years ago
|
||
mozreview-review |
Comment on attachment 8919712 [details]
Bug 1404789: Update test expectations.
https://reviewboard.mozilla.org/r/190624/#review196098
r=me
Attachment #8919712 -
Flags: review?(bzbarsky) → review+
Comment 51•7 years ago
|
||
mozreview-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 52•7 years ago
|
||
mozreview-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 53•7 years ago
|
||
mozreview-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 54•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 55•7 years ago
|
||
mozreview-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.
Assignee | ||
Comment 56•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 57•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 58•7 years ago
|
||
mozreview-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
::: 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?
Assignee | ||
Comment 59•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 60•7 years ago
|
||
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)
Assignee | ||
Comment 61•7 years ago
|
||
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)
Assignee | ||
Comment 62•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 63•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 75•7 years ago
|
||
mozreview-review-reply |
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 76•7 years ago
|
||
mozreview-review |
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 77•7 years ago
|
||
mozreview-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 78•7 years ago
|
||
mozreview-review-reply |
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 79•7 years ago
|
||
mozreview-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/#review196438
Attachment #8919711 -
Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 92•7 years ago
|
||
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
Comment 93•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a1a307de169
https://hg.mozilla.org/mozilla-central/rev/c097b5cc5c07
https://hg.mozilla.org/mozilla-central/rev/f5345c4fda8c
https://hg.mozilla.org/mozilla-central/rev/4fae76403e5d
https://hg.mozilla.org/mozilla-central/rev/86eda2115d75
https://hg.mozilla.org/mozilla-central/rev/45e439db72eb
https://hg.mozilla.org/mozilla-central/rev/c4f26703a9d0
https://hg.mozilla.org/mozilla-central/rev/c9a117613499
https://hg.mozilla.org/mozilla-central/rev/1fff04170822
https://hg.mozilla.org/mozilla-central/rev/fff75157a8cd
https://hg.mozilla.org/mozilla-central/rev/e214368792a2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Assignee: nobody → emilio
You need to log in
before you can comment on or make changes to this bug.
Description
•