1574222 - text-decoration-thickness isn't being serialized as part of the text-decoration shorthand computed-value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. set pref layout.css.text-decoration-thickness.enabled to true
2. Load attached testcase

ACTUAL RESULTS:
The testcase shows this output:

auto: [text-decoration: 'none', text-decoration-thickness: 'auto']
fromFont: [text-decoration: 'none', text-decoration-thickness: 'from-font']
tenPx: [text-decoration: 'none', text-decoration-thickness: '10px']

EXPECTED RESULTS:
For the second and third lines, the text-decoration serialization should match the text-decoration-thickness serialization. (It should say "from-font" and "10px".)

It looks like we're not considering the "text-decoration-thickness" computed value, when serializing a value for the "text-decoration" shorthand...

Comment 1

[Daniel Holbert [:dholbert]]

Looks like this is the last thing that we've got blocking the metabug (bug 1573631) for letting these new text features riding the trains with Firefox 70.

Boris or emilio: I think you both know the serialization code pretty well -- could I interest either of you in fixing this soonish? (and uplifting to 70 early in the 70beta cycle, assuming the patch feels safe)

Comment 2

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #1)

Looks like this is the last thing that we've got blocking the metabug (bug 1573631) for letting these new text features riding the trains with Firefox 70.

Boris or emilio: I think you both know the serialization code pretty well -- could I interest either of you in fixing this soonish? (and uplifting to 70 early in the 70beta cycle, assuming the patch feels safe)

OK. I will take a look next week.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Thanks Boris! Feel free to ni? me if you need me :)

Comment 4

[Boris Chiou [:boris]]

I remember we don't serialize getComputedValue() for all shorthands. The related spec issue is https://github.com/w3c/csswg-drafts/issues/2529, which was resolved last year (but haven't updated the spec yet I guess). That means it's time to fix this issue for all properties.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Ah, sorry, had misread the title. This is expected then, and probably doesn't block shipping the feature.

I thought it was an specified-value thing.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Err, though text-decoration is one of these "shorthands that used to be a longhand", so actually it matters.

We should probably just spot-fix this for now.

Comment 7

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

EXPECTED RESULTS:
For the second and third lines, the text-decoration serialization should match the text-decoration-thickness serialization. (It should say "from-font" and "10px".)

It seems the wpt, https://github.com/web-platform-tests/wpt/blob/master/css/css-text-decor/parsing/text-decoration-computed.html, expects that the serialization of text-decoration contains other longhands, even if we don't specifiy them. So based on it, I would like to make the serialization of text-decoration be something like this:

• if text-decoration-thickness: auto, console.log(getComputedStyle(div).textDecoration) dumps something like: none solid rgb(0, 0, 0) auto.
• if text-decoration-thickness: 10px, console.log(getComputedStyle(div).textDecoration) dumps something like: none solid rgb(0, 0, 0) 10px.

Comment 8

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #7)

(In reply to Daniel Holbert [:dholbert] from comment #0)

EXPECTED RESULTS:
For the second and third lines, the text-decoration serialization should match the text-decoration-thickness serialization. (It should say "from-font" and "10px".)

It seems the wpt, https://github.com/web-platform-tests/wpt/blob/master/css/css-text-decor/parsing/text-decoration-computed.html, expects that the serialization of text-decoration contains other longhands, even if we don't specifiy them. So based on it, I would like to make the serialization of text-decoration be something like this:

• if text-decoration-thickness: auto, console.log(getComputedStyle(div).textDecoration) dumps something like: none solid rgb(0, 0, 0) auto.
• if text-decoration-thickness: 10px, console.log(getComputedStyle(div).textDecoration) dumps something like: none solid rgb(0, 0, 0) 10px.

Please ignore this. I would like to make a PR to update wpt, so we get similar results as those in text-decoration-valid.html.

Comment 9

[Boris Chiou [:boris]]

There is an example:
1.

color: blue;
text-decoration-color: currentColor;
text-decoration-thickness: 10px;

color: blue;
text-decoration-color: blue;
text-decoration-thickness: 10px;

If we only serialize the values we specifiy from gCS(), (1) and (2) would have different results, i.e. 10px v.s. rgb(0, 0, 255) 10px. However, both use blue as text-decoration-color, though (1) uses currentColor.

Looks like we need to always serialize all longhands in text-decoration?

Emilio, do you have any suggestion? Should gCS return all longhands for text-decoration?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I don't think so. Conceptually the process of serializing a shorthand in getComputedStyle should be something like:

• Calling to_resolved_value() on all the values for the properties of the shorthand. This should turn the color as an rgb(a) thing, as colors are always serialized as resolved from getComputedStyle().
• Going from the resolved value al the way to the specified value. This makes the color still an rgba color, not currentColor.
• Serialize the same way we serialize specified shorthands.

So I think we should always serialize the color, because the resolved value (not the computed value) of the color is never currentColor, but probably not all keywords if they're default or such, does that make sense?

Comment 11

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

I don't think so. Conceptually the process of serializing a shorthand in getComputedStyle should be something like:

• Calling to_resolved_value() on all the values for the properties of the shorthand. This should turn the color as an rgb(a) thing, as colors are always serialized as resolved from getComputedStyle().
• Going from the resolved value al the way to the specified value. This makes the color still an rgba color, not currentColor.
• Serialize the same way we serialize specified shorthands.

So I think we should always serialize the color, because the resolved value (not the computed value) of the color is never currentColor, but probably not all keywords if they're default or such, does that make sense?

Yes. color value is the one I concern. The process is really helpful. I'd like to try just alway serialize color for this shorthand. Thanks.

Comment 12

[Boris Chiou [:boris]]

And based on comment 10, the expected result of the attachment is:

auto: [text-decoration: 'rgb(0, 0, 0)', text-decoration-thickness: 'auto']
fromFont: [text-decoration: 'rgb(0, 0, 0) from-font', text-decoration-thickness: 'from-font']
tenPx: [text-decoration: 'rgb(0, 0, 0) 10px', text-decoration-thickness: '10px']

Comment 13

[Boris Chiou [:boris]]

Attachment: Bug 1574222 - Tweak the serialization of text-decoration.

Comment 14

[Boris Chiou [:boris]]

Attachment: Bug 1574222 - Serialize getComputedStyle on text-decoration properly.

The wpt will be updated in
https://github.com/web-platform-tests/wpt/pull/18866.

Comment 15

[Boris Chiou [:boris]]

BTW, I just notice we didn't add tests like text-decoration-thickness-{valid|invalid}.html into css/css-text-decor/parsing/.

Comment 16

[Boris Chiou [:boris]]

Attachment: wpt PR 18866

Comment 17

[Daniel Holbert [:dholbert]]

Once this has landed, please request a beta uplift - thanks!

Comment 18

[Boris Chiou [:boris]]

OK. (Still waiting for the finish of try.)

Comment 19

[Boris Chiou [:boris]]

More test cases should be updated. I will upload an extra patch (for review) which updates those test cases on layout/style/test/ and test expectations on WPT, and then merge it into Bug 1574222 - Serialize gCS on text-decoration properly.

Comment 20

[Boris Chiou [:boris]]

Attachment: Bug 1574222 - Update more mochitest and wpt expectations.

Unfortunatelly, there are some other test cases use text-decoration, so we
have to update them.

I will merge this into the previous patch.

Comment 21

[Boris Chiou [:boris]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7554e13144ef59d972574a99a0d1011a0c43e9b

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc252acb6ee6
Tweak the serialization of text-decoration. r=emilio
https://hg.mozilla.org/integration/autoland/rev/43ce5f2968ea
Serialize getComputedStyle on text-decoration properly. r=emilio,dholbert

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/bc252acb6ee6
https://hg.mozilla.org/mozilla-central/rev/43ce5f2968ea

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 9090851 [details]
Bug 1574222 - Tweak the serialization of text-decoration.

Beta/Release Uplift Approval Request

• User impact if declined: We would like to let text-decoration-thickness ride on the train of 70 beta cycle. If declined, we would fail Bug 1573631.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only change the shorthand serialization, so it should be safe
• String changes made/needed:

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9090851 [details]
Bug 1574222 - Tweak the serialization of text-decoration.

Support for new css specs, let's uplift for beta 7.

Comment 26

[Boris Chiou [:boris]]

Thanks.

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b84d2fe2d2a8
https://hg.mozilla.org/releases/mozilla-beta/rev/c5e6986c6cea