Implement flexbox reordering #10178

dlrobertson · 2016-03-24T20:36:15Z

Add style property for order and implement reordering by this property
in flex flow. Based on previous work by @zentner-kyle.

Fixes: #9957

This change is

highfive · 2016-03-24T20:36:18Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties.mako.rs
@SimonSapin: components/style/properties.mako.rs

jdm · 2016-03-24T20:42:41Z

@bors-servo: try

bors-servo · 2016-03-24T20:42:43Z

⌛ Trying commit 05874ab with merge 50ca528...


        Auto merge of #10178 - danlrobertson:flex-order, r=<try>

@zentner-kyle

Implement flexbox reordering Add style property for `order` and implement reordering by this property in flex flow. Based on previous work by @zentner-kyle. Fixes: #9957  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10178)

dlrobertson · 2016-03-24T20:45:46Z

#9117 attempted to also correctly implement column-reverse and row-reverse. Implementing reverse will take a bit more work. I plan to submit a separate PR for it. Is anyone else working on FlexFlow? I'd like to continue working on it, but I don't want to step on anyone's toes.

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

components/layout/flex.rs, line 72 [r1] (raw file):
I find it more readable to remove these, but could be swayed to think otherwise 😄

components/layout/flex.rs, line 81 [r1] (raw file):
I really don't like that a sort is used each time this is called.

Comments from the review on Reviewable.io

jdm · 2016-03-24T20:56:39Z

Nobody else is working on FlexFlow. Step away!

dlrobertson · 2016-03-24T21:02:31Z

Sweet! Thanks!

bors-servo · 2016-03-24T21:19:41Z

💔 Test failed - mac-rel-wpt

dlrobertson · 2016-03-24T21:41:06Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/layout/flex.rs, line 76 [r1] (raw file):
Per the failure, as_block should not be used like this. My current thought is to patter match on Flow::class. If you know of a better way, just let me know.

Comments from the review on Reviewable.io

larsbergstrom · 2016-03-25T02:19:58Z

@bors-servo try

bors-servo · 2016-03-25T02:20:00Z

🔒 Merge conflict

larsbergstrom · 2016-03-25T02:35:37Z

@bors-servo try

bors-servo · 2016-03-25T02:35:39Z

⌛ Trying commit 557c013 with merge fce55da...


        Auto merge of #10178 - danlrobertson:flex-order, r=<try>

@zentner-kyle

Implement flexbox reordering Add style property for `order` and implement reordering by this property in flex flow. Based on previous work by @zentner-kyle. Fixes: #9957  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10178)

bors-servo · 2016-03-25T03:44:15Z

💔 Test failed - linux-rel

jdm · 2016-03-25T03:51:46Z

  ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flexbox_computedstyle_order-inherit.htm

  ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flexbox_computedstyle_order-invalid.htm

  ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flexbox_computedstyle_order-integer.htm

  ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flexbox_computedstyle_order-negative.htm

  ▶ PASS [expected FAIL] /css-flexbox-1_dev/html/flexbox_computedstyle_order.htm

dlrobertson · 2016-03-25T03:54:13Z

Woohoo! More passing tests!

stshine · 2016-03-25T06:02:00Z

Since the flex items of a container need to be iterated for multiple times by order, IMO a collection of FlexItemInfo is really needed to decrease sorting overheads and furthermore retain states between passes.

bors-servo · 2016-03-26T02:31:14Z

☔ The latest upstream changes (presumably #10085) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-04-06T22:29:19Z

☔ The latest upstream changes (presumably #10407) made this pull request unmergeable. Please resolve the merge conflicts.

dlrobertson · 2016-04-11T16:00:57Z

r? @SimonSapin

bors-servo · 2016-04-11T17:45:22Z

☔ The latest upstream changes (presumably #10490) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2016-04-11T19:32:58Z

r=me modulo your as_block comment.

Reviewed 2 of 4 files at r1, 5 of 5 files at r4, 1 of 2 files at r5, 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

components/layout/flex.rs, line 76 [r1] (raw file):
Sorry I don’t understand this and I don’t see what failure this refers to.

components/layout/flex.rs, line 81 [r1] (raw file):
There could be a "dirty" flag that indicates that FlexFlow::items need to be re-created and sorted. It would need be set when a flex item is added or removed or when their order value changes. This sounds non-trival, so it can be later follow-up.

components/style/properties.mako.rs, line 4928 [r10] (raw file):
input.try is not necessary in this case (since you don’t go on to parse something else when it returns Err), parse_integer can be called directly.

Comments from Reviewable


        Implement flexbox reordering

@zentner-kyle

Add style property for order and implement reordering by this property in flex flow. Based on previous work by @zentner-kyle.

dlrobertson · 2016-04-11T20:57:10Z

r=@SimonSapin

Review status: 8 of 10 files reviewed at latest revision, 5 unresolved discussions.

components/layout/flex.rs, line 76 [r1] (raw file):
This was an old comment before I realized that I forgot to implement as_block for FlexFlow (See the diff a little further down), so there was a test that hit a panic. If there was a flex box that was an item to a flex box, we would hit a panic here. This has been resolved.

components/layout/flex.rs, line 81 [r1] (raw file):
Ah! That is a good idea. It is probably more important now that we store a sorted Vec of FlowRefs in the FlexFlow. I'll add a issue for this.

components/style/properties.mako.rs, line 4928 [r10] (raw file):
Awesome, good catch. I'll make the update shortly

Comments from Reviewable

KiChjang · 2016-04-11T21:13:38Z

@bors-servo r=SimonSapin

bors-servo · 2016-04-11T21:13:39Z

📌 Commit 3580f91 has been approved by SimonSapin

SimonSapin · 2016-04-11T21:29:15Z

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/layout/flex.rs, line 81 [r1] (raw file):
For future readers: #10531

Comments from Reviewable

bors-servo · 2016-04-11T21:47:51Z

⌛ Testing commit 3580f91 with merge df21bb4...


        Auto merge of #10178 - danlrobertson:flex-order, r=SimonSapin

@zentner-kyle

Implement flexbox reordering Add style property for `order` and implement reordering by this property in flex flow. Based on previous work by @zentner-kyle. Fixes: #9957  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10178)

bors-servo · 2016-04-11T22:27:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive assigned asajeffrey Mar 24, 2016

highfive added the S-awaiting-review label Mar 24, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 1b4aba1 to 05874ab Mar 24, 2016

highfive added the S-tests-failed label Mar 24, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 05874ab to 50a14a6 Mar 25, 2016

highfive removed the S-tests-failed label Mar 25, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 50a14a6 to 557c013 Mar 25, 2016

highfive added the S-tests-failed label Mar 25, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 557c013 to 883aa27 Mar 25, 2016

highfive removed the S-tests-failed label Mar 25, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 883aa27 to 402ee16 Mar 25, 2016

highfive added the S-needs-rebase label Apr 6, 2016

asajeffrey removed the S-awaiting-review label Apr 6, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 3d8a315 to 0cff0cb Apr 8, 2016

highfive added the S-awaiting-review label Apr 8, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from 0cff0cb to b293f94 Apr 9, 2016

highfive assigned SimonSapin and unassigned asajeffrey Apr 11, 2016

highfive added the S-needs-rebase label Apr 11, 2016

dlrobertson force-pushed the dlrobertson:flex-order branch from b293f94 to 4570a13 Apr 11, 2016

Implement flexbox reordering

Loading status checks…

3580f91

Add style property for order and implement reordering by this property in flex flow. Based on previous work by @zentner-kyle.

dlrobertson force-pushed the dlrobertson:flex-order branch from 4570a13 to 3580f91 Apr 11, 2016

dlrobertson mentioned this pull request Apr 11, 2016

Implement a "dirty" flag for `FlexFlow::items` #10531

Open

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Apr 11, 2016

bors-servo merged commit 3580f91 into servo:master Apr 11, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 11, 2016

bors-servo mentioned this pull request Apr 11, 2016

Add some Gecko-supported CSS keyword properties #10524

Merged

dlrobertson deleted the dlrobertson:flex-order branch Apr 12, 2016

servo / servo

Implement flexbox reordering #10178

Implement flexbox reordering #10178

dlrobertson commented Mar 24, 2016

highfive commented Mar 24, 2016

jdm commented Mar 24, 2016

bors-servo commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

jdm commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

bors-servo commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

larsbergstrom commented Mar 25, 2016

bors-servo commented Mar 25, 2016

larsbergstrom commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

jdm commented Mar 25, 2016

dlrobertson commented Mar 25, 2016

stshine commented Mar 25, 2016

bors-servo commented Mar 26, 2016

bors-servo commented Apr 6, 2016

dlrobertson commented Apr 11, 2016

bors-servo commented Apr 11, 2016

SimonSapin commented Apr 11, 2016

dlrobertson commented Apr 11, 2016

KiChjang commented Apr 11, 2016

bors-servo commented Apr 11, 2016

SimonSapin commented Apr 11, 2016

bors-servo commented Apr 11, 2016

bors-servo commented Apr 11, 2016

servo / servo

Join GitHub today

Implement flexbox reordering #10178

Implement flexbox reordering #10178

Conversation

dlrobertson commented Mar 24, 2016

highfive commented Mar 24, 2016

jdm commented Mar 24, 2016

bors-servo commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

jdm commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

bors-servo commented Mar 24, 2016

dlrobertson commented Mar 24, 2016

larsbergstrom commented Mar 25, 2016

bors-servo commented Mar 25, 2016

larsbergstrom commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

jdm commented Mar 25, 2016

dlrobertson commented Mar 25, 2016

stshine commented Mar 25, 2016

bors-servo commented Mar 26, 2016

bors-servo commented Apr 6, 2016

dlrobertson commented Apr 11, 2016

bors-servo commented Apr 11, 2016

SimonSapin commented Apr 11, 2016

dlrobertson commented Apr 11, 2016

KiChjang commented Apr 11, 2016

bors-servo commented Apr 11, 2016

SimonSapin commented Apr 11, 2016

bors-servo commented Apr 11, 2016

bors-servo commented Apr 11, 2016