Closed Bug 1364871 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

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

Tracking

()

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

People

(Reporter: xidorn, Assigned: emilio)

References

Details

Attachments

(4 files)

Bug 1364871: Factor out some code from UpdateStyleOfChildAnonBox into UpdateStyleOfOwnedChildFrame.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364871: Add a function to update frame pseudo-element styles during the post-traversal, and restyle bullet frames.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364871: Restyle ::backdrop too.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364871: Revert workaround added in bug 1328319.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
::-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.
Flags: needinfo?(emilio+bugs)
Blocks: 1364873
Assignee: nobody → emilio+bugs
I'm not familiar with stuff in ServoRestyleManager.cpp, so it is probably better to ask bz or heycam to review this.
Flags: needinfo?(emilio+bugs)
Attachment #8869607 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8869608 - Flags: review?(xidorn+moz) → review?(cam)
Comment on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

Fair enough, thanks for letting me know Xidorn! :)
Attachment #8869609 - Flags: review?(xidorn+moz) → review?(cam)
Could you also revert the workaround added in bug 1328319 part 9, and see if it stops crashing?
Blocks: 1366427
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.
Also note that majority of tests of ::backdrop are in fullscreen tests in dom/html/test/test_fullscreen-api.html
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/
Attachment #8869607 - Flags: review?(cam) → review+
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
Attachment #8869608 - Flags: review?(cam) → review+
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 on attachment 8869655 [details]
Bug 1364871: Revert workaround added in bug 1328319.

https://reviewboard.mozilla.org/r/141242/#review144830
Attachment #8869655 - Flags: review?(cam) → review+
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 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 on attachment 8869609 [details]
Bug 1364871: Restyle ::backdrop too.

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

OK, great. :-)
Attachment #8869609 - Flags: review?(cam) → review+
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".
I mean, absolutely-positioned :)
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
I moved the backdrop bits to another function, and added comments there re. the placeholder style.
Pushed by ecoal95@gmail.com:
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
Blocks: 1363590
Depends on: 1374752
Depends on: 1375315
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: