Skip to content

[cssom-1] Serialization for declaration block with variable reference in shorthand may not be useful #2515

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.

Sign up for GitHub

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
Open
upsuper opened this issue Apr 7, 2018 · 3 comments
Open

[cssom-1] Serialization for declaration block with variable reference in shorthand may not be useful #2515

upsuper opened this issue Apr 7, 2018 · 3 comments
Labels
css-variables-1 Current Work cssom-1 Current Work
Milestone
VF2F-2021-02-09 EUR

Comments

@upsuper
Copy link
Member

upsuper commented Apr 7, 2018

The serialization for declaration block with variable reference in shorthand while some longhand is set differently can lead to undesired result, i.e. it would produce non-idempotent result.

For example, for a declaration block like

margin: var(--foo);
margin-top: 10px;

in the current algorithm would be serialized to something like

margin-right: ;
margin-bottom: ;
margin-left: ;
margin-top: 10px;

which apparently doesn't have the same meaning as before. This is unfortunate.

We should probably change the serialization algorithm to that, when we find a longhand which is expanded from some shorthand with variable reference, the shorthand reference is always serialized. If there are longhands that have been changed from the expansion of shorthand (like margin-top in the case above), the longhands are not added to already serialized, so that their value is still preserved and will be serialized later.

The above algorithm should works fine with declaration block from parsing, since the parsing result uses specified order, and thus any update to longhand which was expanded from a shorthand would always come after the shorthand. However, there is currently some problem with CSSOM setters, because in the spec they still set in place rather than append.

For that, we've resolved in #1898 (comment) that setters should append rather than set in place, so this is probably not a problem anyway. We may still want that change to happen first, though.

cc @csnardi
@upsuper upsuper added the cssom-1 Current Work label Apr 7, 2018
@upsuper upsuper mentioned this issue May 10, 2018
Closed
@emilio emilio mentioned this issue Nov 17, 2018
Closed
@upsuper
Copy link
Member Author

upsuper commented Nov 18, 2018

It becomes complicated again that #2924 makes it not append, but just has some constraints...

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 9, 2019
@emilio
Bug 1546983 - followup: use background-color instead of background so…
fe5995b 
… that browser_parsable_css.js doesn't get mad. rs=mconley

It would detect the variable as unreferenced because the background-repeat that
was added on this bug makes the values not serialize, see
w3c/csswg-drafts#2515

CLOSED TREE of course
mykmelez pushed a commit to mykmelez/gecko that referenced this issue May 9, 2019
@emilio
Bug 1546983 - followup: use background-color instead of background so…
490d544 
… that browser_parsable_css.js doesn't get mad. rs=mconley

It would detect the variable as unreferenced because the background-repeat that
was added on this bug makes the values not serialize, see
w3c/csswg-drafts#2515

CLOSED TREE of course
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
@marco-c
Bug 1546983 - followup: use background-color instead of background so…
4c4c7c2 
… that browser_parsable_css.js doesn't get mad. rs=mconley

It would detect the variable as unreferenced because the background-repeat that
was added on this bug makes the values not serialize, see
w3c/csswg-drafts#2515

CLOSED TREE of course

UltraBlame original commit: eb384663078998538a5af4dfa7f45972fe29da49
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
@marco-c
Bug 1546983 - followup: use background-color instead of background so…
3a08927 
… that browser_parsable_css.js doesn't get mad. rs=mconley

It would detect the variable as unreferenced because the background-repeat that
was added on this bug makes the values not serialize, see
w3c/csswg-drafts#2515

CLOSED TREE of course

UltraBlame original commit: eb384663078998538a5af4dfa7f45972fe29da49
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
@marco-c
Bug 1546983 - followup: use background-color instead of background so…
850111b 
… that browser_parsable_css.js doesn't get mad. rs=mconley

It would detect the variable as unreferenced because the background-repeat that
was added on this bug makes the values not serialize, see
w3c/csswg-drafts#2515

CLOSED TREE of course

UltraBlame original commit: eb384663078998538a5af4dfa7f45972fe29da49
@moz-wptsync-bot moz-wptsync-bot mentioned this issue Nov 8, 2020
Merged
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 8, 2020
@emilio @moz-wptsync-bot
Fix declaration block serialization and logical properties.
83053de 
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1675514
gecko-commit: 6dcbbdb84c453149f460f6868eeb135d6622c57d
gecko-reviewers: boris
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 8, 2020
@emilio
Bug 1675514 - Fix declaration block serialization and logical propert…
fe41e97 
…ies. r=boris

As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 8, 2020
@emilio @moz-wptsync-bot
Fix declaration block serialization and logical properties.
0d87c02 
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1675514
gecko-commit: 6dcbbdb84c453149f460f6868eeb135d6622c57d
gecko-reviewers: boris
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Nov 10, 2020
@emilio
Bug 1675514 - Fix declaration block serialization and logical propert…
0280da2 
…ies. r=boris

As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
@tabatkins
Copy link
Member

Commenting to not that legacy shorthands (which serialize to the non-legacy shorthands that replaced them) are a subset of this issue, but are handled slightly differently in CSSOM, so we should make sure they're solved properly at the same time as this.

@astearns astearns added this to the VF2F-2021-02-09 EUR milestone Feb 2, 2021
emilio added a commit to servo/servo that referenced this issue Feb 26, 2021
@emilio
style: Fix declaration block serialization and logical properties.
e1bf164 
As per w3c/csswg-drafts#3030.

Also remove a system font special-case. It's generally handled in
shorthands/font.mako.rs, and when it's not, it's really the same issue
as w3c/csswg-drafts#2515.

Browsers are all over the place for the system font case, so I think
it's fine.

Differential Revision: https://phabricator.services.mozilla.com/D96045
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [cssom-1] Serialization for declaration block with variable reference in shorthand may not be useful.

The full IRC log of that discussion <dael> Topic: [cssom-1] Serialization for declaration block with variable reference in shorthand may not be useful
<dael> github: https://github.com//issues/2515
<dael> Rossen_: Do we have the right people?
<dael> Rossen_: Doubt we have xidorn. Can anyone represent this?
<dael> TabAtkins: This was put on by fantasai, presumably b/c needs to be addressed. not certain we need to address on a call. It's for me and emilio to work on
<dael> Rossen_: I'm fine moving forward if we don't need group consensus.
<dael> TabAtkins: Not yet. May come back later.

@astearns astearns removed the Agenda+ label Mar 24, 2021
@tabatkins tabatkins added the css-variables-1 Current Work label Sep 15, 2021
@cdoublev cdoublev mentioned this issue Mar 7, 2022
Open
@svgeesus svgeesus mentioned this issue Apr 22, 2022
Closed
1 task
This was referenced Oct 31, 2022
Open
Closed
@Loirooriol Loirooriol mentioned this issue Nov 9, 2022
Open
@luxaritas luxaritas mentioned this issue Jan 31, 2023
Open
@kachkaev kachkaev mentioned this issue Jun 8, 2023
Closed
@eoghanmurray eoghanmurray mentioned this issue Aug 17, 2023
Merged
@cdoublev cdoublev mentioned this issue Sep 15, 2023
Open
@eoghanmurray eoghanmurray mentioned this issue Oct 10, 2023
Draft
@spmonahan spmonahan mentioned this issue Jul 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
css-variables-1 Current Work cssom-1 Current Work
Projects
None yet
Milestone
VF2F-2021-02-09 EUR
Development

No branches or pull requests
6 participants
@upsuper @tabatkins @fantasai @astearns @css-meeting-bot and others