Update WR (switch to new text-decorations API in WR). #17814

glennw · 2017-07-21T02:53:39Z

This only makes use of the "Solid" text decoration type, which
matches the existing support. WR now supports dotted, dashed
and wavy text decorations, but supporting those will need some
extra work in Servo to pass through the correct values.

This change is

highfive · 2017-07-21T02:53:43Z

Heads up! This PR modifies the following files:

@emilio: components/layout/webrender_helpers.rs, components/layout/display_list_builder.rs

highfive · 2017-07-21T02:53:44Z

Warning

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

glennw · 2017-07-21T02:53:51Z

r? @emilio or @pcwalton

glennw · 2017-07-21T02:54:03Z

@bors-servo try

bors-servo · 2017-07-21T02:54:07Z

⌛ Trying commit 06fd6b2 with merge f7f69a5...


        Auto merge of #17814 - glennw:update-wr-text-decorations, r=<try>

Update WR (switch to new text-decorations API in WR). This only makes use of the "Solid" text decoration type, which matches the existing support. WR now supports dotted, dashed and wavy text decorations, but supporting those will need some extra work in Servo to pass through the correct values.

bors-servo · 2017-07-21T03:40:59Z

💔 Test failed - linux-rel-css

glennw · 2017-07-21T04:36:10Z

Huh, test failure seems related but doesn't occur locally.

glennw · 2017-07-21T05:09:41Z

Ah, I see the failure now. Before this PR, no text decoration showed up at all in this test, so it accidentally passes. With this update, the text decoration does show up, but exposes a bug in how Servo handles vertical-align. Marked the test as an expected fail given it was previously incorrectly marked as a PASS.

glennw · 2017-07-21T05:09:57Z

@bors-servo try

bors-servo · 2017-07-21T05:10:01Z

⌛ Trying commit 24f9f52 with merge b7019a9...


        Auto merge of #17814 - glennw:update-wr-text-decorations, r=<try>

Update WR (switch to new text-decorations API in WR). This only makes use of the "Solid" text decoration type, which matches the existing support. WR now supports dotted, dashed and wavy text decorations, but supporting those will need some extra work in Servo to pass through the correct values.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17814)

bors-servo · 2017-07-21T06:00:15Z

💔 Test failed - linux-rel-css

jdm · 2017-07-21T12:12:21Z

That is an instance of #13479.

bors-servo · 2017-07-21T23:55:52Z

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

glennw · 2017-07-23T21:37:43Z

@bors-servo try

bors-servo · 2017-07-23T21:37:47Z

⌛ Trying commit 3ec092a with merge 2823794...


        Auto merge of #17814 - glennw:update-wr-text-decorations, r=<try>

Update WR (switch to new text-decorations API in WR). This only makes use of the "Solid" text decoration type, which matches the existing support. WR now supports dotted, dashed and wavy text decorations, but supporting those will need some extra work in Servo to pass through the correct values.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17814)

bors-servo · 2017-07-23T22:39:10Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

glennw · 2017-07-23T22:41:50Z

r? anyone

emilio

LGTM, with that either addressed or explained.

emilio · 2017-07-23T22:55:44Z

components/layout/display_list_builder.rs

@@ -2135,7 +2135,7 @@ impl FragmentDisplayListBuilding for Fragment {
                                       .get_inheritedtext()
                                       ._servo_text_decorations_in_effect;
        // Note that the text decoration colors are always the same as the text color.
-        text_decorations.underline = text_decorations.underline.map(|_| text_color);
+        text_decorations.underline = text_decorations.underline;//.map(|_| text_color);


Why is this correct? Mind leaving a comment so people note it's intentional?

Also, if this change is really correct, please remove the leftover comment.

Oops, that was a mistake, fixed.


        Update WR (switch to new text-decorations API in WR).

This only makes use of the "Solid" text decoration type, which matches the existing support. WR now supports dotted, dashed and wavy text decorations, but supporting those will need some extra work in Servo to pass through the correct values.

glennw · 2017-07-24T03:04:35Z

@bors-servo r=emilio

bors-servo · 2017-07-24T03:04:35Z

📌 Commit dc82366 has been approved by emilio

bors-servo · 2017-07-24T03:04:39Z

⌛ Testing commit dc82366 with merge 46ffcba...


        Auto merge of #17814 - glennw:update-wr-text-decorations, r=emilio

Update WR (switch to new text-decorations API in WR). This only makes use of the "Solid" text decoration type, which matches the existing support. WR now supports dotted, dashed and wavy text decorations, but supporting those will need some extra work in Servo to pass through the correct values.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17814)

bors-servo · 2017-07-24T04:05:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 46ffcba to master...

highfive assigned Manishearth Jul 21, 2017

highfive added the S-awaiting-review label Jul 21, 2017

highfive assigned emilio and unassigned Manishearth Jul 21, 2017

highfive added the S-tests-failed label Jul 21, 2017

glennw force-pushed the glennw:update-wr-text-decorations branch from 06fd6b2 to 24f9f52 Jul 21, 2017

highfive removed the S-tests-failed label Jul 21, 2017

highfive added the S-tests-failed label Jul 21, 2017

highfive added the S-needs-rebase label Jul 21, 2017

glennw force-pushed the glennw:update-wr-text-decorations branch from 24f9f52 to 3ec092a Jul 23, 2017

highfive removed the S-tests-failed label Jul 23, 2017

emilio approved these changes Jul 23, 2017

View changes

glennw force-pushed the glennw:update-wr-text-decorations branch from 3ec092a to dc82366 Jul 24, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 24, 2017

bors-servo merged commit dc82366 into servo:master Jul 24, 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 Jul 24, 2017

servo / servo

Update WR (switch to new text-decorations API in WR). #17814

Update WR (switch to new text-decorations API in WR). #17814

glennw commented Jul 21, 2017 •

edited by larsbergstrom

highfive commented Jul 21, 2017

highfive commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

jdm commented Jul 21, 2017

bors-servo commented Jul 21, 2017

glennw commented Jul 23, 2017

bors-servo commented Jul 23, 2017

bors-servo commented Jul 23, 2017

glennw commented Jul 23, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

glennw commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

servo / servo

Join GitHub today

Update WR (switch to new text-decorations API in WR). #17814

Update WR (switch to new text-decorations API in WR). #17814

Conversation

glennw commented Jul 21, 2017 • edited by larsbergstrom

highfive commented Jul 21, 2017

highfive commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

glennw commented Jul 21, 2017

bors-servo commented Jul 21, 2017

bors-servo commented Jul 21, 2017

jdm commented Jul 21, 2017

bors-servo commented Jul 21, 2017

glennw commented Jul 23, 2017

bors-servo commented Jul 23, 2017

bors-servo commented Jul 23, 2017

glennw commented Jul 23, 2017

emilio left a comment

This comment has been minimized.

emilio Jul 23, 2017

This comment has been minimized.

glennw Jul 24, 2017

glennw commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

glennw commented Jul 21, 2017 •

edited by larsbergstrom