1378287 - Skip parent-display-based display type fixup for all anonymous boxes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

There is a per-anonymous-box flag introduced in bug 1358968, and that only skips this fixup for a single kind of anonymous box (:-moz-display-comboboxcontrol-frame).

However, that actually only makes sense for blockification. When I tried to extend that fixup to cover inlinification in bug 1364274, I hit crashes elsewhere for changing display value of anonymous box.

In general, I think it doesn't really make much sense to do fixup on anonymous box. Anonymous box is kind of a second level fixup on top of display type fixup to normal elements. Frame constructor should avoid requiring another display type fixup for anonymous boxes.

I did a quick try push to have AnonBoxSkipsParentDisplayBasedStyleFixup always return true, and it doesn't seem to have any problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cba251076ce46f22795040230e5809ea97713bcb

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

Review commit: https://reviewboard.mozilla.org/r/154368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154368/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

bz, could you review the patch after you back from vacation?

Comment 3

[Boris Zbarsky [:bzbarsky]]

This seems somewhat risky to me.

I agree that in theory anon boxes would never need to get their display value changed.  But I have a hard time proving that.  Have you done some sort of audit to make sure all existing anon boxes set display values in ua sheets and that those display values make sense in all contexts?

Most simply, what happens to an inline-table table wrapper anon box in a blockification situation?  Or does that work because we blockify the table itself and the table wrapper inherits?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8883466 [details]
Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

https://reviewboard.mozilla.org/r/154368/#review161444

r- at least because this makes stylo and gecko have totally different behavior.  If we actually want to do this, we should do it on the gecko side too so we have a hope of catching regressions.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #3)
> This seems somewhat risky to me.
> 
> I agree that in theory anon boxes would never need to get their display
> value changed.  But I have a hard time proving that.  Have you done some
> sort of audit to make sure all existing anon boxes set display values in ua
> sheets and that those display values make sense in all contexts?

I haven't. Just had a brief look at callsites of ResolveInheritingAnonymousBoxStyle in the frame constructor, it seems to me that majority, if not all, of anonymous boxes don't really care about the display type there. The frame constructor just creates a specific type of frame for them or assign them to an existing frame unconditionally. So I suppose there at least shouldn't be any serious issue with this change.

I can do a more thoroughly audit on anonymous boxes and post later.

> Most simply, what happens to an inline-table table wrapper anon box in a
> blockification situation?  Or does that work because we blockify the table
> itself and the table wrapper inherits?

From the frame constructor, it seems table wrapper inherits the display value from its corresponding table element, and the style context of the table element should be correctly blockified before we start resolving table wrapper.

I'm less concerned about blockification, because the algorithm converts any layout internal display type into block directly, which means there shouldn't be many wrapper anonymous box be constructed in the context needing blockification. And anonymous boxes inside elements usually desire blockification less, I suppose.

I'm a bit more concerned about inlinification, actually, because we don't convert layout internal display types currently (which we should!), but we already skip inlinification for all anonymous boxes (except some) in an independent path, so they are not affected here.

(In reply to Boris Zbarsky [:bz] from comment #4)
> r- at least because this makes stylo and gecko have totally different
> behavior.  If we actually want to do this, we should do it on the gecko side
> too so we have a hope of catching regressions.

This changes both Gecko side and Servo side, no? The changes to nsStyleSet is for Gecko. The only thing diverged here as far as I can see is that we are going to apply the skip-fixup flag onto inlinification in Stylo (in bug 1364274), while in Gecko we use a separate code path to exclude pseudos. But they should (mostly) have identical behavior.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm a bit more concerned about inlinification

Gecko doesn't apply inlinification to any anon boxes except text and ruby pseudos.  And the only reason it applies it to those (for which it's a no-op) is so it can set NS_STYLE_SUPPRESS_LINEBREAK on the style context.  so for inlinification we definitely want to skip for all anon boxes, modulo management of that flag (which itself affects inlinification; looks like it's basically recursive, stopping at anon box boundaries when those anon boxes are not ruby.

> The changes to nsStyleSet is for Gecko.

Yes, but that change doesn't affect inlinification in Gecko, right?  I guess I'm not sure what the semantics of the flag are meant to be and why they're different in Gecko and stylo.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > I'm a bit more concerned about inlinification
> 
> Gecko doesn't apply inlinification to any anon boxes except text and ruby
> pseudos.  And the only reason it applies it to those (for which it's a
> no-op) is so it can set NS_STYLE_SUPPRESS_LINEBREAK on the style context. 
> so for inlinification we definitely want to skip for all anon boxes, modulo
> management of that flag (which itself affects inlinification; looks like
> it's basically recursive, stopping at anon box boundaries when those anon
> boxes are not ruby.
> 
> > The changes to nsStyleSet is for Gecko.
> 
> Yes, but that change doesn't affect inlinification in Gecko, right?  I guess
> I'm not sure what the semantics of the flag are meant to be and why they're
> different in Gecko and stylo.

Right, but that makes inlinification work effectively same between Gecko and Stylo in majority of cases, no? In Gecko, we skip all anon boxes for inlinification except text and ruby pseudos, and in Stylo, with this change, we do so as well. (Text anon box inherit the flag directly, and its display type doesn't matter. Ruby pseudos don't need inlinification, and they get the flag they want anyway.)

The only difference as far as I can see, is that some anon boxes may get the SUPPRESS_LINEBREAK flag, but their display is not inlinified in some edge cases in Stylo, but not Gecko. But that shouldn't cause any issue, because that kind of frames doesn't check this flag at all, and their display type means the flag wouldn't be propagated into their children either.

I can also change the behavior of Gecko to be identical to what I'm going to do in bug 1364274 for stylo, if you feel better with that. It would basically just change whether some anon boxes would get the flag.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: list of anonymous boxes and their display in ua sheets

This is a full list of situation of display type of anonymous boxes.

Those have "display !important", I suppose they don't want their display type to ever be changed at all.

And for those which don't have display value specified or don't even have rule in UA sheet, I suppose they don't really care what display type they have.

Those with display value but don't use !important is probably something really affected by this change.

The :-moz-ruby anon boxes should probably just have !important attached. But they shouldn't be affected anyway, because they are not going to be inlinified, and they shouldn't be generated at all when the environment requires blockification.

The :-moz-anonymous-flex/grid-item is really just for blockifying text, so it isn't something we need to care.

The comment in ua.css says the display type doesn't affect the frame construction for :-moz-scrolled-content, so this is probably fine either way. And display type of :-moz-button-content doesn't seem to affect the frame for it either.

:-moz-fieldset-content really seems to be something which should be inlinified, but it is skipped in Gecko for now anyway. (I think we have a bug for this, actually.)

So based on this, I think skipping display type fixup for all the anon boxes shouldn't be making anything much worse than what we currently have.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, so doing the same thing in Gecko is hard, because in bug 1339287, SkipParentDisplayBasedStyleFixup is disabled whenever the parent frame is not a flex/grid container.

That kind of fix isn't going to work with Stylo because we don't know frame type during resolving styles.

I don't know what really should I do then.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

I think we should just take this patch...

Unlike blockification in flex/grid, inlinification in ruby should rarely happen in practice, and it is just for avoid adding complexity to layout engine for handling nonsense error cases in ruby. That said, it is unlikely to hurt webcompat even if we have some edge cases that don't exactly match between Gecko and Stylo.

Also, we already have somehow extensive test coverage for ruby inlinification, and those tests passing with my patches in this bug and bug 1364274 also means we have matched result for majority of the rare cases, so even if we don't have exactly identical behavior, the difference should be too minor that nobody should actually ever hit.

So as far as it wouldn't crash anything, I don't think we should worry about this at all.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Are you happy with this analysis? What concern do you still have?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8883466 [details]
Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154368/diff/1-2/

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8883466 [details]
Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

https://reviewboard.mozilla.org/r/154368/#review162144

Thank you for doing the analysis.  You're right: it should be safe to skip blockification for all anon boxes.

r=me

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8883466 [details]
Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154368/diff/2-3/

Comment 15

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 8 changes to 8 files
remote: (5ebb577315b2 modifies servo/components/style/gecko/regen_atoms.py from Servo; not enforcing peer review)
remote: (5ebb577315b2 modifies servo/ports/geckolib/glue.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: 5ebb577315b2)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Servo PR: servo/servo#17722

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8883466 [details]
Bug 1378287 - Skip parent-display-based display type fixup for all anonymous boxes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154368/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8927961afc7e
Skip parent-display-based display type fixup for all anonymous boxes. r=bz

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8927961afc7e