Make text decorations have the same color as the text if no shadows are present #12233

KiChjang · 2016-07-04T18:09:27Z

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

There are tests for these changes OR
These changes do not require tests because according to @SimonSapin, making reftests against underlines are impossible

This change is

highfive · 2016-07-04T18:09:31Z

Warning

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

KiChjang · 2016-07-04T18:18:06Z

@bors-servo try

bors-servo · 2016-07-04T18:18:09Z

⌛ Trying commit 22bf780 with merge 230c96d...


        Auto merge of #12233 - KiChjang:underline-color, r=<try>

@SimonSapin

Make text decorations have the same color as the text if no shadows are present  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11870 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because according to @SimonSapin, making reftests against underlines are impossible   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12233)

frewsxcv · 2016-07-04T18:20:19Z

components/layout/display_list_builder.rs

+            stacking_relative_content_box.translate(&Point2D::new(shadow.offset_x, shadow.offset_y))
+        } else {
+            stacking_relative_content_box.translate(&Point2D::new(Au(0), Au(0)))
+        };


Alternatively:

let shadow = text_shadow.map(|s| Point2D::new(s.offset_x, s.offset_y)) .unwrap_or_else(Point2D::zero); let stacking_relative_content_box = stacking_relative_content_box.translate(shadow);

frewsxcv · 2016-07-04T18:23:06Z

components/layout/display_list_builder.rs

+            text_decorations.overline = text_decorations.overline.map(|_| color);
+            text_decorations.line_through = text_decorations.line_through.map(|_| color);
+        } else {
+            // Otherwise, paint the decorations the same color as the text color.
            text_decorations.underline = text_decorations.underline.map(|_| text_color);
            text_decorations.overline = text_decorations.overline.map(|_| text_color);
            text_decorations.line_through = text_decorations.line_through.map(|_| text_color);


Alternatively:

let color = if let Some(shadow) { // If we're painting a shadow, paint the decorations the same color as the shadow. self.style().resolve_color(shadow.color) } else { // Otherwise, paint the decorations the same color as the text color. text_color }; text_decorations.underline = text_decorations.underline.map(|_| color); text_decorations.overline = text_decorations.overline.map(|_| color); text_decorations.line_through = text_decorations.line_through.map(|_| color);

bors-servo · 2016-07-04T19:17:40Z

💔 Test failed - linux-rel

highfive · 2016-07-04T19:17:42Z

  ▶ FAIL [expected PASS] /_mozilla/css/text_shadow_blur_a.html
  └   → /_mozilla/css/text_shadow_blur_a.html ca056db5687f665c3923e05c2a95d4f7ec983246
/_mozilla/css/text_shadow_blur_ref.html 1dcb267142b0d33c7e322a537414003d1344c6f6
Testing ca056db5687f665c3923e05c2a95d4f7ec983246 == 1dcb267142b0d33c7e322a537414003d1344c6f6

  ▶ FAIL [expected PASS] /_mozilla/css/text_shadow_decorations_a.html
  └   → /_mozilla/css/text_shadow_decorations_a.html bb21311f7d2d4ba2db6b013ec30a3d845b540650
/_mozilla/css/text_shadow_decorations_ref.html 85ae9bc63581ee93871a4cbb4c9ffc7dea870140
Testing bb21311f7d2d4ba2db6b013ec30a3d845b540650 == 85ae9bc63581ee93871a4cbb4c9ffc7dea870140

  ▶ FAIL [expected PASS] /_mozilla/css/text_shadow_simple_a.html
  └   → /_mozilla/css/text_shadow_simple_a.html f95ddaa19c0b77350e46c9a47c7736b6b659b280
/_mozilla/css/text_shadow_simple_ref.html 0dd153f974d11ebaf1cc70c397aa5fdcc885a4cb
Testing f95ddaa19c0b77350e46c9a47c7736b6b659b280 == 0dd153f974d11ebaf1cc70c397aa5fdcc885a4cb

  ▶ FAIL [expected PASS] /_mozilla/css/text_shadow_multiple_shadows_a.html
  └   → /_mozilla/css/text_shadow_multiple_shadows_a.html 08440d117eeaf84a5e91ca5ca47a71ea7abd9025
/_mozilla/css/text_shadow_multiple_shadows_ref.html daa1ec97b0a46c19e5da6e06403a314e19a68230
Testing 08440d117eeaf84a5e91ca5ca47a71ea7abd9025 == daa1ec97b0a46c19e5da6e06403a314e19a68230

jdm · 2016-07-04T19:52:59Z

Also from test-css:

  ▶ PASS [expected FAIL] /css21_dev/html4/painting-order-underline-001.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/text-decoration-va-length-001.htm

  ▶ PASS [expected FAIL] /css21_dev/html4/text-decoration-va-length-002.htm

KiChjang · 2016-07-05T04:12:59Z

@bors-servo try


        Auto merge of #12233 - KiChjang:underline-color, r=<try>

@SimonSapin

Make text decorations have the same color as the text if no shadows are present  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11870 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because according to @SimonSapin, making reftests against underlines are impossible   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12233)

bors-servo · 2016-07-05T04:13:01Z

⌛ Trying commit f55ffa4 with merge 9650ef1...

notriddle · 2016-07-05T04:13:48Z

If a text shadow and underline are both present, then the shadow will have the same color as the text instead of its real color.

<!DOCTYPE html>
<span style="text-shadow: 0 0 10px green; color: red; text-decoration: underline">Test text</span>

Couldn't build_display_list_for_text_decoration have two color arguments? One for the text's real color, and one for the shadow?

notriddle · 2016-07-05T04:23:26Z

@KiChjang, you fixed it a few seconds before I posted that comment. Don't mind me.

bors-servo · 2016-07-05T05:05:25Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

emilio · 2016-07-05T05:56:13Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

components/layout/display_list_builder.rs, line 1569 [r2] (raw file):

                                                  self.style().get_cursor(cursor),
                                                  DisplayListSection::Content);
        let color = if let Some(shadow) = text_shadow {

Can we compute the color just in one place?

Something like:

let text_color = if let Some(shadow) = text_shadow {
    self.style().resolve_color(shadow.color)
} else if fragment.selected() {
    ...
} else { 
    ...
};

Also. a link to the relevant spec fragment or similar would be extremely nice. And a line saying: (// Note that text decorations are always the same color as the text.) would be even better :)

LGTM with that :)

Comments from Reviewable

emilio · 2016-07-05T06:20:43Z

Review status: 1 of 4 files reviewed at latest revision, 4 unresolved discussions.

components/layout/display_list_builder.rs, line 1537 [r2] (raw file):

            self.style().get_color().color
        };
        let offset = text_shadow.map(|s| Point2D::new(s.offset_x, s.offset_y)).unwrap_or_else(Point2D::zero);

Also, here you could write:

let (offset, shadow_blur_radius) = match text_shadow {
    Some(shadow) => {...}
    None => {..}
};

That would avoid using map() multiple times over the same value, that feels a bit awkward. That being said it might not be as readable so I also don't have a strong preference.

components/layout/display_list_builder.rs, line 1569 [r2] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Disregard ^, sorry for the spam

Also, note that this _might_ be written the way it was (explicitly receiving the color) to avoid extra indirections when creating related text fragments), though in practice I don't think it matters that much, and I'd prefer it to be readable.

It could be just how thinks have evolved.

Anyway if @pcwalton (who is the original writer it seems) wants to weigh in, it can be moved to the top of the loop in build_fragment_specific_display_items, with this logic just in the for loop, but I don't think it's necessary.

Comments from Reviewable


        Make text decorations have the same color as the text if no shadows a…

…re present

KiChjang · 2016-07-05T09:41:39Z

I'm not sure where the relevant spec is located, but other than that, I've addressed the comments.

emilio · 2016-07-05T18:12:15Z

LGTM

@bors-servo: r+

-S-awaiting-review +S-awaiting-answer

Reviewed 3 of 4 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

bors-servo · 2016-07-05T18:12:16Z

📌 Commit 764b82e has been approved by emilio

bors-servo · 2016-07-06T02:51:15Z

⌛ Testing commit 764b82e with merge 726cd6c...


        Auto merge of #12233 - KiChjang:underline-color, r=emilio

@SimonSapin

Make text decorations have the same color as the text if no shadows are present  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11870 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because according to @SimonSapin, making reftests against underlines are impossible   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12233)

bors-servo · 2016-07-06T03:38:11Z

💔 Test failed - mac-rel-wpt

notriddle · 2016-07-06T03:41:22Z

@bors-servo retry

#11099

cbrewster · 2016-07-06T03:44:53Z

The test failed with a different panic:
#12284

KiChjang · 2016-07-06T05:00:43Z

@bors-servo retry

bors-servo · 2016-07-06T08:05:14Z

⌛ Testing commit 764b82e with merge b577d66...


        Auto merge of #12233 - KiChjang:underline-color, r=emilio

@SimonSapin

Make text decorations have the same color as the text if no shadows are present  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11870 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because according to @SimonSapin, making reftests against underlines are impossible   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12233)

bors-servo · 2016-07-06T08:57:06Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned mbrubeck Jul 4, 2016

highfive added the S-awaiting-review label Jul 4, 2016

frewsxcv reviewed Jul 4, 2016
View changes

highfive added the S-tests-failed label Jul 4, 2016

KiChjang force-pushed the KiChjang:underline-color branch from 22bf780 to 4ff07de Jul 5, 2016

highfive removed the S-tests-failed label Jul 5, 2016

KiChjang force-pushed the KiChjang:underline-color branch from 4ff07de to f55ffa4 Jul 5, 2016

KiChjang force-pushed the KiChjang:underline-color branch from f55ffa4 to 58456a0 Jul 5, 2016

KiChjang force-pushed the KiChjang:underline-color branch 2 times, most recently from 9e6fd14 to 2ce6850 Jul 5, 2016

Make text decorations have the same color as the text if no shadows a…

Loading status checks…

764b82e

…re present

KiChjang force-pushed the KiChjang:underline-color branch from 2ce6850 to 764b82e Jul 5, 2016

emilio added S-awaiting-answer and removed S-awaiting-review labels Jul 5, 2016

highfive assigned emilio and unassigned mbrubeck Jul 5, 2016

highfive added S-awaiting-merge and removed S-awaiting-answer labels Jul 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 6, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 6, 2016

bors-servo merged commit 764b82e into servo:master Jul 6, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 6, 2016

bors-servo mentioned this pull request Jul 6, 2016

Snap text baseline to nearest pixel before positioning glyphs #12270

Closed

3 of 5 tasks complete

KiChjang deleted the KiChjang:underline-color branch Jul 6, 2016

servo / servo

Make text decorations have the same color as the text if no shadows are present #12233

Make text decorations have the same color as the text if no shadows are present #12233

KiChjang commented Jul 4, 2016 •

edited by larsbergstrom

highfive commented Jul 4, 2016

KiChjang commented Jul 4, 2016

bors-servo commented Jul 4, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 4, 2016

highfive commented Jul 4, 2016

jdm commented Jul 4, 2016

KiChjang commented Jul 5, 2016

bors-servo commented Jul 5, 2016

notriddle commented Jul 5, 2016 •

edited

notriddle commented Jul 5, 2016 •

edited

bors-servo commented Jul 5, 2016

emilio commented Jul 5, 2016

emilio commented Jul 5, 2016

KiChjang commented Jul 5, 2016

emilio commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

notriddle commented Jul 6, 2016 •

edited

cbrewster commented Jul 6, 2016

KiChjang commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

servo / servo

Join GitHub today

Make text decorations have the same color as the text if no shadows are present #12233

Make text decorations have the same color as the text if no shadows are present #12233

Conversation

KiChjang commented Jul 4, 2016 • edited by larsbergstrom

highfive commented Jul 4, 2016

KiChjang commented Jul 4, 2016

bors-servo commented Jul 4, 2016

This comment has been minimized.

frewsxcv Jul 4, 2016 • edited

This comment has been minimized.

frewsxcv Jul 4, 2016 • edited

bors-servo commented Jul 4, 2016

highfive commented Jul 4, 2016

jdm commented Jul 4, 2016

KiChjang commented Jul 5, 2016

bors-servo commented Jul 5, 2016

notriddle commented Jul 5, 2016 • edited

notriddle commented Jul 5, 2016 • edited

bors-servo commented Jul 5, 2016

emilio commented Jul 5, 2016

emilio commented Jul 5, 2016

KiChjang commented Jul 5, 2016

emilio commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

notriddle commented Jul 6, 2016 • edited

cbrewster commented Jul 6, 2016

KiChjang commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

KiChjang commented Jul 4, 2016 •

edited by larsbergstrom

frewsxcv Jul 4, 2016 •

edited

frewsxcv Jul 4, 2016 •

edited

notriddle commented Jul 5, 2016 •

edited

notriddle commented Jul 5, 2016 •

edited

notriddle commented Jul 6, 2016 •

edited