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

[cssom] Adjacent space when serializing '/' #4282

Closed
ewilligers opened this issue Sep 7, 2019 · 7 comments
Closed

[cssom] Adjacent space when serializing '/' #4282

ewilligers opened this issue Sep 7, 2019 · 7 comments
Labels
cssom-1

Comments

@ewilligers
Copy link
Contributor

Each browser has variation in whether or not they include whitespace when serializing a '/' in shorthands (example):

Blink:
No spaces for specified font.
Spaces for resolved border-radius, font, grid, grid-template.

Edge 18:
No spaces for specified font.
Spaces for specified border-radius.

Firefox:
No spaces for specified font.
Spaces for specified border-radius, grid, grid-template, mask.

Safari:
No spaces for specified or resolved font.
Spaces for resolved border-radius, grid, grid-template.

Relevant spec: https://drafts.csswg.org/cssom/#serializing-css-values
@emilio
Copy link
Collaborator

emilio commented Sep 8, 2019

Yikes, what a mess...

I think if possible we should try to use spaces everywhere. Tough given everyone agrees on "no spaces for specified fonts" we may want to special-case that one out. WDYT?

@tabatkins
Copy link
Member

All of the cases are equivalent in parsing; so long as you don't have a * before or after the /, a / will always parse as a DELIM token.

For readability's sake, I do agree with always doing spaces. We can special-case 'font' if necessary, but I'd rather try for consistency first and special-case only if there are bugs.

@emilio
Copy link
Collaborator

emilio commented Sep 12, 2019

Sure, I'm happy to make the change in Gecko if Blink is willing to do that too.

@ewilligers
Copy link
Contributor Author

I would like gCS results to round-trip as specified values.

Changing Blink and WPTs for font shorthand to include spaces: https://chromium-review.googlesource.com/c/chromium/src/+/1800894

Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 12, 2019
@ericwilligers @chromium-wpt-export-bot
CSS: Serialize font shorthand with spaces around '/'
414eec1 
Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 12, 2019
@ericwilligers @chromium-wpt-export-bot
CSS: Serialize font shorthand with spaces around '/'
6f894a3 
Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 13, 2019
@ericwilligers @chromium-wpt-export-bot
CSS: Serialize font shorthand with spaces around '/'
99c1915 
Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eae@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696522}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 13, 2019
@ericwilligers @chromium-wpt-export-bot
CSS: Serialize font shorthand with spaces around '/'
5488ba4 
Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eae@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696522}
@emilio
Copy link
Collaborator

emilio commented Sep 13, 2019

I have a patch to Gecko to update to this once WPT gets upstreamed: https://hg.mozilla.org/try/rev/1988d5f527709c4583d4b5315c50c65337fc6101

Note that another place where this character exists (in media queries for now) is <ratio>. It seems like Chrome and Firefox serialize it without spaces (matchMedia("(aspect-ratio: 1/3)").media), but I'd change both.

aarongable pushed a commit to chromium/chromium that referenced this issue Sep 13, 2019
@ericwilligers
CSS: Serialize font shorthand with spaces around '/'
5352723 
Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eae@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696522}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 18, 2019
@ericwilligers @moz-wptsync-bot
Bug 1580847 [wpt PR 19025] - CSS: Serialize font shorthand with space…
1684456 
…s around '/', a=testonly

Automatic update from web-platform-tests
CSS: Serialize font shorthand with spaces around '/'

Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eae@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696522}

--

wpt-commits: 5488ba49034a78c2fdfe7ca0d898ed3adb238f6b
wpt-pr: 19025
dbaron pushed a commit to dbaron/gecko that referenced this issue Sep 19, 2019
@ericwilligers @moz-wptsync-bot
Bug 1580847 [wpt PR 19025] - CSS: Serialize font shorthand with space…
35cdc4d 
…s around '/', a=testonly

Automatic update from web-platform-tests
CSS: Serialize font shorthand with spaces around '/'

Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eae@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696522}

--

wpt-commits: 5488ba49034a78c2fdfe7ca0d898ed3adb238f6b
wpt-pr: 19025
emilio added a commit that referenced this issue Sep 20, 2019
@emilio
[cssom] Specify the serialization of <ratio>.
afe53a0 
Part of #4282
emilio added a commit that referenced this issue Sep 20, 2019
@emilio
[cssom] Specify the serialization of <ratio>.
5539796 
Part of #4282
@emilio
Copy link
Collaborator

emilio commented Sep 20, 2019

Landing that in https://bugzilla.mozilla.org/show_bug.cgi?id=1582627.

@ewilligers
Copy link
Contributor Author

Closing as we have consensus.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
@marco-c
Bug 1580847 [wpt PR 19025] - CSS: Serialize font shorthand with space…
3e47f65 
…s around '/', a=testonly

Automatic update from web-platform-tests
CSS: Serialize font shorthand with spaces around '/'

Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eaechromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Emil A Eklund <eaechromium.org>
Cr-Commit-Position: refs/heads/master{#696522}

--

wpt-commits: 5488ba49034a78c2fdfe7ca0d898ed3adb238f6b
wpt-pr: 19025

UltraBlame original commit: 4997dd1d31994f9f407e4a2f3035489311305ff4
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 1580847 [wpt PR 19025] - CSS: Serialize font shorthand with space…
767eadf 
…s around '/', a=testonly

Automatic update from web-platform-tests
CSS: Serialize font shorthand with spaces around '/'

Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eaechromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Emil A Eklund <eaechromium.org>
Cr-Commit-Position: refs/heads/master{#696522}

--

wpt-commits: 5488ba49034a78c2fdfe7ca0d898ed3adb238f6b
wpt-pr: 19025

UltraBlame original commit: 4997dd1d31994f9f407e4a2f3035489311305ff4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
@marco-c
Bug 1580847 [wpt PR 19025] - CSS: Serialize font shorthand with space…
8c3c56c 
…s around '/', a=testonly

Automatic update from web-platform-tests
CSS: Serialize font shorthand with spaces around '/'

Each '/' is a CSS value should have a space before and after.
w3c/csswg-drafts#4282

Omit 'normal' values from computed font serialization.

Change-Id: Iebbfe80ae0c7c3ebe0101b432c0d95a7d265154f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800894
Commit-Queue: Emil A Eklund <eaechromium.org>
Auto-Submit: Eric Willigers <ericwilligerschromium.org>
Reviewed-by: Emil A Eklund <eaechromium.org>
Cr-Commit-Position: refs/heads/master{#696522}

--

wpt-commits: 5488ba49034a78c2fdfe7ca0d898ed3adb238f6b
wpt-pr: 19025

UltraBlame original commit: 4997dd1d31994f9f407e4a2f3035489311305ff4
@karlcow karlcow mentioned this issue Oct 15, 2020
Closed
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Oct 16, 2020
@emilio
[mediaqueries] Serialize ratios in media queries consistently.
9a5cddd 
Per the discussion in w3c/csswg-drafts#4282
they should serialize with spaces around the slash.

Fixed: 1138849
Change-Id: I4a1b2f740db885fb85cf2b27ede3ce530b44b3e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2474698
Commit-Queue: Emilio Cobos Álvarez <emilio@chromium.org>
Auto-Submit: Emilio Cobos Álvarez <emilio@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817882}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@emilio
[mediaqueries] Serialize ratios in media queries consistently.
0aa8531 
Per the discussion in w3c/csswg-drafts#4282
they should serialize with spaces around the slash.

Fixed: 1138849
Change-Id: I4a1b2f740db885fb85cf2b27ede3ce530b44b3e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2474698
Commit-Queue: Emilio Cobos Álvarez <emilio@chromium.org>
Auto-Submit: Emilio Cobos Álvarez <emilio@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817882}
GitOrigin-RevId: 9a5cddde0a5d24b7a33041a5b0b29eca8eb96974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cssom-1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@tabatkins @ewilligers @emilio