1364871 - stylo: Restyle ::-moz-list-bullet and ::-moz-list-number pseudo-elements (for nsBulletFrame)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

::-moz-list-bullet and ::-moz-list-number are for styling nsBulletFrame.

This is particular important because nsBulletFrame uses counter style, and counter style objects have manual lifetime management, and the objects are supposed to be released after style flush.

For not blocking @counter-style, we are going to leak the counter style objects for now. But we definitely want to fix it before we ship stylo.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364871: Restyle ::backdrop too.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I'm not familiar with stuff in ServoRestyleManager.cpp, so it is probably better to ask bz or heycam to review this.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

Fair enough, thanks for letting me know Xidorn! :)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Could you also revert the workaround added in bug 1328319 part 9, and see if it stops crashing?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This try run is with the patches from bug 1366144 too, without the revert: https://treeherder.mozilla.org/#/jobs?repo=try&revision=490592187662557e393a677c2b159907592d4aa0

This is with the revert: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48e8787259825ffb2e2be8e8b9dfcb123e5eebcc

Also, I was surprised about why there are no ::backdrop tests passing and all that, but I noticed that what we do for ::backdrop is completely wrong, so even though I think it's probably worth landing the commits, I opened bug 1366427 to fix that.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364871: Revert workaround added in bug 1328319.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869607 [details]
Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869608 [details]
Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869607 [details]
Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869608 [details]
Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869655 [details]
Bug 1364871: Revert workaround added in bug 1328319.

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

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Also note that majority of tests of ::backdrop are in fullscreen tests in dom/html/test/test_fullscreen-api.html

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8869607 [details]
Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.

https://reviewboard.mozilla.org/r/141196/#review144824

::: layout/generic/nsIFrame.h:3270
(Diff revision 3)
> +  // A helper both for UpdateStyleOfChildAnonBox, and to update frame-backed
> +  // pseudo-elements in ServoRestyleManager.
> +  //
> +  // This gets a style context that will be the new style context for
> +  // `aChildFrame`, and takes care of updating it, calling CalcStyleDifference,
> +  // and add to the change list as appropriate.

s/add/adding/

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8869608 [details]
Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.

https://reviewboard.mozilla.org/r/141198/#review144826

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144828

::: layout/base/ServoRestyleManager.cpp:244
(Diff revision 3)
> +      MOZ_ASSERT(backdropPlaceholder->IsPlaceholderFrame());
> +      nsIFrame* backdropFrame =
> +        nsPlaceholderFrame::GetRealFrameForPlaceholder(backdropPlaceholder);
> +      MOZ_ASSERT(backdropFrame->IsBackdropFrame());
> +      MOZ_ASSERT(backdropFrame->StyleContext()->GetPseudoType() ==
> +                 CSSPseudoElementType::backdrop);

The UA style sheet rule that makes the ::backdrop frame position:fixed isn't !important, so content can override it back to position:static, in which case you won't have a placeholder frame, I think.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8869655 [details]
Bug 1364871: Revert workaround added in bug 1328319.

https://reviewboard.mozilla.org/r/141242/#review144830

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144828

> The UA style sheet rule that makes the ::backdrop frame position:fixed isn't !important, so content can override it back to position:static, in which case you won't have a placeholder frame, I think.

You'll always have a placeholder frame. `position` of an element in top layer (including `::backdrop` pseudo-element) must either be `fixed` or `absolute`, and any other value needs to be computed to `absolute`.

This logic can be found here: https://dxr.mozilla.org/mozilla-central/rev/8d60d0f825110cfb646ac31dc16dc011708bcf34/layout/style/nsRuleNode.cpp#6107-6116

I guess we probably haven't implemented it for Stylo? That would be a problem... And I suspect we don't have test coverage for this, which is definitely my fault :/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144828

> You'll always have a placeholder frame. `position` of an element in top layer (including `::backdrop` pseudo-element) must either be `fixed` or `absolute`, and any other value needs to be computed to `absolute`.
> 
> This logic can be found here: https://dxr.mozilla.org/mozilla-central/rev/8d60d0f825110cfb646ac31dc16dc011708bcf34/layout/style/nsRuleNode.cpp#6107-6116
> 
> I guess we probably haven't implemented it for Stylo? That would be a problem... And I suspect we don't have test coverage for this, which is definitely my fault :/

Yes, we do implement that: http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/servo/components/style/style_adjuster.rs#36

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144836

OK, great. :-)

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144828

> Yes, we do implement that: http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/servo/components/style/style_adjuster.rs#36

Oh, that's great.

I don't really like the name `out_of_flow_positioned`, though... Floated elements are out-of-flow as well, but it seems they are not included here... In Gecko, `absolute` and `fixed` are called "absoluted-positioned".

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

I mean, absolutely-positioned :)

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

https://reviewboard.mozilla.org/r/141200/#review144828

> Oh, that's great.
> 
> I don't really like the name `out_of_flow_positioned`, though... Floated elements are out-of-flow as well, but it seems they are not included here... In Gecko, `absolute` and `fixed` are called "absoluted-positioned".

I don't think that's particularly clearer personally :).

I like the distinction of `out_of_flow_positioned` and `floated`. If we needed it (for style adjustment I think we don't), we could have a `floated_or_out_of_flow_positioned` or just `out_of_flow`, I guess.

Anyway, happy to bikeshed about the name if you want :P

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869607 [details]
Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141196/diff/3-4/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869608 [details]
Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141198/diff/3-4/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141200/diff/3-4/

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8869655 [details]
Bug 1364871: Revert workaround added in bug 1328319.

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

Comment 31

[Emilio Cobos Álvarez (:emilio)]

I moved the backdrop bits to another function, and added comments there re. the placeholder style.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8036f4516e0c
Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame. r=heycam
https://hg.mozilla.org/integration/autoland/rev/09851f5d647e
Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ca5ce34ad1cf
Restyle ::backdrop too. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9885206e1a48
Revert workaround added in bug 1328319. r=heycam

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52caa4cf1618
Update expectations. r=me

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9885206e1a48

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8036f4516e0c
https://hg.mozilla.org/mozilla-central/rev/09851f5d647e
https://hg.mozilla.org/mozilla-central/rev/ca5ce34ad1cf
https://hg.mozilla.org/mozilla-central/rev/52caa4cf1618