796212 - Add pref to toggle flexbox support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

I'd like to turn on css3 flexbox support in nightly builds, initially guarded by a pref while we hammer out the remaining bugs / missing-chunks that are tracked by bug 783409.

I can easily guard out all the flexbox-specific properties by adding a pref to their chunks in nsCSSPropList.h -- that's easy.  So, if people are doing feature-detection based off of checking if e.g. "elem.style.MozFlex" exists, that'll work perfectly.

However -- it's not as straightforward to disable "display: -moz-flex" / "-moz-inline-flex".

Those are specified in the "display" keyword-table, here:
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp#954
and they're parsed via very generic code -- CSSParserImpl::ParseVariant() which calls nsCSSProps::FindKeyword().  We may not want to slow down that code with special-cases and pref checks.

bz/dbaron, do you know of any better way to guard a particular "display" value with a pref, and do you think it's worth doing so in this case? Or is it sufficient to just have the pref control the flex-specific-properties at this point, and assume that people will do feature-detection using those?

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Guard flexbox-specific properties behind "layout.css.flexbox.enabled" pref

This patch guards the flexbox-specific properties behind a pref, but as noted in previous comment, it doesn't stop "display: -moz-flex" from parsing & being honored.

Comment 2

[David Baron :dbaron:]

I'm surprised you didn't need:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/33bb240b5cab/generate-pref-correctly
(which I need to put in a bug along with another patch).

Comment 3

[Daniel Holbert [:dholbert]]

I do need that. I filed bug 795751 on the need for that, and bz already landed a fix on m-i. :)

Comment 4

[Daniel Holbert [:dholbert]]

(dbaron: feel free to steal the review on this, if you like. I only tagged bz because I don't want to bug you too much while you're on vacation.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666776 [details] [diff] [review]
part 1: Guard flexbox-specific properties behind "layout.css.flexbox.enabled" pref

r=me

As far as display values, what we can do if desired is make kDisplayKTable an array of non-readonly ints, and then write some code that observes the pref and modifies the array to have eCSSKeyword_UNKNOWN instead of eCSSKeyword__moz_flex as needed.  That's probably the simplest and cheapest way to get the pref working for display parsing.

Comment 6

[Daniel Holbert [:dholbert]]

Ooh, I like it!  I'll hack that up as a "patch 2" here.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 0: Add pref "layout.css.flexbox.enabled" to all.js

This patch adds "layout.css.flexbox.enabled" to all.js, for easy searchability in about:config.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make pref control whether "display: -moz-[inline-]flex" parses successfully

This patch does what bz suggested at the end of comment 5.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: Make pref control whether "display: -moz-[inline-]flex" parses successfully

(reposting, with some obsolete/unnecessary comments trimmed)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 2 v3: Make pref control whether "display: -moz-[inline-]flex" parses successfully

part 2 needs some #ifdef guards to build successfully in builds with the MOZ_FLEXBOX build flag disabled. (Those builds don't have e.g. eCSSKeyword__moz_flex defined.)

Added the #ifdef guards in this version.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666855 [details] [diff] [review]
part 0: Add pref "layout.css.flexbox.enabled" to all.js

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 667175 [details] [diff] [review]
part 2 v3: Make pref control whether "display: -moz-[inline-]flex" parses successfully

r=me if you add a comment at the end of kDisplayKTable that the flexbox display types must be the last ones in the table.  Otherwise flipping the flexbox pref off would also pref off whatever comes after these display types.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> r=me if you add a comment at the end of kDisplayKTable that the flexbox
> display types must be the last ones in the table.  Otherwise flipping the
> flexbox pref off would also pref off whatever comes after these display
> types.

Ah, right.  I'll add that comment & land these patches -- but I'll also attach one more patch to remove the need for that that hacky ordering-requirement.

Comment 14

[Daniel Holbert [:dholbert]]

Landed parts 0, 1, 2:
 part 0: https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd4065311eb
 part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c174f3c540f6
 part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6db22beec5

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: part 3: Check for both eCSSKeyword_UNKNOWN *and* -1 in sequence

(In reply to Daniel Holbert [:dholbert] from comment #13)
> >  Otherwise flipping the
> > flexbox pref off would also pref off whatever comes after these display
> > types.
> 
> Ah, right.  I'll add that comment & land these patches -- but I'll also
> attach one more patch to remove the need for that that hacky
> ordering-requirement.

This patch addresses this, by checking for {eCSSKeyword_UNKNOWN, -1}, instead of just eCSSKeyword_UNKNOWN, as the "end-of-table" signal.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: part 3 v2: Check for both eCSSKeyword_UNKNOWN *and* -1 in sequence

(Removed the assertion added in prev. version of this patch -- of cousrse NS_ARRAY_LENGTH can't statically determine the length of an arbitrary passed-in array.  And that line was missing a semicolon, but that's tangential. ;)  )

This version seems to work locally, even after I reorder the keyword table (shifting the flex keywords earlier) to challenge it a bit.

Comment 17

[Daniel Holbert [:dholbert]]

Try run to demonstrate that patch 3 works as-intended:
  https://tbpl.mozilla.org/?tree=Try&rev=b2fb9c900bc3

(That try-push includes a bogus keyword-table-reordering patch to demonstrate that part 3 does actually prevent us from truncating kDisplayKTable -- https://hg.mozilla.org/try/rev/173076e3d24f -- and also includes a MOZ_FLEXBOX-buildflag-enabling patch, so that this code is actually compiled)

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7dd4065311eb
https://hg.mozilla.org/mozilla-central/rev/c174f3c540f6
https://hg.mozilla.org/mozilla-central/rev/5c6db22beec5

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not convinced about part 3.  It would slow down all keyword lookups a bit, and the benefit seems marginal...

Comment 20

[Boris Zbarsky [:bzbarsky]]

At least it's marginal if we expect to unpref flexbox in the near future.  ;)

Comment 21

[Daniel Holbert [:dholbert]]

Fair enough. :)  We may need something like 'part 3' in the future, if we ever have multiple keywords for "display" that we want to make preffable; but I suppose we can take care of that if and when that happens.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> I'm not convinced about part 3.  It would slow down all keyword lookups a
> bit

...though actually, for the record -- the "part 3" patch actually *only* slows down keyword lookups that *fail*.  And it only slows those down by a single comparison ("looks like we've reached the end -- but is the next entry -1? OK, that means it's really the end.")

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  Good point.  In that case, maybe it's worth taking.

Comment 24

[Daniel Holbert [:dholbert]]

I'm in favor of taking it, but I'm biased because I wrote it. :)

I feel like, if we want to safely/robustly support pref-controllable keyword values for CSS properties via the "edit the keyword table" method that you suggest, we'll need something like this eventually.

Alternately: we could also create a new reserved CSS keyword "eCSSKeyword_DISABLED" and shove that in, instead of "UNKNOWN", for keywords that we want to pref off.  Then we could be sure we'll ignore those keywords, without creating ambiguity about where the end of the table is.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm not sure what the right thing is long-term for pref control for values... I have to admit that I'd been thinking of the editable keyword table as a one-off temporary hack. ;)

Comment 26

[Daniel Holbert [:dholbert]]

OK. :) Worrying about it right now might be premature, anyway. I'm happy leaving out part 3 for now.

Comment 27

[Daniel Holbert [:dholbert]]

Landed a trivial followup, to add "#ifdef MOZ_FLEXBOX" guards around the new static vars in nsLayoutUtils -- they're only used in a block of code that's ifdef-guarded, so they trigger "unused variable" build warnings in non-MOZ_FLEXBOX builds:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b72781fd478c

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b72781fd478c