Reset longhands of border-image to their initial value while parsing border shorthand #16522

chenpighead · 2017-04-19T03:52:41Z

To do so, we need to declare the border-image longhands as part of border
shorthand and always reset them.

Spec link: https://drafts.csswg.org/css-backgrounds-3/#the-border-shorthands

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Setting border and border-[side] shorthands should reset longhands of border-image to their initial value #15202 (github issue number if applicable).

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

This change is

highfive · 2017-04-19T03:52:45Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/shorthand/border.mako.rs
@emilio: components/style/properties/shorthand/border.mako.rs

highfive · 2017-04-19T03:52:46Z

Warning

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

chenpighead · 2017-04-19T03:53:37Z

r? @heycam

Set reviewer first, although stylo try is not done yet...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60ef8e6024d2134891d090d37647dcb8ff72755

heycam · 2017-04-19T04:00:36Z

@bors-servo delegate+

bors-servo · 2017-04-19T04:00:37Z

✌️ @chenpighead can now approve this pull request

chenpighead · 2017-04-19T08:09:53Z

Hmmm... I'm a bit confused about the two test failures. @heycam, does that means we'd like to see

border-top: 4px solid; border-right: 4px solid; border-bottom: 4px solid; border-left: 4px solid;

to be serialized to

border: 4px solid;

?

chenpighead · 2017-04-19T10:18:12Z

I guess we might need a special case to handle border::LonghandsToSerialize... :(

heycam · 2017-04-19T14:22:35Z

In general, CSS declarations are serialized using the shortest longhand possible that can be used to account for properties in the declaration. In this case, none of border-{top,right,bottom,left} set the border-image-* properties, but the border shorthand does, so it means we shouldn't serialize to border: 4px solid. (We would if the declaration was border-top: 4px solid; border-right: 4px solid; border-bottom: 4px solid; border-left: 4px solid; border-image: none;.)

Although it looks like Gecko will serialize that to border-width: 4px; border-style: solid; border-color: currentcolor;, so maybe there is a bug in Declaration::ToString. @Xidorn, do you know?

…parsing border shorthand. To do so, we need to declare the border-image longhands as part of border shorthand and always reset them. This could fix couple stylo test failures. See Gecko Bug 1357350 for the test update patches. Spec link: https://drafts.csswg.org/css-backgrounds-3/#the-border-shorthands Note that two unit tests have been fixed as well: 1. border_should_serialize_correctly To verify the correctness of serialization of 'border' shorthand, we need to reset 'border-image' as well. 2. same_longhands_should_serialize_correctly Due to the same reason as above, since the 'border-image' is not reset, the test expectation should be fixed.

chenpighead · 2017-04-21T11:08:14Z

@bors-servo try

bors-servo · 2017-04-21T11:08:18Z

⌛ Trying commit 4352581 with merge 087bf98...

…=<try> Reset longhands of border-image to their initial value while parsing border shorthand To do so, we need to declare the border-image longhands as part of border shorthand and always reset them. Spec link: https://drafts.csswg.org/css-backgrounds-3/#the-border-shorthands  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15202 (github issue number if applicable).  - [ ] 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/16522)

chenpighead · 2017-04-21T11:15:59Z

I've fixed the failures by patching the two failed unit tests.

border_should_serialize_correctly
To verify the correctness of serialization of 'border' shorthand, we need to reset 'border-image' as well. This fix also makes the test to be agreed with the example listed in the spec.
same_longhands_should_serialize_correctly
Due to the same reason as above, since the 'border-image' is not reset, the test expectation should be fixed now.

@heycam, could you review the test fix?

bors-servo · 2017-04-21T11:44:37Z

☀️ 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-msvc-dev
State: approved= try=True

heycam · 2017-04-23T02:06:19Z

Those test changes look good to me, thanks!

jdm · 2017-04-23T02:41:14Z

@bors-servo: r=heycam

bors-servo · 2017-04-23T02:41:15Z

📌 Commit 4352581 has been approved by heycam

bors-servo · 2017-04-23T02:41:19Z

⌛ Testing commit 4352581 with merge 940fb5c...

…=heycam Reset longhands of border-image to their initial value while parsing border shorthand To do so, we need to declare the border-image longhands as part of border shorthand and always reset them. Spec link: https://drafts.csswg.org/css-backgrounds-3/#the-border-shorthands  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15202 (github issue number if applicable).  - [ ] 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/16522)

bors-servo · 2017-04-23T03:05:39Z

💔 Test failed - mac-rel-wpt1

chenpighead · 2017-04-23T07:51:44Z

@bors-servo: retry

bors-servo · 2017-04-23T07:51:48Z

⌛ Testing commit 4352581 with merge 33c6abb...

…=heycam Reset longhands of border-image to their initial value while parsing border shorthand To do so, we need to declare the border-image longhands as part of border shorthand and always reset them. Spec link: https://drafts.csswg.org/css-backgrounds-3/#the-border-shorthands  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15202 (github issue number if applicable).  - [ ] 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/16522)

bors-servo · 2017-04-23T08:22:10Z

💔 Test failed - linux-rel-wpt

chenpighead · 2017-04-23T08:25:33Z

@bors-servo: retry
:-(

bors-servo · 2017-04-23T08:25:35Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-wpt1, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-css, mac-rel-wpt2...

bors-servo · 2017-04-23T08:41:09Z

☀️ 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-msvc-dev
Approved by: heycam
Pushing 33c6abb to master...

highfive assigned pcwalton Apr 19, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 19, 2017

highfive assigned heycam and unassigned pcwalton Apr 19, 2017

chenpighead force-pushed the stylo-border-reset-border-image branch from f2486b6 to 4352581 Compare April 21, 2017 11:07

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 23, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 23, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 23, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 23, 2017

bors-servo merged commit 4352581 into servo:master Apr 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset longhands of border-image to their initial value while parsing border shorthand #16522

Reset longhands of border-image to their initial value while parsing border shorthand #16522

chenpighead commented Apr 19, 2017 •

edited by larsbergstrom

Loading

highfive commented Apr 19, 2017

highfive commented Apr 19, 2017

chenpighead commented Apr 19, 2017

heycam commented Apr 19, 2017

bors-servo commented Apr 19, 2017

chenpighead commented Apr 19, 2017

chenpighead commented Apr 19, 2017

heycam commented Apr 19, 2017

chenpighead commented Apr 21, 2017

bors-servo commented Apr 21, 2017

chenpighead commented Apr 21, 2017

bors-servo commented Apr 21, 2017

heycam commented Apr 23, 2017

jdm commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

chenpighead commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

chenpighead commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

Reset longhands of border-image to their initial value while parsing border shorthand #16522

Reset longhands of border-image to their initial value while parsing border shorthand #16522

Conversation

chenpighead commented Apr 19, 2017 • edited by larsbergstrom Loading

highfive commented Apr 19, 2017

highfive commented Apr 19, 2017

chenpighead commented Apr 19, 2017

heycam commented Apr 19, 2017

bors-servo commented Apr 19, 2017

chenpighead commented Apr 19, 2017

chenpighead commented Apr 19, 2017

heycam commented Apr 19, 2017

chenpighead commented Apr 21, 2017

bors-servo commented Apr 21, 2017

chenpighead commented Apr 21, 2017

bors-servo commented Apr 21, 2017

heycam commented Apr 23, 2017

jdm commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

chenpighead commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

chenpighead commented Apr 23, 2017

bors-servo commented Apr 23, 2017

bors-servo commented Apr 23, 2017

chenpighead commented Apr 19, 2017 •

edited by larsbergstrom

Loading