Implemented display: inline-flex #14978

shinglyu · 2017-01-12T06:41:46Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14685 (github issue number if applicable).

There are tests for these changes

This change is

shinglyu · 2017-01-12T06:44:10Z

components/layout/construct.rs

@@ -1316,6 +1316,39 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
        self.build_flow_for_block_like(flow, node)
    }

+    /// Builds a fragment for a node with 'display: inline-flex'.
+    // Shared code with build_fragment_for_inline_block
+    fn build_fragment_for_inline_flex(&mut self, node: &ConcreteThreadSafeLayoutNode)


This function should probably reuse build_fragment_for_inline_block, but I'm not sure if we want to have a build_fragment_for_inline_block_or_inline_flex and pass a flag into it, or should we have two functions and extract the shared part into a helper function.

Yeah, I agree add a flag should be a better solution.

Also, small nit: the Shared code comment needs another slash in front of it to be a doc comment.

shinglyu · 2017-01-12T06:45:04Z

cc @stshine

shinglyu · 2017-01-12T07:39:33Z

r? anyone?
@highfive where are you?

stshine · 2017-01-12T08:28:08Z

r? @mbrubeck or @pcwalton or @notriddle
I am busy with my job, will try to look later.

mbrubeck · 2017-01-12T22:21:00Z

components/layout/inline.rs

@@ -1154,6 +1155,7 @@ impl InlineFlow {
                }
                (display::T::inline, vertical_align::T::bottom) |
                (display::T::block, vertical_align::T::bottom) |
+                (display::T::inline_flex, vertical_align::T::top) |


top should be bottom here

stshine

Disclaimer: I do not have access as a reviewer.

stshine · 2017-01-13T10:47:34Z

components/layout/construct.rs

@@ -1316,6 +1316,39 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
        self.build_flow_for_block_like(flow, node)
    }

+    /// Builds a fragment for a node with 'display: inline-flex'.
+    // Shared code with build_fragment_for_inline_block
+    fn build_fragment_for_inline_flex(&mut self, node: &ConcreteThreadSafeLayoutNode)


Yeah, I agree add a flag should be a better solution.

stshine · 2017-01-13T10:47:34Z

components/style/properties/properties.mako.rs

@@ -1899,7 +1899,8 @@ pub fn apply_declarations<'a, F, I>(viewport_size: Size2D<Au>,
        % if product == "gecko":
        computed_values::display::T::grid |


Why not also fix inline-grid for stylo?

stshine · 2017-01-13T10:47:34Z

tests/wpt/metadata-css/css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm.ini

@@ -1,4 +0,0 @@
-[flex-flexitem-percentage-prescation.htm]


This test actually does not pass.

shinglyu · 2017-01-18T03:32:31Z

Let me know when it's ready for squashing.

stshine · 2017-01-18T05:08:56Z

Looks good to me, nice work :-)

notriddle · 2017-01-19T18:07:10Z

Squash this, then I'll r+ it.


        Implemented display: inline-flex

shinglyu · 2017-01-23T02:31:58Z

@notriddle Squahed. Thank you!

notriddle · 2017-01-23T03:46:15Z

@bors-servo r+

bors-servo · 2017-01-23T03:46:16Z

📌 Commit e31ee04 has been approved by notriddle

bors-servo · 2017-01-23T03:46:20Z

⌛ Testing commit e31ee04 with merge 9aa4848...


        Auto merge of #14978 - shinglyu:inline-flex, r=notriddle

Implemented display: inline-flex  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14685 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14978)

bors-servo · 2017-01-23T03:52:15Z

💔 Test failed - linux-rel-wpt

stshine · 2017-01-23T03:57:46Z

@bors-servo retry

internal compiler error

bors-servo · 2017-01-23T03:57:50Z

⌛ Testing commit e31ee04 with merge c75946c...


        Auto merge of #14978 - shinglyu:inline-flex, r=notriddle

Implemented display: inline-flex  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14685 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14978)

bors-servo · 2017-01-23T05:16:45Z

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

shinglyu reviewed Jan 12, 2017

View changes

highfive assigned mbrubeck Jan 12, 2017

mbrubeck requested changes Jan 12, 2017

View changes

stshine reviewed Jan 13, 2017

View changes

mbrubeck added the S-needs-code-changes label Jan 13, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 18, 2017

Implemented display: inline-flex

Loading status checks…

e31ee04

shinglyu force-pushed the shinglyu:inline-flex branch from dd345cb to e31ee04 Jan 23, 2017

highfive assigned notriddle and unassigned mbrubeck Jan 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 23, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 23, 2017

bors-servo merged commit e31ee04 into servo:master Jan 23, 2017
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 Jan 23, 2017

shinglyu mentioned this pull request Jan 23, 2017

Fully implement flexbox #12453

Open

servo / servo

Implemented display: inline-flex #14978

Implemented display: inline-flex #14978

shinglyu commented Jan 12, 2017 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Jan 12, 2017

shinglyu commented Jan 12, 2017

stshine commented Jan 12, 2017

This comment has been minimized.

stshine left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Jan 18, 2017

stshine commented Jan 18, 2017 •

edited

notriddle commented Jan 19, 2017

shinglyu commented Jan 23, 2017

notriddle commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

stshine commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

		@@ -1899,7 +1899,8 @@ pub fn apply_declarations<'a, F, I>(viewport_size: Size2D<Au>,
		% if product == "gecko":
		computed_values::display::T::grid \|

servo / servo

Join GitHub today

Implemented display: inline-flex #14978

Implemented display: inline-flex #14978

Conversation

shinglyu commented Jan 12, 2017 • edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Jan 12, 2017

shinglyu commented Jan 12, 2017

stshine commented Jan 12, 2017

This comment has been minimized.

stshine left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shinglyu commented Jan 18, 2017

stshine commented Jan 18, 2017 • edited

notriddle commented Jan 19, 2017

shinglyu commented Jan 23, 2017

notriddle commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

stshine commented Jan 23, 2017

bors-servo commented Jan 23, 2017

bors-servo commented Jan 23, 2017

shinglyu commented Jan 12, 2017 •

edited by larsbergstrom

stshine commented Jan 18, 2017 •

edited