1340723 - stylo: dynamic changes to CSS offsets on relatively positioned (rel-pos) inline with a block child don't work right

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Because we don't restyle the anon block anonymous box.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase that doesn't involve positioning

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1340723 part 1. Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls.

Review commit: https://reviewboard.mozilla.org/r/116622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116622/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1340723 part 2. Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager.

Review commit: https://reviewboard.mozilla.org/r/116624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116624/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1340723 part 3. Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.

Review commit: https://reviewboard.mozilla.org/r/116626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116626/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8842889 [details]
Bug 1340723 part 1.  Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls.

https://reviewboard.mozilla.org/r/116622/#review118378

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8842890 [details]
Bug 1340723 part 2.  Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager.

https://reviewboard.mozilla.org/r/116624/#review118380

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8842891 [details]
Bug 1340723 part 3.  Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.

https://reviewboard.mozilla.org/r/116626/#review118306

Awesome! Thanks for fixing this :)

r=me w/ nits

::: layout/generic/nsInlineFrame.h:121
(Diff revision 1)
>               : (!GetNextInFlow());
>    }
>  
> +  // Update the style on the block wrappers around our non-inline-outside kids.
> +  // This will only be called when such wrappers in fact exist.
> +  virtual void UpdateStyleOfOwnedAnonBoxes(mozilla::ServoStyleSet* aStyleSet,

nit: no need for `virtual`.

::: layout/generic/nsInlineFrame.cpp:1036
(Diff revision 1)
> +  MOZ_ASSERT(GetStateBits() & NS_FRAME_OWNS_ANON_BOXES,
> +             "Why did we get called?");
> +  MOZ_ASSERT(GetStateBits() & NS_FRAME_PART_OF_IBSPLIT,
> +             "Why did we have the NS_FRAME_OWNS_ANON_BOXES bit set?");
> +  // Note: this assert _looks_ expensive, but it's cheap in all the cases when
> +  // it passes!

Also, I guess IB splits aren't ultra-common like, let's say, text frames (though probably not specially rare).

::: layout/reftests/ib-split/relpos-inline-1a.html:12
(Diff revision 1)
> +    <link rel="help" href="http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level"/>
> +    <meta name="flags" content="" />
> +    <style>
> +      span { position: relative; left: 200px }
> +      html {
> +        overflow: hidden;

I can't believe we had no tests for this tbh :/.

::: layout/reftests/ib-split/relpos-inline-1b.html:26
(Diff revision 1)
> +    <script>
> +      onload = function() {
> +        var s = document.querySelector("span");
> +        s.offsetWidth; // flush layout
> +        s.style.left = "200px";
> +        document.documentElement.className = "";

I thought reftests waited after the onload event, is `reftest-wait` really necessary?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8842889 [details]
Bug 1340723 part 1.  Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls.

https://reviewboard.mozilla.org/r/116622/#review118386

::: layout/generic/nsFrame.cpp:10257
(Diff revision 1)
>    return IsFrameScrolledOutOfView(this);
>  }
>  
> +/* virtual */
> +void
> +nsIFrame::UpdateStyleOfOwnedAnonBoxes(ServoStyleSet* aStyleSet,

Also, one last suggestion. I'd love to see this being passed by reference.

I didn't do it initially in `RecreateStyleContexts` because heycam said that the local style in `RestyleManager` was passing it by pointer.

I kind of disagree with that (and I'd love to see more references used), but of course I don't think I have a word there. You do. Anyway, if you want to change it here, that's fine by me, otherwise by pointer is also ok.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Just to make it clear it won't be null?

I can absolutely do that.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

That'd be great, thanks! :)

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8842890 [details]
Bug 1340723 part 2.  Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager.

sr=me with the bit check hoisted into an inline function.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8842891 [details]
Bug 1340723 part 3.  Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.

sr=me, though I'm no expert on {ib} splits.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8842889 [details]
Bug 1340723 part 1.  Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116622/diff/1-2/

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8842890 [details]
Bug 1340723 part 2.  Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116624/diff/1-2/

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8842891 [details]
Bug 1340723 part 3.  Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116626/diff/1-2/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/130e62d89663
part 1.  Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/f6370635b090
part 2.  Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/8335b277da3f
part 3.  Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.  r=emilio

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8842891 [details]
Bug 1340723 part 3.  Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes.

https://reviewboard.mozilla.org/r/116626/#review118306

> nit: no need for `virtual`.

Following file style.

> Also, I guess IB splits aren't ultra-common like, let's say, text frames (though probably not specially rare).

They're actually pretty common.  Not like textframes, of course, but more than a lot of other layout things. :(

> I can't believe we had no tests for this tbh :/.

Yeah, I was pretty sad to see that.  We had some tests that test it incidentally, but they fail in stylo for other reasons.  :(

> I thought reftests waited after the onload event, is `reftest-wait` really necessary?

I basically don't trust any of that stuff, and especially don't trust someone not changing it in the future, so trying to be explicit...

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/130e62d89663
https://hg.mozilla.org/mozilla-central/rev/f6370635b090
https://hg.mozilla.org/mozilla-central/rev/8335b277da3f