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.

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

[css-om][css-backgrounds] Serialization of background: none #8496

Closed
nt1m opened this issue Feb 24, 2023 · 12 comments
Closed

[css-om][css-backgrounds] Serialization of background: none #8496

nt1m opened this issue Feb 24, 2023 · 12 comments
Labels
Closed Accepted by CSSWG Resolution css-backgrounds-3 Current Work cssom-1 Current Work Needs Testcase (WPT)

Comments

@nt1m
Copy link
Member

nt1m commented Feb 24, 2023

Should background: none serialize to background: transparent or background: none ?

Latest STP serializes as transparent, whereas FF serializes as none.

@emilio @Loirooriol Wdyt?
@Loirooriol
Copy link
Contributor

I guess this fits into the umbrella of #8155.
STP serializes as transparent because in WebKit/WebKit#9849 @darinadler interpreted that <'background-color'> appears 1st in grammar order. It's not completely clear, but it seems to me that the 1st one is actually <bg-image>.

@darinadler
Copy link

darinadler commented Feb 24, 2023 •
edited
Loading

The reason you and I came to different conclusions, I suppose, is that background-color is the first item in the final item in the comma separated list of values, and bg-image is the first item in the non-final items. If there is only a single value and no comma, how does the grammar work, and make it valid without a comma, but bg-image counts as first? That’s where we didn’t understand it the same way.

@Loirooriol
Copy link
Contributor

Mm, looking at it again, I'm less sure about it :)
If none is expected I would change the grammar to

<bg-layer>       = <bg-image> || <bg-position> [ / <bg-size> ]? || <repeat-style> || <attachment> || <box> || <box>
<final-bg-layer> = <bg-image> || <bg-position> [ / <bg-size> ]? || <repeat-style> || <attachment> || <box> || <box> || <'background-color'>

If transparent is expected, I would add a note or something.

@nt1m nt1m added the Agenda+ label Feb 27, 2023
@gsnedders
Copy link
Contributor

see also #418 more generally about background serialisation

@darinadler darinadler mentioned this issue Mar 5, 2023
Merged
webkit-early-warning-system pushed a commit to darinadler/WebKit that referenced this issue Mar 6, 2023
@darinadler
REGRESSION (STP 164): new failure in css/css-backgrounds/parsing/back…
bec59bd 
…ground-shorthand-serialization.html

https://bugs.webkit.org/show_bug.cgi?id=252925
rdar://problem/106208628

Reviewed by Tim Nguyen.

Revert the change from https://commits.webkit.org/260157@main that made background-color
serialize first. This is being discussed in w3c/csswg-drafts#8496,
but while it's discussed, it seems good to change the behavior back since that matches
other browsers and some WPT test expectations.

* LayoutTests/editing/deleting/paste-with-transparent-background-color-expected.txt:
Change expectations back to expect background-color to be serialized last, after the other
longhands that are part of the background shorthand
* LayoutTests/editing/deleting/paste-with-transparent-background-color-live-range-expected.txt: Ditto.
* LayoutTests/fast/backgrounds/background-shorthand-after-set-backgroundSize-expected.txt: Ditto.
* LayoutTests/fast/backgrounds/background-shorthand-after-set-backgroundSize.html: Ditto.
* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt: Ditto.
* LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html: Ditto.
* LayoutTests/fast/css/remove-shorthand-expected.txt: Ditto.
* LayoutTests/fast/dom/background-shorthand-csstext-expected.txt: Ditto.
* LayoutTests/fast/dom/background-shorthand-csstext.html: Ditto.

* Source/WebCore/css/CSSProperties.json: Move background-color to the end of the list of longhands
for the background shorthand.

Canonical link: https://commits.webkit.org/261250@main
@css-meeting-bot
Copy link
Member

css-meeting-bot commented Mar 8, 2023 •
edited by fantasai
Loading

The CSS Working Group just discussed [css-om][css-backgrounds] Serialization of `background: none` , and agreed to the following:

  • RESOLVED: Move color at the end of the final-bg-layer grammar, to make it serialize as `none`
The full IRC log of that discussion <emilio> TabAtkins: question is how does `background: none` serialize. Using shortest serialization and grammar is unclear
<emilio> ... because if you only specify one layer it matches final-background-layer and image-layer
<emilio> ... and since color is the first in the final-background-layer safari they serialize transparent
<emilio> ... other browsers serialize none
<emilio> ... proposal is to move color to the end of final-bg-layer
<emilio> ... which makes it serialize as none
<emilio> fantasai: I think it's great because it moves color to the end
<emilio> ... which makes sense because it's at the bottom
<emilio> ... but that would be a change to how a color + image serializes, and I'm not sure that's Web-compatible <emilio> RESOLVED: Move color at the end of the final-bg-layer grammar, to make it serialize as `none`

@mozilla-apprentice mozilla-apprentice mentioned this issue Mar 8, 2023
Closed
@chromium-helper chromium-helper mentioned this issue Mar 8, 2023
Closed
@tabatkins
Copy link
Member

Not captured in the minutes - we're hoping this is web-compatible, since currently if you specify a color and an image, the color will serialize first.

@darinadler
Copy link

Not captured in the minutes - we're hoping this is web-compatible, since currently if you specify a color and an image, the color will serialize first.

Not sure that’s quite right: The specification says that, but do any web browsers currently do that?

@Loirooriol
Copy link
Contributor

Loirooriol commented Mar 8, 2023 •
edited
Loading

About @fantasai's comment:

var {style} = document.body;
style.background = 'url("") blue';
style.background;

Gecko: blue url("")
Blink: url("") blue
WebKit: url("") blue (or blue url("") between WebKit/WebKit@5d2cf87 and WebKit/WebKit@bec59bd)

@tabatkins
Copy link
Member

Okay, Blink (and WK before this recent change) serializing color after makes me very confident it's web-compatible.

tabatkins added a commit that referenced this issue Mar 9, 2023
@tabatkins
[css-backgrounds-3] Move <bg-color> to the end of the <final-bg-layer…
d0fba52 
…> component. #8496
@tabatkins
Copy link
Member

k, edit in. Color is now last in the final layer, so a none serializes consistently in all layers as none.

@SebastianZ
Copy link
Contributor

Just to provide a little background why the color was put first in the final layer:

So this was initially pushed by me back then. Though I agree it makes sense to have the color at the end for the same reason @fantasai and @LeaVerou gave now and in the past.

Sebastian

@fantasai
Copy link
Collaborator

@tabatkins Don't just edit and close issues on specs that aren't yours. This needs to go into the Changes list, for example, which you didn't remember to do.

fantasai added a commit that referenced this issue Mar 14, 2023
@fantasai
[css-backgrounds-3] Update Changes #8496
6aa105a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Closed Accepted by CSSWG Resolution css-backgrounds-3 Current Work cssom-1 Current Work Needs Testcase (WPT)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@gsnedders @tabatkins @fantasai @SebastianZ @astearns @nt1m @Loirooriol @css-meeting-bot @darinadler