Repository navigation
Values set by mask and background shorthands should not serialize as "initial" - #9849
Conversation
|
EWS run on previous version of this PR (hash 620a5ab) Details
|
620a5ab to
babcc53
Compare
|
EWS run on previous version of this PR (hash babcc53) Details
|
nt1m
left a comment
There was a problem hiding this comment.
Looks good! Can you export the WPT changes and check the cross browser results in the checks tab on the WPT PR?
babcc53 to
9e49d81
Compare
|
EWS run on current version of this PR (hash 9e49d81) Details
|
I exported the changes in web-platform-tests/wpt#38456 but I don’t really know how to check the results for other browsers; need someone to show me how, I guess. |
|
Interesting, Firefox serializes Chrome seems to keep serializing with initial which is wrong. Here's the Firefox diff: https://wpt.fyi/results/css/css-backgrounds/parsing/background-valid.html?diff&filter=ADC&run_id=5146658350825472&run_id=6193847596482560 Seems good to go! |
…"initial" https://bugs.webkit.org/show_bug.cgi?id=251838 rdar://problem/105114588 Reviewed by Tim Nguyen. Needed to expand the function that serializes longhands to cover this case. Also fixed serialization of the background shorthand to more closely match the specification and a mistake in the parser that allowed invalid background shorthand values involving sizes without positions. * LayoutTests/editing/deleting/paste-with-transparent-background-color-expected.txt: * LayoutTests/editing/deleting/paste-with-transparent-background-color-live-range-expected.txt: Updated for progression where we no longer add "background-image: none" as a side effect. * LayoutTests/fast/backgrounds/background-shorthand-after-set-backgroundSize-expected.txt: * LayoutTests/fast/backgrounds/background-shorthand-after-set-backgroundSize.html: * LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-expected.txt: * LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html: * LayoutTests/fast/css/remove-shorthand-expected.txt: * LayoutTests/fast/dom/background-shorthand-csstext-expected.txt: * LayoutTests/fast/dom/background-shorthand-csstext.html: Updated to assume background-color will be serialized first, as specified. * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-invalid-expected.txt: Updated to expect PASS for the test of parsing a size without a position. * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-valid-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-backgrounds/parsing/background-valid.html: Added tests of the longhand values. Removed result that allowed WebKit's old incorrect serialization. * LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-valid.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-valid.sub.html: * LayoutTests/platform/glib/imported/w3c/web-platform-tests/css/css-masking/parsing/mask-valid.sub-expected.txt: Added tests of the longhand values. * Source/WebCore/css/CSSProperties.json: Moved background-color to the start of the background shorthand so serialization matches the specification. * Source/WebCore/css/StyleProperties.cpp: (WebCore::serializeLonghandValue): Added special case for the lists that are used for the mask and background longhands, because the parser puts placeholders in there when parsing the shorthands. * Source/WebCore/css/parser/CSSPropertyParser.cpp: (WebCore::CSSPropertyParser::consumeBackgroundShorthand): Only parse size immediately after a position. The old code required that it be after a position, but not immediately after. Removed comment saying this makes "invalid longhands". They are now valid, and the serializeLonghandValue function above makes them serialize correctly. Canonical link: https://commits.webkit.org/260157@main
9e49d81 to
5d2cf87
Compare
|
Committed 260157@main (5d2cf87): https://commits.webkit.org/260157@main Reviewed commits have been landed. Closing PR #9849 and removing active labels. |
| "background": { | ||
| "codegen-properties": { | ||
| "longhands": [ | ||
| "background-color", |
There was a problem hiding this comment.
Why move this? The spec says "canonical order: per grammar", and backgrfound-color only appears at the end in the <final-bg-layer>.
There was a problem hiding this comment.
If there's only one background-image, then background-color does appear first. The spec is just saying you can't mention multiple background-colors in the shorthand (which makes sense).
I think this deserves some clarification?
There was a problem hiding this comment.
Well, even if there is only a single layer, <bg-layer> will be omitted but it's still in the grammar, so background-color appears last. I agree that "canonical order: per grammar" may not be much clear in these complex cases, but I don't see what backs this:
Moved background-color to the start of the background shorthand so serialization matches the specification.
There was a problem hiding this comment.
https://www.w3.org/TR/css-backgrounds-3/#typedef-final-bg-layer lists the color first. That is where I got the idea from. Maybe I am looking in the wrong place.
There was a problem hiding this comment.
I see your point now.
I’d be happy to move background-color back to the end. I suggest we try to resolve this in w3c/csswg-drafts#418 and then change to match at that time. Whoever wrote the WPT test apparently read the specification the same way I did.
There was a problem hiding this comment.
Note that the WebKit parser does not parse a trailing color as a separate final background layer, nor should it. So while the grammar states something like that, it’s not at all consistent with how the values are interpreted. Anyway, I’m willing to change back.
5d2cf87
9e49d81
🧪 mac-wk1