Implement flexible box layout for row container #12330

stshine · 2016-07-08T03:00:22Z

This pull requests implements basic flexible box layout for row container.
It has implemented most basic flexbox features, including grow, shrink, multi-line, *reverse properties, and alignment under justify-content, align-items, align-self, align-content.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

r? @pcwalton

This change is

highfive · 2016-07-08T03:00:26Z

Warning

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

stshine · 2016-07-08T03:03:24Z

There a several concerns for this pull request.

One is that previously every method involves a vtable lookup, so I add a style field and clone the style of the flow to it, but I am not very sure what the price is.
Another one is that there are occurrences of 'box-sizing' property here and there to get the proper main size. I wonder if there is a way to reduce them.

stshine · 2016-07-12T17:05:32Z

ping @pcwalton
Mind sharing some general opinion about this pull request? There are a bunch of PRs pending this.

shinglyu · 2016-07-15T15:35:46Z

(tracking #12453)

shinglyu · 2016-07-18T05:16:29Z

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

components/layout/flex.rs, line 145 [r1] (raw file):

impl FlexItem {
    pub fn new(flow: FlowRef) -> FlexItem {
        let style = flow.as_block().fragment.style.clone();

Will the style ever change after we clone it (e.g. by JavaScript)? Do we need to sync it up with the style side?

components/layout/flex.rs, line 222 [r1] (raw file):

        let ref fragment = self.flow.as_block().fragment;
        let adjustment;
        match mode {

We could just let adjustment = match mode {...}

Comments from Reviewable

shinglyu · 2016-07-18T05:17:17Z

Disclaimer: I don't have the right to r+ yet, just providing some feedback.

stshine · 2016-07-18T11:39:01Z

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

components/layout/flex.rs, line 145 [r1] (raw file):

Previously, shinglyu (Shing Lyu) wrote…

Will the style ever change after we clone it (e.g. by JavaScript)? Do we need to sync it up with the style side?

What cloned is just a Arc pointer to the actual `ServoComputedValues`, so It should not be a problem.

components/layout/flex.rs, line 191 [r1] (raw file):

                        (MaybeAuto::from_style(margin.inline_start, Au(0)).specified_or_zero() +
                         MaybeAuto::from_style(margin.inline_end, Au(0)).specified_or_zero())
                    }

Here when box-sizing is set to border-box, The border and padding length contained in main size is computed at intrinsic size calculating, which set percentage padding to zero. Maybe I should take percentage padding into account for inline main size.

components/layout/flex.rs, line 222 [r1] (raw file):

Previously, shinglyu (Shing Lyu) wrote…

We could just let adjustment = match mode {...}

Previously I do the border and margin computing here. Will fix it.

Comments from Reviewable

stshine · 2016-07-18T13:12:46Z

Disclaimer: I don't have the right to r+ yet, just providing some feedback.

May I ask your general opinion about this pull request? If you think this pull request is acceptable, I will make other PRs that depend on this.

pcwalton · 2016-07-18T18:22:37Z

components/layout/flex.rs

+    /// The 'order' property of this item.
+    pub order: i32,
+    /// Whether the main size has met its constraint.
+    pub is_freezed: bool,


nit: should this be is_frozen?

pcwalton · 2016-07-18T18:24:21Z

components/layout/flex.rs

+    }
+
+    /// Return the outer main size of the item, including paddings and margins,
+    /// clamped by max and min size.


Maybe this should become a method on Fragment?

All these methods exists as the result of refactoring the method of obtaining a line in flexbox. Both this and the adjustment issues remain here because I previously do margin and padding calculation in outer_main_size() . This point is quite valid, but 1. I am not sure if other components need this method, 2. This will make the style field unnecessary and make auto_margin_num() a function instead of a method. Maybe I should do this after more consideration, together with renaming the Mode enum to Direction and moving it to model.rs.

pcwalton · 2016-07-18T18:25:02Z

components/layout/flex.rs

+    pub fn outer_main_size(&self, mode: Mode) -> Au {
+        let ref fragment = self.flow.as_block().fragment;
+        let adjustment;
+        match mode {


nit: you can shorten this to let adjustment = match mode { ... } and then remove the adjustment =s below and the semicolons

pcwalton · 2016-07-18T18:25:49Z

components/layout/flex.rs

+    }
+}
+
+/// Represent a child in a flex container. Most fields here are used in


nit: "Represent" -> "Represents"

pcwalton · 2016-07-18T18:26:53Z

Looks good, just had a few nits. Thanks!

stshine · 2016-07-19T01:29:04Z

components/layout/flex.rs

+                        let margin = self.style.logical_margin();
+                        (MaybeAuto::from_style(margin.inline_start, Au(0)).specified_or_zero() +
+                         MaybeAuto::from_style(margin.inline_end, Au(0)).specified_or_zero())
+                    }


Here when the the box-sizing property is border-box, percentage padding included in main size is calculated to zero as a result of bubble_inline_sizes. Maybe I should recalculate them?

stshine · 2016-07-19T02:51:38Z

Issues addressed. Thanks!

shinglyu · 2016-07-20T04:10:25Z

Does any other code use these methods? If it's for future patches, maybe we should merge this right before the one that uses it.

stshine · 2016-07-20T06:24:53Z

@shinglyu told me for a big feature I should make separate commits and submit them in on PR. If so I will close other PRs, rename this PR, and push consequent commits for flexbox here.

bors-servo · 2016-07-20T09:42:05Z

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

jdm · 2016-07-20T13:31:32Z

That sounds like a good plan.


        Extend fields of FlexItem and add methods

Extend fields of `FlexItem` struct with values that are necessary to resolve flexible lengths, and the 'order' property. Add other methods for size computing to make the code more modular.

bors-servo · 2016-08-02T21:28:49Z

📌 Commit b2a8d74 has been approved by pcwalton

KiChjang · 2016-08-03T03:00:29Z

@bors-servo try- clean retry r=pcwalton

bors-servo · 2016-08-03T03:00:29Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors-servo · 2016-08-03T03:00:30Z

📌 Commit b2a8d74 has been approved by pcwalton

bors-servo · 2016-08-03T03:00:32Z

⌛ Testing commit b2a8d74 with merge fe47bdf...


        Auto merge of #12330 - stshine:flexitem, r=pcwalton

@pcwalton

Implement flexible box layout for row container  This pull requests implements basic flexible box layout for row container. It has implemented most basic flexbox features, including grow, shrink, multi-line, *reverse properties, and alignment under `justify-content`, `align-items`, `align-self`, `align-content`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] There are tests for these changes  r? @pcwalton  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12330)

bors-servo · 2016-08-03T03:53:43Z

💔 Test failed - mac-rel-css

highfive · 2016-08-03T03:53:46Z

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox-flex-direction-default.htm
  └   → /css-flexbox-1_dev/html/flexbox-flex-direction-default.htm a505f9f57c0b83f64a1ebf31bb8fad4649d94d07
/css-flexbox-1_dev/html/reference/flexbox-flex-direction-ref.htm 54fcc475c3f793d40ae0710e0c82021a8495aa35
Testing a505f9f57c0b83f64a1ebf31bb8fad4649d94d07 == 54fcc475c3f793d40ae0710e0c82021a8495aa35

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox-flex-direction-row-reverse.htm
  └   → /css-flexbox-1_dev/html/flexbox-flex-direction-row-reverse.htm a505f9f57c0b83f64a1ebf31bb8fad4649d94d07
/css-flexbox-1_dev/html/reference/flexbox-flex-direction-ref.htm 54fcc475c3f793d40ae0710e0c82021a8495aa35
Testing a505f9f57c0b83f64a1ebf31bb8fad4649d94d07 == 54fcc475c3f793d40ae0710e0c82021a8495aa35

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox-flex-direction-row.htm
  └   → /css-flexbox-1_dev/html/flexbox-flex-direction-row.htm a505f9f57c0b83f64a1ebf31bb8fad4649d94d07
/css-flexbox-1_dev/html/reference/flexbox-flex-direction-ref.htm 54fcc475c3f793d40ae0710e0c82021a8495aa35
Testing a505f9f57c0b83f64a1ebf31bb8fad4649d94d07 == 54fcc475c3f793d40ae0710e0c82021a8495aa35

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox-flex-wrap-wrap.htm
  └   → /css-flexbox-1_dev/html/flexbox-flex-wrap-wrap.htm a505f9f57c0b83f64a1ebf31bb8fad4649d94d07
/css-flexbox-1_dev/html/reference/flexbox-flex-direction-ref.htm 54fcc475c3f793d40ae0710e0c82021a8495aa35
Testing a505f9f57c0b83f64a1ebf31bb8fad4649d94d07 == 54fcc475c3f793d40ae0710e0c82021a8495aa35

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox-flex-wrap-wrap-reverse.htm
  └   → /css-flexbox-1_dev/html/flexbox-flex-wrap-wrap-reverse.htm a505f9f57c0b83f64a1ebf31bb8fad4649d94d07
/css-flexbox-1_dev/html/reference/flexbox-flex-direction-ref.htm 54fcc475c3f793d40ae0710e0c82021a8495aa35
Testing a505f9f57c0b83f64a1ebf31bb8fad4649d94d07 == 54fcc475c3f793d40ae0710e0c82021a8495aa35

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flexbox_box-clear.htm
  └   → /css-flexbox-1_dev/html/flexbox_box-clear.htm c032f9ccd110b4e972d0da4d7a9f2d0f6398a5f4
/css-flexbox-1_dev/html/reference/flexbox_box-clear-ref.htm eed758e56c9cd89490d0fdfda6531af771783e28
Testing c032f9ccd110b4e972d0da4d7a9f2d0f6398a5f4 == eed758e56c9cd89490d0fdfda6531af771783e28

stshine · 2016-08-03T12:59:55Z

Some upstream changes break the margin collapsing of references, which is sad. (Maybe also #12696 and #12676)


        Add Minor fixes and tests

wafflespeanut · 2016-08-03T13:25:15Z

@bors-servo r=pcwalton

bors-servo · 2016-08-03T13:25:15Z

📌 Commit 60e2f44 has been approved by pcwalton

bors-servo · 2016-08-03T14:24:32Z

⌛ Testing commit 60e2f44 with merge 15947f8...


        Auto merge of #12330 - stshine:flexitem, r=pcwalton

@pcwalton

Implement flexible box layout for row container  This pull requests implements basic flexible box layout for row container. It has implemented most basic flexbox features, including grow, shrink, multi-line, *reverse properties, and alignment under `justify-content`, `align-items`, `align-self`, `align-content`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] There are tests for these changes  r? @pcwalton  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12330)

bors-servo · 2016-08-03T15:27:08Z

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

pcwalton · 2016-08-03T20:59:30Z

🎉 Thanks for pushing this through, @stshine!

stshine · 2016-08-03T22:20:18Z

I feel satisfied and honored to be able to implement this for servo. Thank you @pcwalton again and thanks to everyone who helped!

highfive assigned pcwalton Jul 8, 2016

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

stshine mentioned this pull request Jul 15, 2016

Fully implement flexbox #12453

Open

pcwalton reviewed Jul 18, 2016
View changes

components/layout/flex.rs Outdated

/// The 'order' property of this item.

pub order: i32,

/// Whether the main size has met its constraint.

pub is_freezed: bool,

This comment has been minimized.

Sign in to view

Copy link

pcwalton Jul 18, 2016
Contributor

nit: should this be is_frozen?

pcwalton reviewed Jul 18, 2016
View changes

components/layout/flex.rs Outdated

}

}

/// Represent a child in a flex container. Most fields here are used in

This comment has been minimized.

Sign in to view

Copy link

pcwalton Jul 18, 2016
Contributor

nit: "Represent" -> "Represents"

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 18, 2016

stshine reviewed Jul 19, 2016
View changes

stshine force-pushed the stshine:flexitem branch from ee4755b to 47f55be Jul 19, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 19, 2016

stshine mentioned this pull request Jul 19, 2016

Add FlexLine and method for flexible lengths resolving #12509

Closed

2 of 5 tasks complete

highfive added the S-needs-rebase label Jul 20, 2016

Extend fields of FlexItem and add methods

5da880e

Extend fields of `FlexItem` struct with values that are necessary to resolve flexible lengths, and the 'order' property. Add other methods for size computing to make the code more modular.

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Aug 2, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 3, 2016

Add Minor fixes and tests

Loading status checks…

60e2f44

stshine force-pushed the stshine:flexitem branch from b2a8d74 to 60e2f44 Aug 3, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Aug 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 3, 2016

bors-servo merged commit 60e2f44 into servo:master Aug 3, 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 Aug 3, 2016

stshine mentioned this pull request Sep 7, 2016

Partial flex implementation of flex-grow, flex-shrink and flex-basis for inline mode #12298

Closed

4 of 5 tasks complete

servo / servo

Implement flexible box layout for row container #12330

Implement flexible box layout for row container #12330

stshine commented Jul 8, 2016 •

edited

highfive commented Jul 8, 2016

stshine commented Jul 8, 2016

stshine commented Jul 12, 2016 •

edited

shinglyu commented Jul 15, 2016

shinglyu commented Jul 18, 2016

shinglyu commented Jul 18, 2016

stshine commented Jul 18, 2016

stshine commented Jul 18, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented Jul 18, 2016

This comment has been minimized.

stshine commented Jul 19, 2016

shinglyu commented Jul 20, 2016

stshine commented Jul 20, 2016

bors-servo commented Jul 20, 2016

jdm commented Jul 20, 2016

bors-servo commented Aug 2, 2016

KiChjang commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

highfive commented Aug 3, 2016

stshine commented Aug 3, 2016

wafflespeanut commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

pcwalton commented Aug 3, 2016

stshine commented Aug 3, 2016

servo / servo

Join GitHub today

Implement flexible box layout for row container #12330

Implement flexible box layout for row container #12330

Conversation

stshine commented Jul 8, 2016 • edited

highfive commented Jul 8, 2016

stshine commented Jul 8, 2016

stshine commented Jul 12, 2016 • edited

shinglyu commented Jul 15, 2016

shinglyu commented Jul 18, 2016

shinglyu commented Jul 18, 2016

stshine commented Jul 18, 2016

stshine commented Jul 18, 2016 • edited

This comment has been minimized.

pcwalton Jul 18, 2016

This comment has been minimized.

pcwalton Jul 18, 2016

This comment has been minimized.

stshine Jul 19, 2016

This comment has been minimized.

pcwalton Jul 18, 2016

This comment has been minimized.

pcwalton Jul 18, 2016

pcwalton commented Jul 18, 2016

This comment has been minimized.

stshine Jul 19, 2016

stshine commented Jul 19, 2016

shinglyu commented Jul 20, 2016

stshine commented Jul 20, 2016

bors-servo commented Jul 20, 2016

jdm commented Jul 20, 2016

bors-servo commented Aug 2, 2016

KiChjang commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

highfive commented Aug 3, 2016

stshine commented Aug 3, 2016

wafflespeanut commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

bors-servo commented Aug 3, 2016

pcwalton commented Aug 3, 2016

stshine commented Aug 3, 2016

stshine commented Jul 8, 2016 •

edited

stshine commented Jul 12, 2016 •

edited

stshine commented Jul 18, 2016 •

edited