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

Have shorthand parsing functions not return Option for their longhands #15761

Merged
merged 5 commits into from Feb 28, 2017

Conversation

Projects
None yet
7 participants
@upsuper
Copy link
Member

commented Feb 28, 2017

This change is Reviewable

upsuper added some commits Feb 28, 2017

Resolve color:currentcolor during computing
The keyword value "currentcolor" should be preserved rather than being
replaced by the CSS-wide keyword "inherit" during parsing.
Have shorthand parsing functions return values
Shorthands are responsible to set all its longhands to a proper value,
rather than returning None.

Fixes #15380.
@highfive

This comment has been minimized.

Copy link

commented Feb 28, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/inherited_svg.mako.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/values/specified/length.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/shorthand/background.mako.rs, components/style/properties/shorthand/list.mako.rs, components/style/properties/shorthand/inherited_text.mako.rs, components/style/properties/shorthand/text.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/font.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/shorthand/box.mako.rs, components/style/properties/shorthand/column.mako.rs, components/style/values/specified/mod.rs, components/style/properties/longhand/color.mako.rs, components/style/properties/shorthand/outline.mako.rs, components/style/properties/shorthand/mask.mako.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/column.mako.rs
  • @KiChjang: components/script/dom/element.rs
  • @fitzgen: components/script/dom/element.rs
  • @emilio: components/style/properties/shorthand/inherited_svg.mako.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs, components/style/values/specified/length.rs, components/style/properties/longhand/outline.mako.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/properties.mako.rs, components/style/properties/shorthand/background.mako.rs, components/style/properties/shorthand/list.mako.rs, components/style/properties/shorthand/inherited_text.mako.rs, ports/geckolib/glue.rs, components/style/properties/shorthand/text.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/shorthand/font.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/shorthand/box.mako.rs, components/style/properties/shorthand/column.mako.rs, components/style/values/specified/mod.rs, components/style/properties/longhand/color.mako.rs, components/style/properties/shorthand/outline.mako.rs, components/style/properties/shorthand/mask.mako.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/column.mako.rs
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@highfive highfive assigned Manishearth and unassigned pcwalton Feb 28, 2017

@Manishearth
Copy link
Member

left a comment

r=me

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@bors-servo r=Maniahearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

📌 Commit f33b0b4 has been approved by Maniahearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

⌛️ Testing commit f33b0b4 with merge 73cfc50...

bors-servo added a commit that referenced this pull request Feb 28, 2017

Auto merge of #15761 - upsuper:shorthand, r=Maniahearth
Have shorthand parsing functions not return Option for their longhands

<!-- 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/15761)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

 ▶ Unexpected subtest result in /cssom/CSSStyleRule.html:
  │ FAIL [expected PASS] CSSStyleRule: style property
  │   → assert_equals: expected "1px solid" but got "1px solid currentColor"
  │ 
  │ @http://web-platform.test:8000/cssom/CSSStyleRule.html:22:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/cssom/CSSStyleRule.html:13:3
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

⌛️ Trying commit e850d95 with merge a905c0d...

bors-servo added a commit that referenced this pull request Feb 28, 2017

Auto merge of #15761 - upsuper:shorthand, r=<try>
Have shorthand parsing functions not return Option for their longhands

<!-- 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/15761)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

📌 Commit e850d95 has been approved by Manishearth

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

⌛️ Testing commit e850d95 with merge 7ee6294...

bors-servo added a commit that referenced this pull request Feb 28, 2017

Auto merge of #15761 - upsuper:shorthand, r=Manishearth
Have shorthand parsing functions not return Option for their longhands

<!-- 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/15761)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

☀️ 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
Approved by: Manishearth
Pushing 7ee6294 to master...

@bors-servo bors-servo merged commit e850d95 into servo:master Feb 28, 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

@hiikezoe hiikezoe referenced this pull request Mar 1, 2017

Closed

Set initial values for animation and transition property. #15780

3 of 4 tasks complete

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 1, 2017

Bug 1343166 - Update test expectation for servo/servo#15761.
--HG--
extra : rebase_source : 54155b656ce81f6d10454d95b122dcb738c4490f
@canaltinova

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

Btw, after this PR we are unable to determine which sub-longhands are specified in a shorthand, right?
I once filed an issue about what to do with the unspecified sub-longhand serialization(#14832) because some shorthands were printing unspecified sub-longhands and some shorthands weren't. I guess now all of them are printing initial values. If so, is it a right approach?

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Mar 9, 2017

JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Mar 13, 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.