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

Jump to bottom

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

Merged
merged 2 commits into from Sep 10, 2019
Merged

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

merged 2 commits into from Sep 10, 2019

Conversation

BorisChiou
Copy link
Member

@BorisChiou BorisChiou commented Sep 5, 2019 •
edited

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

@ericwilligers This may need your review.

cc @emilio

@ewilligers
Copy link
Contributor

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

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
Copy link
Member Author

Updated the description.

@ewilligers
Copy link
Contributor

ewilligers commented Sep 7, 2019 •
edited

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 commented Sep 9, 2019 •
edited

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.

ewilligers
ewilligers approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@ewilligers ewilligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
[css-text-decor-4] Fix the expected serialization of text-decoration.
e86d3fd 
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
[css-text-decor-4] Fix text-decoration-serialization.tentative.html a…
a179204 
…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
Copy link
Member Author

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
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 11, 2019
@BorisChiou
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
4db87c1 
… 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
@BorisChiou
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
2e63f84 
… 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 text_decoration/computed_value branch September 13, 2019 01:00
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 15, 2019
@BorisChiou
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
bc8e897 
… 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
@BorisChiou
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
8825c43 
… 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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
bc00d5c 
… 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

UltraBlame original commit: 43ce5f2968ea046eeea78089e4f80efc09c8fb8a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
@marco-c
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
23536c6 
… 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

UltraBlame original commit: 43ce5f2968ea046eeea78089e4f80efc09c8fb8a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
@marco-c
Bug 1574222 - Serialize getComputedStyle on text-decoration properly.…
f67239c 
… 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

UltraBlame original commit: 43ce5f2968ea046eeea78089e4f80efc09c8fb8a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewilligers ewilligers ewilligers approved these changes

@fantasai fantasai Awaiting requested review from fantasai

@kojiishi kojiishi Awaiting requested review from kojiishi

@dbaron dbaron Awaiting requested review from dbaron

@svgeesus svgeesus Awaiting requested review from svgeesus

@tabatkins tabatkins Awaiting requested review from tabatkins

Assignees

@kojiishi kojiishi

Labels
css-text-decor css-variables wg-css
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@BorisChiou @ewilligers @kojiishi @emilio @wpt-pr-bot