Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-text-decor-4] Fix the expected serialization of text-decoration. #18866

Merged

Conversation

@BorisChiou
Copy link
Member

@BorisChiou BorisChiou commented Sep 5, 2019

We add some test cases for text-decoration-thickness in text-decoration.

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 we always serizlize text-decoration-color because it is impossible to be currentColor. For other keywords, if they are initial values, we omit them.

@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 5, 2019

@ericwilligers This may need your review.

cc @emilio

@ewilligers
Copy link
Contributor

@ewilligers ewilligers commented Sep 5, 2019

Are you suggesting the serialization of text-decoration from gCS should be different in the following two cases? Is there a precedent?

color: blue;
text-decoration-style: dotted;
text-decoration-color: currentColor;
color: blue;
text-decoration-style: dotted;
text-decoration-color: blue;
@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 5, 2019

Are you suggesting the serialization of text-decoration from gCS should be different in the following two cases? Is there a precedent?

This is a good example. I didn't notice any precedent, at least now. Emilio suggests to me that it'd be better to always serialize color because its resolved value is never currentColor. For other keywords, if they are default values, we omit them. I will update this patch to let you review again. Thanks for the question and example.

@BorisChiou BorisChiou force-pushed the BorisChiou:text_decoration/computed_value branch from 9e5b12c to 32c6de3 Sep 5, 2019
@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 5, 2019

Updated the description.

@ewilligers
Copy link
Contributor

@ewilligers ewilligers commented Sep 7, 2019

Serialize the same way we serialize specified shorthands.

Are you saying that for the specified value, none and solid should be omitted whenever a color is specified, or are none and solid always omitted, and currentcolor added if the result would otherwise be empty? For example, how should a specified value of none solid be serialized?

@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 9, 2019

Sorry for the confusing description. I reply these below.

Are you saying that for the specified value, none and solid should be omitted whenever a color is specified

For specified shorthand, we omit none and solid if the color is not currentcolor. For example:

div.style.textDecoration = "none rgb(0, 0, 255)";
console.log(div.style.textDecoration); // output is "rgb(0, 0, 255)"

div.style.textDecoration = "solid rgb(0, 0, 255)";
console.log(div.style.textDecoration); // output is "rgb(0, 0, 255)"

or are none and solid always omitted, and currentcolor added if the result would otherwise be empty? For example, how should a specified value of none solid be serialized?

For example:

div.style.textDecoration = "none solid";
console.log(div.style.textDecoration); // output is "none"

div.style.textDecoration = "none solid currentcolor";
console.log(div.style.textDecoration); // output is "none"

In this example, all keywords, including color, are default value, so we just serialize it as none.

Sorry, we landed this change into text-decoration-valid.html before discussing with you. Perhaps it's worth to write it down in the spec.

gCS() is a different case. currentcolor is always converted into an rgb value, so

div.style.textDecoration = "none solid currentcolor";
console.log(getComputedStyle(div).textDecoration);
// output is "rgb(0, 0, 255)", assume currentcolor is blue.

Note: I intend to land the serialization for gCS() on text-decoration in mozilla bug 1574222.

Copy link
Contributor

@ewilligers ewilligers left a comment

Do the specs specify where text-decoration-thickness appears in the text-decoration shorthand?

I only see "This property, which is also a sub-property of the text-decoration shorthand, ..."

@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 9, 2019

Do the specs specify where text-decoration-thickness appears in the text-decoration shorthand?

I only see "This property, which is also a sub-property of the text-decoration shorthand, ..."

I think the answer is no. This is another spec issue, so I need to file one for this. Anyway, I put it in the last one in the text-decoration shorthand for now.

BTW, I found there are some other test cases also need to be updated. I will upload an extra patch for review. Thanks.

BorisChiou added 2 commits Sep 5, 2019
We add some test cases for `text-decoration-thickness` in `text-decoration`.

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 we always serizlize `text-decoration-color` because it is impossible to be
`currentColor`. For other keywords, if they are initial values, we omit them.
…nd variable-presentation-attribute.html.

text-decoration-serialization should be updated because we would like to
update the serialization of text-decoration.

variable-presentation-attribute.html tests CSS variable and SVG
presentation attributes. In SVG2, the text decoration is determined
respectively by the text-decoration-line and text-decoration-style, so we
test these two longhands instead, to avoid hitting the change of the
serialization of text-decoration.
@BorisChiou BorisChiou force-pushed the BorisChiou:text_decoration/computed_value branch from a588ae2 to a179204 Sep 10, 2019
@BorisChiou
Copy link
Member Author

@BorisChiou BorisChiou commented Sep 10, 2019

Rebased and try to rerun tests because we hit the Github API issue: #18704

@emilio emilio merged commit 28cd992 into web-platform-tests:master Sep 10, 2019
11 checks passed
11 checks passed
update-pr-preview
Details
Azure Pipelines Build #20190910.149 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 11, 2019
… r=emilio,dholbert

The wpt will be updated in
web-platform-tests/wpt#18866.

Besides, there are some other test cases use text-decoration, so we
have to update them as well. The rule is: if the test case is not related to
old `text-decoration` longhand, we use `text-decoration-line` instead.
If the test case is for testing the change of `text-decoration` from
longhand to shorthand, we should use the correct serialization.

Differential Revision: https://phabricator.services.mozilla.com/D44909

--HG--
extra : moz-landing-system : lando
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Sep 11, 2019
… r=emilio,dholbert

The wpt will be updated in
web-platform-tests/wpt#18866.

Besides, there are some other test cases use text-decoration, so we
have to update them as well. The rule is: if the test case is not related to
old `text-decoration` longhand, we use `text-decoration-line` instead.
If the test case is for testing the change of `text-decoration` from
longhand to shorthand, we should use the correct serialization.

Differential Revision: https://phabricator.services.mozilla.com/D44909
@BorisChiou BorisChiou deleted the BorisChiou:text_decoration/computed_value branch Sep 13, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 15, 2019
… r=emilio,dholbert a=lizzard

The wpt will be updated in
web-platform-tests/wpt#18866.

Besides, there are some other test cases use text-decoration, so we
have to update them as well. The rule is: if the test case is not related to
old `text-decoration` longhand, we use `text-decoration-line` instead.
If the test case is for testing the change of `text-decoration` from
longhand to shorthand, we should use the correct serialization.

Differential Revision: https://phabricator.services.mozilla.com/D44909

--HG--
extra : source : 43ce5f2968ea046eeea78089e4f80efc09c8fb8a
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Sep 16, 2019
… r=emilio,dholbert a=lizzard

The wpt will be updated in
web-platform-tests/wpt#18866.

Besides, there are some other test cases use text-decoration, so we
have to update them as well. The rule is: if the test case is not related to
old `text-decoration` longhand, we use `text-decoration-line` instead.
If the test case is for testing the change of `text-decoration` from
longhand to shorthand, we should use the correct serialization.

Differential Revision: https://phabricator.services.mozilla.com/D44909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.