Add keyword "auto" to flexbox min-size property #14332

shinglyu · 2016-11-23T03:07:47Z

This is the first patch for #12610, the algorithm itself took longer than I expected, so I'm landing this foundational work first.

This adds a new initial value auto to the min-width and min-height specified on a flexitem. It also provides a fallback of "0px" if we try to get the min-width/height on a non-flexitem box using getComputedStyle.

The auto resolution algorithm is not ready yet, so we fall back to 0px during layout.

r?@stshine

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

There are tests for these changes OR
These changes do not require tests because _____

This change is


        Change min-width/height to LengthOrPercentageOrAuto for flexbox


        Let computed value for min-size=0px for non-flexitem


        Added wpt test to test min-size fallback behavior

stshine

Generally this looks good to me, but need some changes.
Disclaimer: I do not have the right to approve this pull request, just give out some suggestions.

stshine · 2016-11-23T05:51:16Z

components/layout/model.rs

+        LengthOrPercentageOrAuto::Auto => Au(0),
+    }
+}
+


This function is unnessasary, and its name is quite confusing, hinting that it will return a MaybeAuto type. Use MaybeAuto::from_style().specified_or_zero() should be enough.

stshine · 2016-11-23T05:51:16Z

components/style/properties/properties.mako.rs

+                computed::LengthOrPercentageOrAuto::Length(Au(0))
+            );
+        }
+    % endfor


I can not understand why we need this. I assume that you are going to change specified_or_auto() later? But use MaybeAuto::from_style().specified_or_zero() should provide us enough flexibility.

Just found that for non-flex items the computed value of min-width should be 0px if not specified, never mind.

Yes, I also added a wpt test for that. But I'm not sure if I put it in the correct location.

:) It is ok to put css tests there because they can not be easily upstreamed.

stshine · 2016-11-23T05:59:16Z

cc @SimonSapin for the style part changes.

SimonSapin · 2016-11-28T09:08:47Z

Reviewed 8 of 8 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/layout/model.rs, line 460 at r3 (raw file):

Previously, stshine wrote…

This function is unnessasary, and its name is quite confusing, hinting that it will return a MaybeAuto type. Use MaybeAuto::from_style().specified_or_zero() should be enough.

I agree that the name should indicate that 'auto' is treated as zero.

components/style/properties/properties.mako.rs, line 1706 at r2 (raw file):
This is more specific than flex items:

https://drafts.csswg.org/css-flexbox/#valdef-min-width-auto

On a flex item whose overflow is visible in the main axis, when specified on the flex item’s main-axis min-size property

Comments from Reviewable

bors-servo · 2016-12-12T13:00:48Z

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

shinglyu · 2016-12-21T08:49:19Z

Based on the new comment, it's non-trivial to determine if an element is a flex item, so I need some other way around this:

    // FIXME(heycam): We should look past any display:contents ancestors to
    // determine if we are a flex or grid item, but we don't have access to
    // grandparent or higher style here.

stshine · 2016-12-21T11:45:47Z

IMO we currently do not support display: contents in servo, so maybe you do not need to worry about it too much since the quoted spec is for layout.

SimonSapin · 2016-12-21T14:12:06Z

But Firefox does support display: contents, and therefore Stylo will need it.

In terms of threading / parallel tree traversal, the styling of ancestors is finished by the time we style an element. It should be possible to make this available to the cascading code.

emilio · 2017-01-17T00:16:25Z

Yeah, this will need to look up (or have some kind of hint from the parent). It's not problematic for now though, and you can ignore that until we implement display contents for stylo.

jdm · 2017-01-27T19:03:22Z

What's the next step here? Who's responsible for it?

SimonSapin · 2017-01-28T09:16:02Z

I’ve requested code changes from @shinglyu, the most significant one being checking not just for flex items but also "whose overflow is visible [etc.]" #14332 (comment)

shinglyu · 2017-01-28T11:28:34Z

Sorry, I'll get back to this after the holidays Simon Sapin <notifications@github.com> 於 2017年1月28日週六 17:16 寫道：

…

I’ve requested code changes from @shinglyu <https://github.com/shinglyu>, the most significant one being checking not just for flex items but also "whose overflow is visible [etc.]" #14332 (comment) <#14332 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14332 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGbJ3njlDwIoVkFXJNxY7W_IOvy43ZRks5rWwdZgaJpZM4K6Ljj> .

nox · 2017-03-03T11:42:59Z

@shinglyu Status?

shinglyu · 2017-03-06T07:20:23Z

I'm fully occupied by Stylo right now. Maybe need to postpone this for a while. If anyone is interested to take over please go ahead.

stshine · 2017-03-13T11:54:00Z

@shinglyu I would like to finish this if you do not have time.

emilio · 2017-03-13T15:02:31Z

That'd be awesome @stshine! :)

stshine · 2017-03-13T16:20:36Z

@emilio I'll take a look at the stylo implemention first :)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)


        Auto merge of #15969 - stshine:auto-min-size, r=<try>

style: Support "auto" keyword on min-{width, height} properties  Based on the work of #14332 . ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15969)

emilio · 2017-09-05T15:51:27Z

Let's close this due to inactivity, and given it needs a huge rebase.

shinglyu added 3 commits Oct 21, 2016

Change min-width/height to LengthOrPercentageOrAuto for flexbox

8dfe8ca

Let computed value for min-size=0px for non-flexitem

095de77

Added wpt test to test min-size fallback behavior

Loading status checks…

e7b518d

stshine reviewed Nov 23, 2016

View changes

SimonSapin self-assigned this Nov 27, 2016

SimonSapin added the S-awaiting-review label Nov 27, 2016

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Nov 28, 2016

highfive added the S-needs-rebase label Dec 12, 2016

emilio mentioned this pull request Jan 17, 2017

min-{width,height} should support "auto" value #15045

Closed

stshine mentioned this pull request Mar 15, 2017

style: Support "auto" keyword on min-{width, height} properties #15969

Closed

3 of 5 tasks complete

nox closed this Mar 24, 2017

nox reopened this Mar 24, 2017

emilio closed this Sep 5, 2017

servo / servo

Add keyword "auto" to flexbox min-size property #14332

Add keyword "auto" to flexbox min-size property #14332

shinglyu commented Nov 23, 2016 •

edited by larsbergstrom

stshine left a comment •

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.

stshine commented Nov 23, 2016

SimonSapin commented Nov 28, 2016

bors-servo commented Dec 12, 2016

shinglyu commented Dec 21, 2016

stshine commented Dec 21, 2016

SimonSapin commented Dec 21, 2016

emilio commented Jan 17, 2017

jdm commented Jan 27, 2017

SimonSapin commented Jan 28, 2017

shinglyu commented Jan 28, 2017

nox commented Mar 3, 2017

shinglyu commented Mar 6, 2017

stshine commented Mar 13, 2017

emilio commented Mar 13, 2017

stshine commented Mar 13, 2017

emilio commented Sep 5, 2017

servo / servo

Join GitHub today

Add keyword "auto" to flexbox min-size property #14332

Add keyword "auto" to flexbox min-size property #14332

Conversation

shinglyu commented Nov 23, 2016 • edited by larsbergstrom

stshine left a comment • edited

This comment has been minimized.

stshine Nov 23, 2016

This comment has been minimized.

stshine Nov 23, 2016

This comment has been minimized.

stshine Nov 23, 2016

This comment has been minimized.

shinglyu Nov 25, 2016

This comment has been minimized.

stshine Nov 25, 2016

stshine commented Nov 23, 2016

SimonSapin commented Nov 28, 2016

bors-servo commented Dec 12, 2016

shinglyu commented Dec 21, 2016

stshine commented Dec 21, 2016

SimonSapin commented Dec 21, 2016

emilio commented Jan 17, 2017

jdm commented Jan 27, 2017

SimonSapin commented Jan 28, 2017

shinglyu commented Jan 28, 2017

nox commented Mar 3, 2017

shinglyu commented Mar 6, 2017

stshine commented Mar 13, 2017

emilio commented Mar 13, 2017

stshine commented Mar 13, 2017

emilio commented Sep 5, 2017

shinglyu commented Nov 23, 2016 •

edited by larsbergstrom

stshine left a comment •

edited