Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: Restyle additional style contexts

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
2 months ago
4 hours ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 months ago
Looking over ::-moz-math-anonymous and similar stuff, it seems they're restyled using the {Get,Set}AdditionalStyleContext API.

We should presumably restyle those.
I think this goes with bz's anonymous box restyles stuff.
Assignee: nobody → bzbarsky
Priority: -- → P2
Once this is fixed, we should try the test case from bug 1376406 to ensure that its Servo-side fix in bug 1379865 had the desired effect.
Blocks: 1382222
(Assignee)

Comment 3

a day ago
We've received already two reports of this, and this isn't blocked on anything particular afaict.

Boris, should I just take this?
Flags: needinfo?(bzbarsky)
If you have the bandwidth for it, sure!
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

a day ago
Assignee: bzbarsky → emilio+bugs
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Might be worth asserting that anon boxes don't have additional style contexts.
(Assignee)

Comment 8

a day ago
(In reply to Boris Zbarsky [:bz] from comment #7)
> Might be worth asserting that anon boxes don't have additional style
> contexts.

Yeah, in the second commit message I mention it... I guess I'll do that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

23 hours ago
Duplicate of this bug: 1371030

Comment 12

10 hours ago
mozreview-review
Comment on attachment 8888390 [details]
Bug 1366721: Switch all the APIs in ServoStyleSet to use ServoStyleContext.

https://reviewboard.mozilla.org/r/159340/#review165092

::: layout/style/ServoStyleSet.h:133
(Diff revision 1)
> -  already_AddRefed<nsStyleContext>
> +  already_AddRefed<ServoStyleContext>
>    ResolveStyleFor(dom::Element* aElement,
>                    ServoStyleContext* aParentContext,
>                    LazyComputeBehavior aMayCompute);

Hooray for already_AddRef<BaseClass>'s ability to autoconvert from already_AddRef<DerivedClass>, in StyleSetHandle::Ptr::ResolveStyleFor. :-)

::: layout/style/ServoStyleSet.h:364
(Diff revision 1)
>  
>    /**
>     * Resolve style for the given element, and return it as a
> -   * ServoComputedValues, not an nsStyleContext.
> +   * ServoStyleContext.
> +   *
> +   * FIXME(emilio): Is there a point in this after bug 1367904?

Probably not!
Attachment #8888390 - Flags: review?(cam) → review+

Comment 13

9 hours ago
mozreview-review
Comment on attachment 8888391 [details]
Bug 1366721: Restyle additional style contexts in ServoRestyleManager.

https://reviewboard.mozilla.org/r/159342/#review165096

r=me with the appropriate thing done for anonymous boxes.

::: commit-message-b9cca:3
(Diff revision 2)
> +I was about to assert that other non-primary frames don't have additional style
> +contexts everywhere, but that wouldn't make much sense, given they don't
> +correspond to an element we could selector-match against.

Couldn't they in theory use the same element as the primary frame?  Though I don't think we have any cases of non-primary frames that use additional style contexts.  You could assert that somewhere under UpdateStyleOfOwnedAnonBoxes if you want.

::: layout/base/ServoRestyleManager.cpp:436
(Diff revision 2)
> +  RefPtr<ServoStyleContext> newContext =
> +    aRestyleState.StyleSet().ResolvePseudoElementStyle(

What about if the old style context was for an anonymous box?  Or does that not happen for the current GetAdditionalStyleContext implementations?  If so, maybe assert it up the top of the function.

(I notice that GeckoRestyleManager doesn't re-selector match (non-XUL) additional style contexts, but that doesn't seem to be an important optimization.)

::: layout/base/ServoRestyleManager.cpp:635
(Diff revision 2)
>        MOZ_ASSERT(!styleFrame);
>        displayContentsNode->mStyle = newContext;
>      }
>  
>      if (styleFrame) {
> +      UpdateAdditionalStyleContexts(styleFrame, aRestyleState);

What about continuations?  Maybe it doesn't matter in practice, since all of the frames that have additional style contexts can never be split?  Maybe assert this in UpdateAdditionalStyleContexts?
Attachment #8888391 - Flags: review?(cam) → review+

Comment 14

9 hours ago
mozreview-review
Comment on attachment 8888496 [details]
Bug 1366721: Update reftest expectations.

https://reviewboard.mozilla.org/r/159462/#review165102

Might be worth adding a reftest based on the button test case from bug 1382222?
Attachment #8888496 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

4 hours ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6ce98d80d882
Switch all the APIs in ServoStyleSet to use ServoStyleContext. r=heycam
https://hg.mozilla.org/integration/autoland/rev/346f57883936
Restyle additional style contexts in ServoRestyleManager. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e7c0f783e4fc
Update reftest expectations. r=heycam
You need to log in before you can comment on or make changes to this bug.