Closed
Bug 1347411
Opened 8 years ago
Closed 8 years ago
stylo: Fix dynamic style recomputation for various simple enough anon boxes
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
No description provided.
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 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8847416 [details] Bug 1347411 part 1. Make sure to correctly update anon boxes belonging to ::before/::after pseudo-elements. https://reviewboard.mozilla.org/r/120370/#review122354 ::: layout/base/ServoRestyleManager.cpp:256 (Diff revision 1) > > + if (pseudoFrame->GetStateBits() & NS_FRAME_OWNS_ANON_BOXES) { > + // XXX It really would be good to pass the actual changehint for our > + // ::before/::after here, but we never computed it! > + pseudoFrame->UpdateStyleOfOwnedAnonBoxes(*aStyleSet, aChangeList, > + nsChangeHint(0)); I think we should use nsChangeHint_Hints_NotHandledForDescendants when we don't know the right value here (since it's only used to pass through as CalcStyleDifference's aParentHintsNotHandledForDescendants argument). (Though I should soon be able to remove aParentHintsNotHandledForDescendants, and thus the aHintForThisFrame argument here, in bug 1302054.) ::: layout/reftests/generated-content/dynamic-table-cell-indent.html:19 (Diff revision 1) > + </table> > + <script> > + onload = function() { > + var tr = document.querySelector("tr"); > + // Make sure it's laid out > + window.w = tr.offsetWidth; Nit: I don't think there's any risk of the offsetWidth getter call being optimized out (assuming that's what this is a defense against), so we don't really need to assign to window.w here.
Attachment #8847416 -
Flags: review?(cam) → review+
Comment 9•8 years ago
|
||
Can we assert somewhere that NS_FRAME_OWNS_ANON_BOXES is set correctly? Maybe when we append a frame to a container, and that frame is an anonymous box, we can assert that that frame's content's primary frame has the bit set?
Flags: needinfo?(bzbarsky)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8847417 [details] Bug 1347411 part 2. Fix stylo to properly update styles for the various frames that use FCDATA_WITH_WRAPPING_BLOCK to wrap an anonymous block around their kids. https://reviewboard.mozilla.org/r/120372/#review122360 ::: layout/generic/nsHTMLCanvasFrame.cpp:427 (Diff revision 1) > + MOZ_ASSERT(mFrames.FirstChild(), "Must have our button-content anon box"); > + MOZ_ASSERT(!mFrames.FirstChild()->GetNextSibling(), > + "Must only have our button-content anon box"); s/button-content/canvas content/ or something. ::: layout/reftests/canvas/reftest-stylo.list:117 (Diff revision 1) > fails asserts-if(stylo,1) == 1304353-text-global-alpha-1.html 1304353-text-global-alpha-1.html # bug 1324700 > fails asserts-if(stylo,1) == 1304353-text-global-alpha-2.html 1304353-text-global-alpha-2.html # bug 1324700 > fails asserts-if(stylo,1) == 1304353-text-global-composite-op-1.html 1304353-text-global-composite-op-1.html # bug 1324700 > + > +skip-if(stylo) == text-indent-1a.html text-indent-1a.html # Bug 1347410 > +fails == text-indent-1b.html text-indent-1b.html Is this failing for the same reason as the other test (vw/vh flakiness)? ::: layout/reftests/forms/button/dynamic-text-indent.html:7 (Diff revision 1) > +<html class="reftest-wait"> > + <button>Some text</button> > + <script> > + onload = function() { > + var obj = document.querySelector("button"); > + window.w = obj.getBoundingClientRect().width; Does this do something different from obj.offsetWidth (in terms of flushing)?
Attachment #8847417 -
Flags: review?(cam) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8847418 [details] Bug 1347411 part 3. Fix stylo to properly update styles for the content container frame of <fieldset>. https://reviewboard.mozilla.org/r/120374/#review122364 ::: layout/reftests/forms/fieldset/dynamic-text-indent.html:13 (Diff revision 1) > + f.style.textIndent = "100px"; > + document.documentElement.className = ""; > + } > + </script> > +</html> > + Nit: drop this line.
Attachment #8847418 -
Flags: review?(cam) → review+
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8847419 [details] Bug 1347411 part 4. Change the various anonymous boxes associated with framesets to be non-inheriting, since their styles aren't really used for anything. https://reviewboard.mozilla.org/r/120376/#review122366
Attachment #8847419 -
Flags: review?(cam) → review+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8847420 [details] Bug 1347411 part 5. Change stylo to correctly recompute style on the anonymous boxes that hang off comboboxes. https://reviewboard.mozilla.org/r/120378/#review122368 ::: layout/reftests/forms/select/dynamic-text-overflow-1-ref.html:2 (Diff revision 1) > +<!DOCTYPE html> > +<select style="width: 100px; overflow: hidden; text-overflow: ellipsis;">> Nit: one too many ">"s.
Attachment #8847420 -
Flags: review?(cam) → review+
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8847421 [details] Bug 1347411 part 6. Change stylo to correctly recompute style of column-content anonymous boxes. https://reviewboard.mozilla.org/r/120380/#review122372
Attachment #8847421 -
Flags: review?(cam) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8847422 [details] Bug 1347411 part 7. Change stylo to correctly recompute style of SVG anonymous wrapper boxes. https://reviewboard.mozilla.org/r/120382/#review122376 Can you add a test for the SVG outer frame? r=me with that. ::: layout/reftests/svg/marker-dynamic-opacity-ref.html:10 (Diff revision 1) > + markerWidth="6" markerHeight="6" orient="auto" > + style="opacity: 0.2"> > + <path d="M 0 0 L 10 5 L 0 10 z" /> > + </marker> > + </defs> > + <polyline points="10,90 50,80 90,20" fill="none" stroke="black" Nit: trailing space. ::: layout/reftests/svg/marker-dynamic-opacity-ref.html:13 (Diff revision 1) > + > + Nit: drop these two lines. ::: layout/reftests/svg/marker-dynamic-opacity.html:10 (Diff revision 1) > + <marker id="Triangle" viewBox="0 0 10 10" refX="1" refY="5" > + markerWidth="6" markerHeight="6" orient="auto"> > + <path d="M 0 0 L 10 5 L 0 10 z" /> > + </marker> > + </defs> > + <polyline points="10,90 50,80 90,20" fill="none" stroke="black" Nit: and here. ::: layout/reftests/svg/marker-dynamic-opacity.html:21 (Diff revision 1) > + > + Nit: and these two.
Attachment #8847422 -
Flags: review?(cam) → review+
![]() |
Assignee | |
Comment 16•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847416 [details] Bug 1347411 part 1. Make sure to correctly update anon boxes belonging to ::before/::after pseudo-elements. https://reviewboard.mozilla.org/r/120370/#review122354 > I think we should use nsChangeHint_Hints_NotHandledForDescendants when we don't know the right value here (since it's only used to pass through as CalcStyleDifference's aParentHintsNotHandledForDescendants argument). > > (Though I should soon be able to remove aParentHintsNotHandledForDescendants, and thus the aHintForThisFrame argument here, in bug 1302054.) Done, good catch. 0 is clearly the wrong thing here. > Nit: I don't think there's any risk of the offsetWidth getter call being optimized out (assuming that's what this is a defense against), so we don't really need to assign to window.w here. Not yet. In the future, who knows. I've considered adding some smarts to bindings to optimize it out in situations like this, fwiw.
![]() |
Assignee | |
Comment 17•8 years ago
|
||
> Maybe when we append a frame to a container, and that frame is an anonymous box, we
> can assert that that frame's content's primary frame has the bit set?
That won't work, for at least two reasons:
1) We're not using this mechanism for all anonymous boxes, just a subset. We could bake knowledge of that subset into the assert, I guess....
2) There are situations where this doesn't actually hold. For example:
<div style="display: table-row">Text</div>
Here the frame with the bit set (for handling its cellcontent block) is the anonymous table cell frame we create, but the primary frame of its content (which is the <div>) is the table row frame.
I'll think a bit about how we can assert this.
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 18•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847417 [details] Bug 1347411 part 2. Fix stylo to properly update styles for the various frames that use FCDATA_WITH_WRAPPING_BLOCK to wrap an anonymous block around their kids. https://reviewboard.mozilla.org/r/120372/#review122360 > s/button-content/canvas content/ or something. Good catch, done. > Is this failing for the same reason as the other test (vw/vh flakiness)? I think so, yes. When I wrote the test it failed because we just had no vw support, but now I think it's just flakiness. I'll add the bug annotation here. > Does this do something different from obj.offsetWidth (in terms of flushing)? No, but there is no offsetWidth on SVG elements.
![]() |
Assignee | |
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847418 [details] Bug 1347411 part 3. Fix stylo to properly update styles for the content container frame of <fieldset>. https://reviewboard.mozilla.org/r/120374/#review122364 > Nit: drop this line. Done.
![]() |
Assignee | |
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847420 [details] Bug 1347411 part 5. Change stylo to correctly recompute style on the anonymous boxes that hang off comboboxes. https://reviewboard.mozilla.org/r/120378/#review122368 > Nit: one too many ">"s. Good catch, fixed.
![]() |
Assignee | |
Comment 21•8 years ago
|
||
Upon reflection this morning, I found two bugs in part 6: 1) I was setting the NS_FRAME_OWNS_ANON_BOXES flag for continuing columnset frames. That's wrong. I've moved it into the three other callers of NS_NewColumnSetFrame instead, like so: nsContainerFrame* columnSetFrame = NS_NewColumnSetFrame(mPresShell, aStyleContext, nsFrameState(NS_FRAME_OWNS_ANON_BOXES)); 2) I was walking the entire mFrames list in nsColumnSetFrame::DoUpdateStyleOfOwnedAnonBoxes but all of those are next-in-flows of the first one, and UpdateStyleOfChildAnonBox already walks continuations. So I only need to do this for the first thing in mFrames. Fixed accordingly, with some comments.
![]() |
Assignee | |
Comment 22•8 years ago
|
||
> and UpdateStyleOfChildAnonBox already walks continuations
I also added an assert to UpdateStyleOfChildAnonBox that its arg's GetPrevContinuation is null, to catch this in the future.
![]() |
Assignee | |
Comment 23•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847422 [details] Bug 1347411 part 7. Change stylo to correctly recompute style of SVG anonymous wrapper boxes. https://reviewboard.mozilla.org/r/120382/#review122376 > Nit: trailing space. Fixed. > Nit: drop these two lines. Done. > Nit: and here. Fixed. > Nit: and these two. Fixed.
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 31•8 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s e03a4d02e124 -d 7df65af169f5: rebasing 382197:e03a4d02e124 "Bug 1347411 part 1. Make sure to correctly update anon boxes belonging to ::before/::after pseudo-elements. r=heycam" merging layout/base/ServoRestyleManager.cpp merging layout/reftests/generated-content/reftest-stylo.list warning: conflicts while merging layout/reftests/generated-content/reftest-stylo.list! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
![]() |
Assignee | |
Comment 32•8 years ago
|
||
Going to land this directly, not via mozreview, because in the usual way rebasing on top of autoland makes it impossible to push to mozreview.
Comment 33•8 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/466383d8137c part 1. Make sure to correctly update anon boxes belonging to ::before/::after pseudo-elements. r=heycam https://hg.mozilla.org/integration/autoland/rev/e150bb1b7484 part 2. Fix stylo to properly update styles for the various frames that use FCDATA_WITH_WRAPPING_BLOCK to wrap an anonymous block around their kids. r=heycam https://hg.mozilla.org/integration/autoland/rev/396da770127f part 3. Fix stylo to properly update styles for the content container frame of <fieldset>. r=heycam https://hg.mozilla.org/integration/autoland/rev/95e3f7eb66ce part 4. Change the various anonymous boxes associated with framesets to be non-inheriting, since their styles aren't really used for anything. r=heycam https://hg.mozilla.org/integration/autoland/rev/3c00ccf0cf5e part 5. Change stylo to correctly recompute style on the anonymous boxes that hang off comboboxes. r=heycam https://hg.mozilla.org/integration/autoland/rev/bac9ba31bdf1 part 6. Change stylo to correctly recompute style of column-content anonymous boxes. r=heycam https://hg.mozilla.org/integration/autoland/rev/aa279848f213 part 7. Change stylo to correctly recompute style of SVG anonymous wrapper boxes. r=heycam
Comment 34•8 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/56cfd054a215 followup. Unmark the canvas text-indent test as failing, because now it passes. Oh, for non-random vw/vh behavior!
Comment 35•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/466383d8137c https://hg.mozilla.org/mozilla-central/rev/e150bb1b7484 https://hg.mozilla.org/mozilla-central/rev/396da770127f https://hg.mozilla.org/mozilla-central/rev/95e3f7eb66ce https://hg.mozilla.org/mozilla-central/rev/3c00ccf0cf5e https://hg.mozilla.org/mozilla-central/rev/bac9ba31bdf1 https://hg.mozilla.org/mozilla-central/rev/aa279848f213 https://hg.mozilla.org/mozilla-central/rev/56cfd054a215
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•