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)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(7 files)

Bug 1347411 part 1. Make sure to correctly update anon boxes belonging to ::before/::after pseudo-elements.
(deleted), text/x-review-board-request
heycam
: review+
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.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1347411 part 3. Fix stylo to properly update styles for the content container frame of <fieldset>.
(deleted), text/x-review-board-request
heycam
: review+
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.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1347411 part 5. Change stylo to correctly recompute style on the anonymous boxes that hang off comboboxes.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1347411 part 6. Change stylo to correctly recompute style of column-content anonymous boxes.
(deleted), text/x-review-board-request
heycam
: review+
Details
Bug 1347411 part 7. Change stylo to correctly recompute style of SVG anonymous wrapper boxes.
(deleted), text/x-review-board-request
heycam
: review+
Details 
      No description provided.
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+
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 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 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 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 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 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 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+
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.
> 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)
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.
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.
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.
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.
> 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.
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.
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)
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.
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
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!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: