Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set flex-basis to 0% when omitted in flex shorthand. #17073

Merged
merged 1 commit into from May 29, 2017

Conversation

Projects
None yet
5 participants
@upsuper
Copy link
Member

commented May 29, 2017

This should fix bug 1331530.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 29, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/length.rs, components/style/properties/helpers.mako.rs, components/style/properties/shorthand/position.mako.rs
  • @emilio: components/style/values/specified/length.rs, components/style/properties/helpers.mako.rs, components/style/properties/shorthand/position.mako.rs
@highfive

This comment has been minimized.

Copy link

commented May 29, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio
Copy link
Member

left a comment

r=me with that bit.

flex_basis: basis.unwrap_or(longhands::flex_basis::SpecifiedValue::zero()),
// Per spec, this should be SpecifiedValue::zero(), but all
// browsers currently agree on using `0%`. This is a spec
// change which hasn't be adopted by browsers.

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Member

Could you link to the either the spec change or the bug?

@emilio

emilio approved these changes May 29, 2017

@upsuper upsuper force-pushed the upsuper:flex-basis-zero branch from 9b54e60 to a6c01b6 May 29, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit a6c01b6 has been approved by emilio

@highfive highfive assigned emilio and unassigned nox May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit a6c01b6 with merge 71372d0...

bors-servo added a commit that referenced this pull request May 29, 2017

Auto merge of #17073 - upsuper:flex-basis-zero, r=emilio
Set flex-basis to 0% when omitted in flex shorthand.

This should fix [bug 1331530](https://bugzilla.mozilla.org/show_bug.cgi?id=1331530).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17073)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

💔 Test failed - linux-dev

@upsuper

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit a6c01b6 with merge dae80dc0423132f685b971a1c02425905295842d...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

💔 Test failed - android

@upsuper

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit a6c01b6 with merge c47f215...

bors-servo added a commit that referenced this pull request May 29, 2017

Auto merge of #17073 - upsuper:flex-basis-zero, r=emilio
Set flex-basis to 0% when omitted in flex shorthand.

This should fix [bug 1331530](https://bugzilla.mozilla.org/show_bug.cgi?id=1331530).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17073)
<!-- Reviewable:end -->
@upsuper

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

r? @nox

@highfive highfive assigned nox and unassigned emilio May 29, 2017

@emilio
Copy link
Member

left a comment

Sigh, I guess.

flex_basis: basis.unwrap_or(longhands::flex_basis::SpecifiedValue::zero()),
// Per spec, this should be SpecifiedValue::zero(), but all
// browsers currently agree on using `0%`. This is a spec
// change which hasn't be adopted by browsers:

This comment has been minimized.

Copy link
@emilio

emilio May 29, 2017

Member

nit: hasn't been.

@emilio

This comment has been minimized.

Copy link
Member

commented May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit 4622b2d has been approved by emilio

@highfive highfive assigned emilio and unassigned nox May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit 4622b2d with merge d865ef8ccd83d83b12a70ab52628eedafe0a290a...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

💔 Test failed - android

@emilio

This comment has been minimized.

Copy link
Member

commented May 29, 2017

@bors-servo r-

Err, please fix the grammar nit :)

@upsuper upsuper force-pushed the upsuper:flex-basis-zero branch from 4622b2d to acb7242 May 29, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented May 29, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

📌 Commit acb7242 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Testing commit acb7242 with merge 76daf46...

bors-servo added a commit that referenced this pull request May 29, 2017

Auto merge of #17073 - upsuper:flex-basis-zero, r=emilio
Set flex-basis to 0% when omitted in flex shorthand.

This should fix [bug 1331530](https://bugzilla.mozilla.org/show_bug.cgi?id=1331530).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17073)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

@bors-servo bors-servo merged commit acb7242 into servo:master May 29, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@upsuper upsuper deleted the upsuper:flex-basis-zero branch May 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.