style: Turn the CSS flexible box model on by default. #13590

pcwalton · 2016-10-04T23:19:38Z

See
https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo
for discussion.

r? @larsbergstrom

This change is

highfive · 2016-10-04T23:19:41Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/box.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/shorthand/position.mako.rs

highfive · 2016-10-04T23:19:42Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

stshine · 2016-10-04T23:35:09Z

@bors-servo try
Since column flexbox is still not implemented there may be some false-positives. But that may not be a big deal, I suppose.


        Auto merge of #13590 - pcwalton:turn-flexbox-on, r=<try>

@larsbergstrom

style: Turn the CSS flexible box model on by default. See https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo for discussion. r? @larsbergstrom  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13590)

bors-servo · 2016-10-04T23:35:12Z

⌛ Trying commit 3383184 with merge 4f67355...

bors-servo · 2016-10-05T00:14:08Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-05T00:14:10Z

  ▶ FAIL [expected PASS] /_mozilla/css/dirty_viewport.html
  └   → /_mozilla/css/dirty_viewport.html 111b0141f71ae09366d1ce49dcece7c79df65484
/_mozilla/css/dirty_viewport_ref.html 068fd52a0e48bc9f97e4b141e6c2d91351ec8dc4
Testing 111b0141f71ae09366d1ce49dcece7c79df65484 == 068fd52a0e48bc9f97e4b141e6c2d91351ec8dc4

stshine · 2016-10-05T00:31:44Z

I am comfused that with prefs enabled on my local machine there are tens of tests passed. And how can the current failed test been affected?
cc @SimonSapin

larsbergstrom · 2016-10-05T01:00:44Z

I'm in favor of this (and the code changes look good), provided that the changes in test expectations are reasonable. Flexbox is definitely starting to look better, and having it on by default will let us get more feedback, which is great while we also have multiple people actively working on it (especially @stshine and @shinglyu).

stshine · 2016-10-05T01:11:07Z

Sorry, did not notice the mailing list discussion before.
My original plan is to enable them after I have implemented column flexbox. But I fell into the rabbit hole of marking the flex items approperately during construction, and have been struggling in the mud of nsCSSFrameConstructor.cpp since then.
Since I may have digged too deep, IMHO it is generally good if you agree to enable them for practical reason if the number of false-positive tests enabled is small. And of course I hope to enable them, why not 😄

pcwalton · 2016-10-05T03:04:15Z

@stshine I'm not sure what you mean by "I am comfused that with prefs enabled on my local machine there are tens of tests passed." Could you rephrase?

stshine · 2016-10-05T03:22:33Z

@pcwalton If run the tests with flex related properties enabled in prefs.json there will be more than ten additional tests got passed.

pcwalton · 2016-10-05T20:03:56Z

@bors-servo: try

I don't understand how the above test could possibly be related and it passes locally.

pcwalton · 2016-10-05T20:31:58Z

@stshine With this patch, the same number of flexbox tests pass as with master. Looking at one of them, I think margin handling is making a whole bunch of them fail.

This is a good reason to land this IMO: we can't catch regressions without it.


        style: Turn the CSS flexible box model on by default.

See https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo for discussion.

pcwalton · 2016-10-05T20:36:54Z

@bors-servo: try

bors-servo · 2016-10-05T20:36:58Z

⌛ Trying commit 806c6e9 with merge b43f4b9...


        Auto merge of #13590 - pcwalton:turn-flexbox-on, r=<try>

@larsbergstrom

style: Turn the CSS flexible box model on by default. See https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo for discussion. r? @larsbergstrom  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13590)

bors-servo · 2016-10-05T21:15:10Z

💔 Test failed - linux-rel-css

highfive · 2016-10-05T21:15:12Z

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/abspos-replaced-width-margin-000.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ Shutting down the Constellation after generating an output file or exit flag specified
  └ thread panicked while processing panic. aborting.

pcwalton · 2016-10-05T22:41:53Z

@bors-servo: try

stshine · 2016-10-05T22:55:19Z

flexbox_absolute-atomic.htm seems to be the only false-positives since it has a abs-pos child in the flex container and the current code does nothing about it. Otherwise this should be OK.

jdm · 2016-10-05T23:25:41Z

@bors-servo: retry

bors-servo · 2016-10-05T23:25:43Z

⌛ Trying commit 806c6e9 with merge 95285fe...


        Auto merge of #13590 - pcwalton:turn-flexbox-on, r=<try>

@larsbergstrom

style: Turn the CSS flexible box model on by default. See https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo for discussion. r? @larsbergstrom  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13590)

bors-servo · 2016-10-06T00:52:43Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

stshine · 2016-10-06T16:05:16Z

See #12453 (comment) for an incomplete list of unimplemented features of flexbox if anyone is interested.

emilio

r=me with that if you want to get this landed. I think the consensus is that this is what most people wanted.

emilio · 2016-10-12T08:22:00Z

components/style/properties/longhand/box.mako.rs

@@ -23,7 +23,7 @@
        """.split()
        if product == "gecko":
            values += "-moz-box -moz-inline-box".split()
-        experimental_values = set("flex".split())
+        experimental_values = set()


can you get rid of this variable entirely and its use below too?

jdm · 2016-11-02T15:01:35Z

@pcwalton One small change required, then r+.

stshine · 2016-11-04T00:33:08Z

If this get merged it should close #12191, #12455 and #13576 .

nox · 2016-11-07T11:33:10Z

Superseded by #14111.

pcwalton added I-enhancement S-needs-decision A-layout/flex labels Oct 4, 2016

highfive assigned larsbergstrom Oct 4, 2016

highfive added the S-awaiting-review label Oct 4, 2016

highfive added the S-tests-failed label Oct 5, 2016

stshine mentioned this pull request Oct 5, 2016

Enable flexbox layout by default #13576

Closed

style: Turn the CSS flexible box model on by default.

Loading status checks…

806c6e9

See https://groups.google.com/forum/#!topic/mozilla.dev.servo/MWBms2_BMJo for discussion.

pcwalton force-pushed the pcwalton:turn-flexbox-on branch from 3383184 to 806c6e9 Oct 5, 2016

highfive removed the S-tests-failed label Oct 5, 2016

highfive added the S-tests-failed label Oct 5, 2016

emilio reviewed Oct 12, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-decision S-tests-failed labels Nov 2, 2016

nox closed this Nov 7, 2016

stshine mentioned this pull request Jul 10, 2017

Implement flex-grow #12455

Closed

servo / servo

style: Turn the CSS flexible box model on by default. #13590

style: Turn the CSS flexible box model on by default. #13590

pcwalton commented Oct 4, 2016 •

edited by larsbergstrom

highfive commented Oct 4, 2016

highfive commented Oct 4, 2016

stshine commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 5, 2016

highfive commented Oct 5, 2016

stshine commented Oct 5, 2016 •

edited

larsbergstrom commented Oct 5, 2016

stshine commented Oct 5, 2016

pcwalton commented Oct 5, 2016

stshine commented Oct 5, 2016

pcwalton commented Oct 5, 2016

pcwalton commented Oct 5, 2016

pcwalton commented Oct 5, 2016

bors-servo commented Oct 5, 2016

bors-servo commented Oct 5, 2016

highfive commented Oct 5, 2016

pcwalton commented Oct 5, 2016

stshine commented Oct 5, 2016 •

edited

jdm commented Oct 5, 2016

bors-servo commented Oct 5, 2016

bors-servo commented Oct 6, 2016

stshine commented Oct 6, 2016 •

edited

emilio left a comment

This comment has been minimized.

jdm commented Nov 2, 2016

stshine commented Nov 4, 2016

nox commented Nov 7, 2016

servo / servo

Join GitHub today

style: Turn the CSS flexible box model on by default. #13590

style: Turn the CSS flexible box model on by default. #13590

Conversation

pcwalton commented Oct 4, 2016 • edited by larsbergstrom

highfive commented Oct 4, 2016

highfive commented Oct 4, 2016

stshine commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 5, 2016

highfive commented Oct 5, 2016

stshine commented Oct 5, 2016 • edited

larsbergstrom commented Oct 5, 2016

stshine commented Oct 5, 2016

pcwalton commented Oct 5, 2016

stshine commented Oct 5, 2016

pcwalton commented Oct 5, 2016

pcwalton commented Oct 5, 2016

pcwalton commented Oct 5, 2016

bors-servo commented Oct 5, 2016

bors-servo commented Oct 5, 2016

highfive commented Oct 5, 2016

pcwalton commented Oct 5, 2016

stshine commented Oct 5, 2016 • edited

jdm commented Oct 5, 2016

bors-servo commented Oct 5, 2016

bors-servo commented Oct 6, 2016

stshine commented Oct 6, 2016 • edited

emilio left a comment

This comment has been minimized.

emilio Oct 12, 2016 • edited

jdm commented Nov 2, 2016

stshine commented Nov 4, 2016

nox commented Nov 7, 2016

pcwalton commented Oct 4, 2016 •

edited by larsbergstrom

stshine commented Oct 5, 2016 •

edited

stshine commented Oct 5, 2016 •

edited

stshine commented Oct 6, 2016 •

edited

emilio Oct 12, 2016 •

edited