Closed Bug 1340723 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files)

Attached file Testcase
Because we don't restyle the anon block anonymous box.
Blocks: 1337696
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
Attachment #8842889 - Flags: review?(emilio+bugs) → review+
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
Attachment #8842890 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8842891 - Flags: review?(emilio+bugs) → review+
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.
Just to make it clear it won't be null?

I can absolutely do that.
That'd be great, thanks! :)
Attachment #8842889 - Flags: superreview+
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.
Attachment #8842890 - Flags: superreview+
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.
Attachment #8842891 - Flags: superreview+
Assignee: nobody → bzbarsky
Pushed by bzbarsky@mozilla.com:
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 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...
You need to log in before you can comment on or make changes to this bug.