Closed
Bug 1366721
Opened 7 years ago
Closed 7 years ago
stylo: Restyle additional style contexts
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(3 files)
Looking over ::-moz-math-anonymous and similar stuff, it seems they're restyled using the {Get,Set}AdditionalStyleContext API. We should presumably restyle those.
Comment 1•7 years ago
|
||
I think this goes with bz's anonymous box restyles stuff.
Assignee: nobody → bzbarsky
Priority: -- → P2
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years 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)
Assignee | ||
Updated•7 years ago
|
Assignee: bzbarsky → emilio+bugs
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
Might be worth asserting that anon boxes don't have additional style contexts.
Assignee | ||
Comment 8•7 years 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) |
Comment 12•7 years 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•7 years 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•7 years 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•7 years 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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ce98d80d882 https://hg.mozilla.org/mozilla-central/rev/346f57883936 https://hg.mozilla.org/mozilla-central/rev/e7c0f783e4fc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 20•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #2) > 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. Just checked, and that test still works.
You need to log in
before you can comment on or make changes to this bug.
Description
•