Closed Bug 1574222 Opened 4 months ago Closed 3 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: dholbert, Assigned: boris)

References

(Blocks 2 open bugs, )

Details

Attachments

(4 files, 1 obsolete file)

Attached file 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...

Attachment #9085809 - Attachment description: text-decor.htmltestcase 1 → testcase 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)

(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.

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

Assignee: nobody → boris.chiou

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.

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.

No longer blocks: 1573631
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 137688

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.

Blocks: 1573631
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
See Also: → 137688
Status: REOPENED → ASSIGNED

(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.

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?

Flags: needinfo?(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?

Flags: needinfo?(emilio)

(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.

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']

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

Attached file wpt PR 18866

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

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

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.

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

I will merge this into the previous patch.

Attachment #9090852 - Attachment description: Bug 1574222 - Serialize gCS on text-decoration properly. → Bug 1574222 - Serialize getComputedStyle on text-decoration properly.
Attachment #9091621 - Attachment is obsolete: true
Blocks: 1580343
Pushed by bchiou@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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:
Attachment #9090851 - Flags: approval-mozilla-beta?
Attachment #9090852 - Flags: approval-mozilla-beta?

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

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

Attachment #9090851 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9090852 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Thanks.

You need to log in before you can comment on or make changes to this bug.