Closed Bug 796212 Opened 7 years ago Closed 7 years ago

Add pref to toggle flexbox support

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 4 obsolete files)

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?
Depends on: 795751
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #666776 - Flags: review?(bzbarsky)
OS: Linux → All
Hardware: x86_64 → All
I do need that. I filed bug 795751 on the need for that, and bz already landed a fix on m-i. :)
(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 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.
Attachment #666776 - Flags: review?(bzbarsky) → review+
Ooh, I like it!  I'll hack that up as a "patch 2" here.
Summary: Add pref to enable flexbox support → Add pref to toggle flexbox support
Attachment #666776 - Attachment description: patch 1: Guard flexbox-specific properties behind "layout.css.flexbox.enabled" pref → part 1: Guard flexbox-specific properties behind "layout.css.flexbox.enabled" pref
This patch adds "layout.css.flexbox.enabled" to all.js, for easy searchability in about:config.
Attachment #666855 - Flags: review?(bzbarsky)
This patch does what bz suggested at the end of comment 5.
Attachment #666858 - Flags: review?(bzbarsky)
(reposting, with some obsolete/unnecessary comments trimmed)
Attachment #666858 - Attachment is obsolete: true
Attachment #666858 - Flags: review?(bzbarsky)
Attachment #666859 - Flags: review?(bzbarsky)
Blocks: 783409
Blocks: 797022
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.
Attachment #666859 - Attachment is obsolete: true
Attachment #666859 - Flags: review?(bzbarsky)
Attachment #667175 - Flags: review?(bzbarsky)
Comment on attachment 666855 [details] [diff] [review]
part 0: Add pref "layout.css.flexbox.enabled" to all.js

r=me
Attachment #666855 - Flags: review?(bzbarsky) → review+
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.
Attachment #667175 - Flags: review?(bzbarsky) → review+
(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.
(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.
Attachment #667327 - Flags: review?(bzbarsky)
(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.
Attachment #667327 - Attachment is obsolete: true
Attachment #667327 - Flags: review?(bzbarsky)
Attachment #667348 - Flags: review?(bzbarsky)
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)
I'm not convinced about part 3.  It would slow down all keyword lookups a bit, and the benefit seems marginal...
At least it's marginal if we expect to unpref flexbox in the near future.  ;)
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.
Whiteboard: [leave open]
Attachment #667348 - Attachment is obsolete: true
Attachment #667348 - Flags: review?(bzbarsky) → checkin-
(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.")
Oh, hmm.  Good point.  In that case, maybe it's worth taking.
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.
Blocks: 797601
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. ;)
OK. :) Worrying about it right now might be premature, anyway. I'm happy leaving out part 3 for now.
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
https://hg.mozilla.org/mozilla-central/rev/b72781fd478c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 801098
You need to log in before you can comment on or make changes to this bug.