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-backgrounds] Change canonical serialization for box-shadow #2305

Closed
csnardi opened this issue Feb 12, 2018 · 7 comments
Closed

[css-backgrounds] Change canonical serialization for box-shadow #2305

csnardi opened this issue Feb 12, 2018 · 7 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-backgrounds-3 Current Work Tested Memory aid - issue has WPT tests

Comments

@csnardi
Copy link
Contributor

csnardi commented Feb 12, 2018

The spec declares the syntax for a shadow in box-shadow to be:

<shadow> = inset? && <length>{2,4} && <color>?

However, Firefox, WebKit, and Chrome all serialize this as <color>? && <length>{2,4} && inset?. This matches the serialization for drop-shadow() (see https://drafts.fxtf.org/filter-effects/#funcdef-filter-drop-shadow) and text-shadow (see https://drafts.csswg.org/css-text-decor-3/#text-shadow-property), which both are declared as <color>? && <length>{2,3}.

For consistency, <shadow> should be defined as:

<shadow> = <color>? && <length>{2,4} && inset?

in order for serialization to match 3 out of the 4 major browsers and text-shadow/drop-shadow().
fantasai added a commit that referenced this issue Feb 14, 2018
@fantasai
[css-backgrounds-3] Match grammar of <<shadow>> to that of text-shado…
2d702bb 
…w/drop-shadow() and implementations. #2305
@fantasai
Copy link
Collaborator

Pushed out tentative edits, waiting for WG approval.

csnardi added a commit to csnardi/web-platform-tests that referenced this issue Feb 14, 2018
@csnardi
Test box-shadow serialization
b5e246e 
Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
@csnardi csnardi mentioned this issue Feb 14, 2018
Merged
csnardi added a commit to csnardi/web-platform-tests that referenced this issue Feb 14, 2018
@csnardi
Test box-shadow serialization
8e9f916 
Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
@css-meeting-bot
Copy link
Member

The Working Group just discussed [css-backgrounds] Change canonical serialization for box-shadow, and agreed to the following resolutions:

  • RESOLVED: switch the order to match current implementations
The full IRC log of that discussion <dael> Topic: [css-backgrounds] Change canonical serialization for box-shadow
<dael> github: https://github.com//issues/2305
<dael> TabAtkins: Going by standard rule that default serialization should match grammar that implies color after link but impl oes color before links. We should change grammer to match impl.
<dael> Rossen_: Looks like edits were pushed out. So only thing we need to do is get consensus.
<dael> Rossen_: Any other opinions as to why we shouldn't do this?
<dbaron> This is part of why I am not a fan of that general rule being imposed without doing compatibility testing first... although I think we may have rolled back the rule a bit?
<bradk> +1
<AmeliaBR> +1 to matching reality & also other properties
<dael> Rossen_: Objections to switch the order to match current impl?
<dael> RESOLVED: switch the order to match current implementations

csnardi added a commit to csnardi/web-platform-tests that referenced this issue Mar 4, 2018
@csnardi
Correct serialization of box-shadow and text-shadow
cc00e2e 
Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.
@csnardi csnardi mentioned this issue Mar 4, 2018
Merged
@csnardi
Copy link
Contributor Author

csnardi commented Mar 7, 2018

@fantasai I created two PRs for tests that should verify this change; they should be linked to this issue.

@TalbotG
Copy link
Collaborator

TalbotG commented Mar 13, 2018

@csnardi If you need more tests, you know that I have 15 tests targeting box-shadow in

http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/

http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/box-shadow-021.xht
...
http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/box-shadow-036.xht

@csnardi
Copy link
Contributor Author

csnardi commented Mar 13, 2018

@TalbotG Thanks, I didn't know that! I'll take a look at them a little bit later and add in any parsing cases I missed to the first pull request linked above.

@csnardi
Copy link
Contributor Author

csnardi commented Apr 6, 2018

@fantasai Friendly reminder if you have some time for reviewing web-platform-tests/wpt#9800 and web-platform-tests/wpt#9519 which should close out this issue.

fantasai pushed a commit to web-platform-tests/wpt that referenced this issue Apr 9, 2018
@csnardi @fantasai
Correct serialization of box-shadow and text-shadow
a835486 
Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.
gsnedders pushed a commit to web-platform-tests/wpt that referenced this issue Apr 10, 2018
@csnardi @gsnedders
Test box-shadow serialization (#9519)
5a93530 
Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 15, 2018
@csnardi @jgraham
Bug 1443103 [wpt PR 9800] - Correct serialization of box-shadow and t…
02d309b 
…ext-shadow, a=testonly

Automatic update from web-platform-testsCorrect serialization of box-shadow and text-shadow

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.

wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800
wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 20, 2018
@csnardi @jgraham
Bug 1438276 [wpt PR 9519] - Test box-shadow serialization, a=testonly
c30053e 
Automatic update from web-platform-testsTest box-shadow serialization (#9519)

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
--

wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519
wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519
@csnardi csnardi closed this as completed Apr 23, 2018
@csnardi
Copy link
Contributor Author

csnardi commented Apr 23, 2018

I think the needs testcase label can be removed now.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
@marco-c
Bug 1443103 [wpt PR 9800] - Correct serialization of box-shadow and t…
866a8b1 
…ext-shadow, a=testonly

Automatic update from web-platform-testsCorrect serialization of box-shadow and text-shadow

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.

wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800
wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800

UltraBlame original commit: 0b20cccaed6e6890823b8daad46167e0c89f8667
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
@marco-c
Bug 1443103 [wpt PR 9800] - Correct serialization of box-shadow and t…
73a06ee 
…ext-shadow, a=testonly

Automatic update from web-platform-testsCorrect serialization of box-shadow and text-shadow

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.

wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800
wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800

UltraBlame original commit: 0b20cccaed6e6890823b8daad46167e0c89f8667
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
@marco-c
Bug 1438276 [wpt PR 9519] - Test box-shadow serialization, a=testonly
e7f1dc2 
Automatic update from web-platform-testsTest box-shadow serialization (#9519)

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
--

wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519
wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519

UltraBlame original commit: 897ebf44f505010b0be8d4773368bc926c32315e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
@marco-c
Bug 1443103 [wpt PR 9800] - Correct serialization of box-shadow and t…
8117255 
…ext-shadow, a=testonly

Automatic update from web-platform-testsCorrect serialization of box-shadow and text-shadow

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed to the color then lengths. https://drafts.csswg.org/css-text-decor-3/#text-shadow-property also defines the canonical serialization for text-shadow as the color and then lengths. This caused the test to fail in Firefox, Chrome, and Safari. Update the test to match the spec in both instances.

wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800
wpt-commits: a835486e59a94236a55107fe34925079b33ef247
wpt-pr: 9800

UltraBlame original commit: 0b20cccaed6e6890823b8daad46167e0c89f8667
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
@marco-c
Bug 1438276 [wpt PR 9519] - Test box-shadow serialization, a=testonly
2b4041f 
Automatic update from web-platform-testsTest box-shadow serialization (#9519)

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
--

wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519
wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519

UltraBlame original commit: 897ebf44f505010b0be8d4773368bc926c32315e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
@marco-c
Bug 1438276 [wpt PR 9519] - Test box-shadow serialization, a=testonly
91891c6 
Automatic update from web-platform-testsTest box-shadow serialization (#9519)

Following w3c/csswg-drafts#2305, the canonical serialization for box-shadow was changed from inset? && <length>{2,4} && <color>? to <color>? && <length>{2,4} && inset?. Convert an existing box-shadow text into a testharness.js test in order to test serialization as well.
--

wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519
wpt-commits: 5a93530d4db27df7a3154d9a5b75a0f36d592c6a
wpt-pr: 9519

UltraBlame original commit: 897ebf44f505010b0be8d4773368bc926c32315e
@fantasai fantasai added Tested Memory aid - issue has WPT tests Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Dec 16, 2020
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 Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-backgrounds-3 Current Work Tested Memory aid - issue has WPT tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fantasai @astearns @csnardi @css-meeting-bot @TalbotG