1231682 - Support "-webkit-box-pack: justify"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

In bug 1208635, we added support for "-webkit-box-pack" as an alias for "justify-content".

This works for almost all -webkit-box-pack values, except for one -- "justify".

Filing this bug on adding support for parsing that -webkit-box-pack value.

Comment 1

[Daniel Holbert [:dholbert]]

Per discussion with dbaron yesterday: I think we can handle this by adding a member var in CSSParserImpl to represent the property-alias that's currently being parsed, and then we can vary our parsing behavior as-needed depending on the value of that variable.

In this case, the behavior-difference is that we'd want to use kBoxPackKTable as our keyword-table, and then map from those keywords' numeric representations to the equivalent justify-content numeric representations/.  And in particular, from NS_STYLE_BOX_PACK_JUSTIFY to NS_STYLE_ALIGN_SPACE_BETWEEN.

(We may need to add some extra handling to convert back to "justify" in elem.style / window.getComputedStyle APIs for the webkit-prefixed property, too; I'm not as sure about how the alias-mapping works on that end.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1231682 part 1: When nsCSSParser is parsing a property-alias, keep track of the alias in a member-var. r=heycam

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1231682 part 2: Support "-webkit-box-pack: justify" as a special-case exception during "justify-content" parsing. r=mats

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

Comment 4

[Daniel Holbert [:dholbert]]

So basically:
 * Part 1 changes the LookupEnabledProperty() API (and the nsCSSProps::LookupProperty() API that it it invokes) to return the property-alias via an outparam.  It also changes all of the LookupEnabledProperty() clients to save this alias temporarily in a member-var, so that it's available to us while we're parsing.

 * Part 2 checks whether we're parsing "-webkit-box-pack" and if so, makes one extra attempt at parsing (with a keyword table that maps "justify" to our enum representation of "space-between").  It also enables pieces of some reftests that have been disabled due to the lack of "-webkit-box-pack:justify" support, and it adds a new reftest to verify that @supports correctly reflects our support for this keyword.

Comment 5

[Mats Palmgren (inactive)]

Hmm, I tested "-webkit-box-pack: justify" in Chrome and it seems it's NOT supported there,
at least not for display:grid and display:flex.  It seems to be supported only for
display:-webkit-box, AFAICT.

So, which browsers support this property and on which display types exactly?

I feel pretty strongly that we should NOT introduce support for this property on display
types where web compat problems do not exist, e.g. for display:grid which no browser have
even shipped yet.

At first glance, it appears that these patches introduce support for "-webkit-box-pack:
justify" everywhere we support 'justify-content', is that correct?

Comment 6

[Mats Palmgren (inactive)]

Conversely, why do we support:
  display: -webkit-box;
  justify-content: flex-end;
when Chrome doesn't?

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)
> Hmm, I tested "-webkit-box-pack: justify" in Chrome and it seems it's NOT
> supported there,
> at least not for display:grid and display:flex.  It seems to be supported
> only for
> display:-webkit-box, AFAICT.

Correct. Chrome has an independent implementation of "display: -webkit-box" vs "display: flex"; we do not.

Our strategy for supporting "display: -webkit-box" & associated properties is to simply treat them as aliases for their modern flexbox equivalents.  This almost entirely "just works" (including e.g. treating -webkit-box-flex as an alias for the "flex" shorthand).

As you note, it does mean that we'll accept/honor some CSS that Chrome would reject -- for example, we'll right-align stuff inside of an element with "display: flex; -webkit-box-pack: end", whereas Chrome will disregard the -webkit-box-pack there.

That hasn't been an actual problem that we've observed in the wild, and the (low) risk of honoring code like that is an acceptable tradeoff for the benefit of cheap -webkit-box support.

> So, which browsers support this property and on which display types exactly?

Every other browser supports this property, on "display:-webkit-box" and "display: -webkit-inline-box".

We support it on "display: -webkit-box" and "display:flex" variants.

> I feel pretty strongly that we should NOT introduce support for this
> property on display
> types where web compat problems do not exist, e.g. for display:grid which no
> browser have
> even shipped yet.

That ship has already sailed, to some extent -- see comment 0.  As of bug 1208635, we simply support -webkit-box-pack as an alias for justify-content.

> At first glance, it appears that these patches introduce support for
> "-webkit-box-pack: justify" everywhere we support 'justify-content', is that correct?

Yes.  (More specifically: these patches extend our already-existing "-webkit-box-pack is an alias of justify-content" mapping to allow for one bonus extra value, "justify", which gets mapped to the correct justify-content value when parsed.)

(In reply to Mats Palmgren (:mats) from comment #6)
> Conversely, why do we support:
>   display: -webkit-box;
>   justify-content: flex-end;
> when Chrome doesn't?

Again, because we treat -webkit-box as an alias for flex. Chrome does not.

Comment 8

[Daniel Holbert [:dholbert]]

Note that we are not aiming for 100% correctness (i.e. matching Chrome) across every possible usage of -webkit-box & related properties.  Our aliasing strategy for -webkit-box is about the 99% usecases -- a simple horizontal or vertical box with maybe a few flexible elements in it, and maybe some alignment.

See screenshots in http://www.otsukare.info/2016/01/04/webkit-resolved-fixed for what sorts of wins this simple aliasing buys us on the mobile web.

There *is* a possibility that some pathological cases may produce undesirable behavior -- particularly if authors combine legacy css with modern CSS & express conflicting intentions with the two. (That  seems to be what you're concerned about.)  That hasn't been a problem that we've run into so far, though; and I think it's unlikely to be common. (And the compat wins that it buys us are likely worth it.)

If you have any ideas for an alternative way to support the "-webkit-box" family of properties without aliasing & without too much work, I'd be open to hearing it, though...

Comment 9

[Mats Palmgren (inactive)]

> Again, because we treat -webkit-box as an alias for flex. Chrome does not.

Well, I think we're clearly going down the wrong path here.
Supporting align-/justify-* properties on "display:-webkit-box" and
supporting -webkit-box-* properties on display:grid/flex creates a huge
web compat risk, simply because other browsers do not support those
combinations.

What we should do is implement '-webkit-box' as a separate value on
'display' and then support the -webkit-box-* properties only there.
I think we can still implement layout using nsFlexContainerFrame.

It might be possible to map '-webkit-box' to 'flex' in ApplyStyleFixups
and record the original value in mOriginalDisplay and then use different
properties in layout based on that?

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> If you have any ideas for an alternative way to support the "-webkit-box"
> family of properties without aliasing & without too much work, I'd be open
> to hearing it, though...

I'm not familiar with -webkit-box-* properties in general, but if they are
straightforward enumerations, like it seems -webkit-box-pack is, then I think
we should just add them as new separate properties.  Simple properties like
that is boilerplate code that doesn't take much time to implement -- relative
implementing the layout for them.  But we already have layout, so it's just
a matter of using the right property values based on 'display'.  That seems
simpler than the patches in this bug.

I'm thinking something like this, in layout:
GetJustifyContent() {
  if (StyleDisplay()->mOriginalDisplay == WEBKIT_BOX)
    return StylePosition()->mWebkitBoxPack;
  else
    return StylePosition()->ComputedJustifyContent();
}

We just have to make sure we use the correct style values for mWebkitBoxPack
so that the current layout code just works.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #9)
> Supporting align-/justify-* properties on "display:-webkit-box" and
> supporting -webkit-box-* properties on display:grid/flex creates a huge
> web compat risk, simply because other browsers do not support those
> combinations.

I'm skeptical that this is an actual risk in the real world, though it does seem conceivable that we could run into something like this.

> What we should do is implement '-webkit-box' as a separate value on
> 'display'

(Incidentally, I'm about to do this in bug 1236400, since display:-webkit-box needs to have different inline-child-wrapping rules from display:flex.)

> and then support the -webkit-box-* properties only there.
> I think we can still implement layout using nsFlexContainerFrame.

True, this could work (via stuff like your GetJustifyContent() function in comment 10).

> It might be possible to map '-webkit-box' to 'flex' in ApplyStyleFixups
> and record the original value in mOriginalDisplay and then use different
> properties in layout based on that?

We probably don't want to rely on ApplyStyleFixups for something like this, since it breaks style-context sharing and hence has a memory penalty. (And some sites likely do silly things like "* { display: -webkit-box }")

=========

How about this:
 - I'm already going to be introducing code to accept "-webkit-box" as an actual computed display value (which maps to nsFlexContainerFrame), over in bug 1236400.
 - I'll file a bug to attack immediately after that one, to look into un-aliasing these -webkit-box-* alignment properties and making our flexbox code use them in the way that you suggest in comment 10.
 - In the meantime, I'd still lean towards taking this bug's patches (as a relatively small step down this aliasing path that we're already pretty far down), since they get us better emulation (adding just one more keyword-value), and they get us closer to being able to ship our unprefixing/aliasing behavior past Aurora which will get us huge mobile webcompat wins.  

Note that the more-complex part of this bug (part 1, for "what alias are we parsing right now") is something that we kinda need elsewhere, too, for bug 1231829.  It's not entirely in the service of this one special case.  So really, the only piece of this bug that's related to this aliasing that you frown upon is the 3-line code change in "part 2".

What do you think?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> So really, the only piece of this bug that's related to this aliasing that you
> frown upon is the 3-line code change in "part 2".

(er, the 3 lines in nsCSSParser.cpp plus the new property-table in nsCSSProps, I suppose. So a bit more than 3 lines, but not much.)

Comment 13

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> How about this:
>  - I'm already going to be introducing code to accept "-webkit-box" as an
> actual computed display value (which maps to nsFlexContainerFrame), over in
> bug 1236400.
>  - I'll file a bug to attack immediately after that one, to look into
> un-aliasing these -webkit-box-* alignment properties and making our flexbox
> code use them in the way that you suggest in comment 10.

OK, that seems like a reasonable compromise.

(Just to clarify: what I'm strongly against, on principle, is infecting
standard display:grid/flex(*) with this -webkit-box-* crap, especially
since other browsers don't do that.  I don't care if we support align-/
justify-* for "display:-webkit-box" or not, so for that part I'm mostly
concerned with the web compat risk.

(*) and I suspect also display:block, table types and column layout once
we support align/justify there.)

Comment 14

[Mats Palmgren (inactive)]

So, using this in Chrome:

  display: -webkit-box;
  -webkit-box-pack: justify;
  -webkit-box-orient: vertical;

it looks like it should map to 'align-content: space-between'.
Do we handle "-webkit-box-orient: vertical" in a way that swaps these values
or something?  or is this case rare enough that we don't care about compat?

Comment 15

[Mats Palmgren (inactive)]

Oh, it seems we already do that.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #14)
> Do we handle "-webkit-box-orient: vertical" in a way that swaps these values
> or something?  or is this case rare enough that we don't care about compat?

We handle "-webkit-box-orient: vertical" as an alias for "flex-direction: column".  So, it produces a veritcal flexbox, and -webkit-box-pack (aliased to justify-content:space-between) will operate in the vertical axis, as it should.

* (modulo the writing-mode -- so, if the writing-mode is vertical, then we instead map "-webkit-box-orient: vertical" to "flex-direction: row". Either way, it produces a vertical flexbox)

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> (*) and I suspect also display:block, table types and column layout once
> we support align/justify there.)

(Your footnote about blocks/tables here is the most actually-worrying-in-the-real-world thing to me, FWIW. I can *totally* see legacy content having copypasted, inactive "-webkit-box-pack"/"-webkit-box-align" junk on blocks & tables...  And when we broaden our align-*/justify-* support to do stuff on blocks & tables, this inactive CSS would become active & potentially cause trouble.)

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8731850 [details]
MozReview Request: Bug 1231682 part 2: Support "-webkit-box-pack: justify" as a special-case exception during "justify-content" parsing. r=mats

https://reviewboard.mozilla.org/r/40885/#review37463

r=mats with the condition that we unalias it soon-ish, as discussed above.

Please also add a XXX comment somewhere, in ParseAlignJustifyContent here,
or somewhere strategic in part 1, that this alias is a temporary solution
that will be replaced in a future bug.

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8731849 [details]
MozReview Request: Bug 1231682 part 1: When nsCSSParser is parsing a property-alias, keep track of the alias in a member-var. r=heycam

https://reviewboard.mozilla.org/r/40883/#review38051

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8731849 [details]
MozReview Request: Bug 1231682 part 1: When nsCSSParser is parsing a property-alias, keep track of the alias in a member-var. r=heycam

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

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8731850 [details]
MozReview Request: Bug 1231682 part 2: Support "-webkit-box-pack: justify" as a special-case exception during "justify-content" parsing. r=mats

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

Comment 22

[Daniel Holbert [:dholbert]]

So, Try revealed some mochitest failures:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=118a389a63b7

The failures happen because this patch only makes us support stuff like...
  elem.style = "-webkit-box-pack: justify"
...but this patch does NOT let us support:
  elem.style.WebkitBoxPack = "justify"

The latter doesn't work, because my "part 1" strategy here (setting mAliasBeingParsed) doesn't get a chance to take effect. That use-case ends up entering the parser via nsCSSParser::ParseProperty, with the alias *already* having been converted to the aliased property (eCSSProperty_justify_content in this case) -- so it's too late for us to know that we're really parsing an alias.

So, "part 1" might be fatally flawed here...  Rather than trying to work around that, it's probably best that I just fix this correctly via the strategy that mats suggested in comment 10. (which is bug 1257688)

So my current plan here is just to fix bug 1257688, & then resolve this as FIXED by that bug.

(Sorry for using up review cycles for ultimately-doomed patches here!  It was still definitely useful for me, in any case, because it got us to comment 10 & the ultimate likely-good way forward here. :))

(Also: fortunately, the other bug that potentially going to use "part 1" -- bug 1231829 -- seems like it may be unnecessary as well, per recent comments there.)

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

(Here's a testcase for this bug, for verification purposes.)

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> it's probably best that I just fix this correctly via the
> strategy that mats suggested in comment 10. (which is bug 1257688)
> 
> So my current plan here is just to fix bug 1257688, & then resolve this as
> FIXED by that bug.

Bug 1257688 has landed, and this is now FIXED by that bug.  I verified that the testcase passes in a local trunk build. (I believe it should pass in tomorrow's Nightly, too.)

Comment 25

[Daniel Holbert [:dholbert]]

(And this is in-testsuite+ via the "part 2" patch on bug 1257688, specifically this cset:
 https://hg.mozilla.org/mozilla-central/rev/bd3daa360f1b )

Comment 26

[Daniel Holbert [:dholbert]]

I just noticed that the webkit-box-anon-flex-items-* reftests each have...
>     -webkit-box-pack: justify;
>    justify-content: space-between; /* XXX remove when bug 1231682 is fixed */
...to polyfill this bug with justify-content, before it was fixed. (note the reference to this bug's bug number)

That "justify-content" style is now redundant and can go away. I'll push a late-breaking test-only followup patch to make that change.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99404af2e2b3
late-breaking followup: remove some now-redundant CSS from webkit-box reftests, now that "-webkit-box-pack: justify" works. (no review, test-only)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99404af2e2b3

Comment 29

[Daniel Holbert [:dholbert]]

I landed comment 26 / comment 27's cset on aurora, too.  (It's test-only, hence a=NPOTB.)  This helps bug 1309119's "part 0" backport apply cleanly.

Comment 30

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a68c83d7e509a04b4a250f4ea1bf71af51a5169