Closed Bug 1358968 Opened 8 years ago Closed 8 years ago

stylo: ensure nsComboxboxDisplayFrames skip parent display-based style fixups when restyled

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

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

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1358968 - Part 1: Store in nsCSSAnonBoxList.h whether an anon box skips parent display-based style fixups.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1358968 - Part 2: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when resolving anon box style.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1358968 - Part 3: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when restyling anon boxes with ReparentStyleContext.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1358968 - Part 4: Give nsComboboxDisplayFrame a name in frame tree dumps.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
We pass the right CascadeFlags in to Servo_ComputedValues_GetForAnonymousBox, but we don't set the flag when restyling the pseudo.
Comment on attachment 8860929 [details] Bug 1358968 - Part 1: Store in nsCSSAnonBoxList.h whether an anon box skips parent display-based style fixups. https://reviewboard.mozilla.org/r/132932/#review135814 ::: layout/style/nsCSSAnonBoxList.h:24 (Diff revision 1) > + * > * The second argument is the string value of the atom. > * > + * The third argument is whether the anonymous box skips parent-based display > + * fixups (such as blockification inside flex containers). This must be true > + * for CSS_NON_INHERITING_ANON_BOX. Why even have a third arg for CSS_NON_INHERITING_ANON_BOX? You can change this line: # define CSS_NON_INHERITING_ANON_BOX CSS_ANON_BOX to # define CSS_NON_INHERITING_ANON_BOX(ident, atom) CSS_ANON_BOX(ident, atom, true) and things should Just Work, I would think, without the need for the static assert. Or if we want: # define CSS_NON_INHERITING_ANON_BOX(ident, atom) CSS_ANON_BOX(ident, atom, ) so it won't even compile if some consumer uses that third arg and doesn't explicitly define CSS_NON_INHERITING_ANON_BOX as "nothing", as your patch does. ::: layout/style/nsCSSAnonBoxes.h:62 (Diff revision 1) > +#undef CSS_NON_INHERITING_ANON_BOX > +#undef CSS_ANON_BOX > + false; > + } > + > + static bool AnonBoxSkipsParentDisplayBasedStyleFixup(nsIAtom* aPseudo) Needs a comment explaining what it does and the preconditions. For example, why _not_ allow passing non-inherited anon boxes? That's entirely unclear.
Attachment #8860929 - Flags: review?(bzbarsky)
Comment on attachment 8860930 [details] Bug 1358968 - Part 2: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when resolving anon box style. https://reviewboard.mozilla.org/r/132934/#review135816 r=me ::: layout/style/ServoStyleSet.cpp:455 (Diff revision 1) > - uint32_t aFlags) > { > MOZ_ASSERT(nsCSSAnonBoxes::IsAnonBox(aPseudoTag) && > !nsCSSAnonBoxes::IsNonInheritingAnonBox(aPseudoTag)); > > - MOZ_ASSERT(aFlags == 0 || > + bool skipFixup = The linear walk here is not great, but unless we switch everyone to anon box ids like we have for pseudo-elements, I'm not sure we can do better. :( And I guess since in that linear walk all the booleans are known at compile time we can assume the compiler will in fact reduce it to a linear walk over just the atoms that correspond to anon boxes that have the boolean set to "true"? Is that why we excluded non-inheriting anon boxes from the function to start with? ::: layout/style/ServoStyleSet.cpp:477 (Diff revision 1) > > - // FIXME(bz, bug 1344914) We should really GetContext here and make skipFixup > + // FIXME(bz, bug 1344914) We should really GetContext here. > - // work there. > return NS_NewStyleContext(aParentContext, mPresContext, aPseudoTag, > CSSPseudoElementType::InheritingAnonBox, > - computedValues.forget(), skipFixup); > + computedValues.forget(), false); Why is this passing `false` instead of `skipFixup`? I guess the point is that the value is unused by NS_NewStyleContext in the servo case anyway? If so, document that, and file a followup to remove the value from the servo overloads of NS_NewStyleContext and the nsStyleContext ctor. And probably from nsStyleContext::FinishConstruction and instead do the call to ApplyStyleFixups from the non-servo nsStyleContext ctor directly, right? Or you could do that in an additional changeset here; either way.
Attachment #8860930 - Flags: review?(bzbarsky) → review+
Comment on attachment 8860931 [details] Bug 1358968 - Part 3: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when restyling anon boxes with ReparentStyleContext. https://reviewboard.mozilla.org/r/132936/#review135818 r=me
Attachment #8860931 - Flags: review?(bzbarsky) → review+
Comment on attachment 8860932 [details] Bug 1358968 - Part 4: Give nsComboboxDisplayFrame a name in frame tree dumps. https://reviewboard.mozilla.org/r/132938/#review135820 r=me
Attachment #8860932 - Flags: review?(bzbarsky) → review+
Comment on attachment 8860933 [details] style: Update atom generation script for nsCSSAnonBoxList.h changes. https://reviewboard.mozilla.org/r/132940/#review135826
Attachment #8860933 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860934 [details] style: Regenerate atoms and pseudo-elements. https://reviewboard.mozilla.org/r/132942/#review135828 This will need to be squashed with the next one I guess.
Attachment #8860934 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860935 [details] style: Store in Gecko PseudoElements whether they skip parent display-based style fixups. https://reviewboard.mozilla.org/r/132944/#review135830 ::: servo/components/style/gecko/selector_parser.rs:44 (Diff revision 1) > /// pseudos. > /// > /// Also, we can further optimize PartialEq and hash comparing/hashing only the > /// atoms. > #[derive(Clone, Debug, PartialEq, Eq, Hash)] > -pub struct PseudoElement(Atom, bool); > +pub struct PseudoElement(Atom, bool, bool); Could we name the fields? ::: servo/ports/geckolib/glue.rs:456 (Diff revision 1) > None > } else { > let atom = Atom::from(pseudo_tag); > - Some(PseudoElement::from_atom_unchecked(atom, /* anon_box = */ false)) > + Some(PseudoElement::from_atom_unchecked(atom, > + /* anon_box = */ false, > + /* skip_display_fixup = */ false)) Or perhaps make that second field a bitfield or something, so this is a bit more legible. Not a huge deal though.
Attachment #8860935 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860936 [details] style: Skip parent display-based style fixups when restyling anon boxes. https://reviewboard.mozilla.org/r/132946/#review135834 Are we even restyling these right now? I believe that was the point of bug 1331047 :). Anyway, this looks fine, though if it could wait a few days, I'd really appreciate it. ::: servo/components/style/matching.rs:389 (Diff revision 1) > fn cascade_with_rules(&self, > shared_context: &SharedStyleContext, > font_metrics_provider: &FontMetricsProvider, > rule_node: &StrongRuleNode, > primary_style: &ComputedStyle, > - is_pseudo: bool) > + pseudo: Option<&PseudoElement>) I'm killing all this and adding a `implemented_pseudo_element` in bug 1331047, which would make this refactoring unnecessary. It also reworks pretty much all this code, so if this last patch could wait until bug 1331047 lands it'd be great, because it'd become a one-liner condition in `skip_root_and_item_based_display_fixup` instead, and it saves me the rebase pain :) If this is urgent for some reason, feel free to land this in the meantime, I'll rebase.
Attachment #8860936 - Flags: review?(emilio+bugs) → review+
Well, I guess this one is a frame-backed pseudo. Still I'm not sure we're restyling them right now.
I guess part 2 means we could also fail to restyle them in Gecko, any chance to add a test for this?
> Still I'm not sure we're restyling them right now. We are for some but not others. https://public.etherpad-mozilla.org/p/anon-box-stylo tracks the progress. Combobox display is one of the ones we do reresolve; see bug 1347411.
Priority: -- → P1
Assignee: nobody → cam
Comment on attachment 8860929 [details] Bug 1358968 - Part 1: Store in nsCSSAnonBoxList.h whether an anon box skips parent display-based style fixups. https://reviewboard.mozilla.org/r/132932/#review135814 > Why even have a third arg for CSS_NON_INHERITING_ANON_BOX? > > You can change this line: > > # define CSS_NON_INHERITING_ANON_BOX CSS_ANON_BOX > > to > > # define CSS_NON_INHERITING_ANON_BOX(ident, atom) CSS_ANON_BOX(ident, atom, true) > > and things should Just Work, I would think, without the need for the static assert. Or if we want: > > # define CSS_NON_INHERITING_ANON_BOX(ident, atom) CSS_ANON_BOX(ident, atom, ) > > so it won't even compile if some consumer uses that third arg and doesn't explicitly define CSS_NON_INHERITING_ANON_BOX as "nothing", as your patch does. I was just trying to avoid changing servo/components/style/binding_tools/regen_atoms.py further, but it's probably not a big deal to do that, so I will.
Comment on attachment 8860930 [details] Bug 1358968 - Part 2: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when resolving anon box style. https://reviewboard.mozilla.org/r/132934/#review135816 > The linear walk here is not great, but unless we switch everyone to anon box ids like we have for pseudo-elements, I'm not sure we can do better. :( > > And I guess since in that linear walk all the booleans are known at compile time we can assume the compiler will in fact reduce it to a linear walk over just the atoms that correspond to anon boxes that have the boolean set to "true"? Is that why we excluded non-inheriting anon boxes from the function to start with? Yes, I'm relying on the compiler optimizing out those (false && ...) checks. It's not why we're making non-inheriting anonymous boxes skip the fixup in the first place -- which I am assuming (and to which effect I'll add a comment in part 1) is because it is not that useful given we don't have any inherited styles -- but it is why I'm skipping checking them in this function. We still need to do the linear check of "do we have an inheriting anon box" in ReparentStyleContext, though. > Why is this passing `false` instead of `skipFixup`? > > I guess the point is that the value is unused by NS_NewStyleContext in the servo case anyway? If so, document that, and file a followup to remove the value from the servo overloads of NS_NewStyleContext and the nsStyleContext ctor. And probably from nsStyleContext::FinishConstruction and instead do the call to ApplyStyleFixups from the non-servo nsStyleContext ctor directly, right? > > Or you could do that in an additional changeset here; either way. Yes. I'll add a comment and do those other cleanups.
Comment on attachment 8860929 [details] Bug 1358968 - Part 1: Store in nsCSSAnonBoxList.h whether an anon box skips parent display-based style fixups. https://reviewboard.mozilla.org/r/132932/#review136334 r=me
Attachment #8860929 - Flags: review?(bzbarsky) → review+
(needinfo myself to rebase and land this once bug 1331047 has landed.)
Depends on: 1331047
Flags: needinfo?(cam)
Attachment #8860933 - Attachment is obsolete: true
Attachment #8860934 - Attachment is obsolete: true
Attachment #8860935 - Attachment is obsolete: true
Attachment #8860936 - Attachment is obsolete: true
I believe that the display fixup patches here are not right btw (well, they're not wrong, but they're just dead code). We should never arrive at the traversal code with an anon-box pseudo-element. If there's any element-backed pseudo that should skip fixup then that needs to look at `self.implemented_pseudo_element()` instead. Otherwise we should just be doing/skipping the fixup in Servo_ComputedValues_GetForAnonymousBox.
Depends on: 1360508
Flags: needinfo?(cam)
Looks like these patches can be used as is, and we can just drop the Servo-side changes.
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6accb58e9ff4 Part 1: Store in nsCSSAnonBoxList.h whether an anon box skips parent display-based style fixups. r=bz https://hg.mozilla.org/integration/autoland/rev/746178a442c4 Part 2: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when resolving anon box style. r=bz https://hg.mozilla.org/integration/autoland/rev/193716dbcabb Part 3: Use nsCSSAnonBoxList.h data to skip parent display-based style fixups when restyling anon boxes with ReparentStyleContext. r=bz https://hg.mozilla.org/integration/autoland/rev/dbf029237e45 Part 4: Give nsComboboxDisplayFrame a name in frame tree dumps. r=bz
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1cfa322bb1f2 Static analysis fixup. r=me (CLOSED TREE)
Blocks: 1367210
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: