Closed Bug 1344914 Opened 3 years ago Closed 3 years ago

stylo: Why does ServoStyleSet::ResolveAnonymousBoxStyle not call GetContext?

Categories

(Core :: CSS Parsing and Computation, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

Details

Attachments

(1 file)

It's directly calling NS_NewStyleContext, which skips some work GetContext does...

Presumably this has to do with the broken fixup handling in GetContext; if so we might want to just fix that.
Flags: needinfo?(cam)
I don't know if it was trying to avoid calling into broken fixup handling deliberately.  The fixup skipping stuff in nsStyleContext's constructor doesn't actually have any effect in stylo -- anything like that should be handled on the Servo side, to influence the ServoComputedValues object we get before sticking it in an nsStyleContext.  So probably we could just call GetContext here.
Flags: needinfo?(cam)
> anything like that should be handled on the Servo side, to influence the
> ServoComputedValues object we get before sticking it in an nsStyleContext

Do we actually do that in this case?  I don't see a mechanism for it....
No, I don't think we do.  So I guess it's likely some form controls etc. are incorrectly blockifying their anonymous box children when display:grid/flex are specified on them.
Sorry, that's completely wrong.  We do skip fixups for NAC roots, using the TElement::skip_root_and_item_based_display_fixup function to check whether to do it, rather than relying on the flags that are passed in.
> We do skip fixups for NAC roots

We actually skip it for all NAC, looks like.  That doesn't seem right to me.  We should be skipping it for NAC roots, and pseudo-elements, but probably not other NAC...
Flags: needinfo?(bobbyholley)
I'll defer to heycam on questions surrounding style fixup, which I don't know much about. However, I generally agree that this work should happen during the servo cacade and shouldn't live on nsStyleContext.
Flags: needinfo?(bobbyholley) → needinfo?(cam)
Priority: -- → P5
Flags: needinfo?(cam)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> We actually skip it for all NAC, looks like.  That doesn't seem right to me.
> We should be skipping it for NAC roots, and pseudo-elements, but probably
> not other NAC...

I split that issue out into bug 1359303.
Comment on attachment 8861286 [details]
Bug 1344914 - stylo: Stop pretending to handle style fixups on the C++ side.

https://reviewboard.mozilla.org/r/133246/#review136338

r=me with the nits below fixed.

::: layout/style/nsStyleContext.cpp:130
(Diff revision 1)
>      PresContext()->PresShell()->StyleSet()->RootStyleContextAdded();
>    }
>  
>    mSource.AsGeckoRuleNode()->SetUsedDirectly(); // before ApplyStyleFixups()!
> -  FinishConstruction(aSkipParentDisplayBasedStyleFixup);
> +  FinishConstruction();
> +  ApplyStyleFixups(aSkipParentDisplayBasedStyleFixup);

Header definition of ApplyStyleFixups should document that it's only called for Gecko style contexts.

And the impl should assert that.
Attachment #8861286 - Flags: review?(bzbarsky) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ec24df56cce
stylo: Stop pretending to handle style fixups on the C++ side. r=bz
https://hg.mozilla.org/mozilla-central/rev/0ec24df56cce
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → cam
You need to log in before you can comment on or make changes to this bug.