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.

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

[motion-1] Fix offset-path parsing test #340

Closed
danielsakhapov opened this issue May 22, 2023 · 17 comments
Closed

[motion-1] Fix offset-path parsing test #340

danielsakhapov opened this issue May 22, 2023 · 17 comments
Labels
focus area: Motion Path test-change-proposal Proposal to add or remove tests for an interop area

Comments

@danielsakhapov
Copy link

Test List

https://wpt.fyi/results/css/motion/parsing/offset-path-parsing-valid.html

Rationale

Update tests to be up-to-date with the spec
web-platform-tests/wpt#40120

@danielsakhapov danielsakhapov added the test-change-proposal Proposal to add or remove tests for an interop area label May 22, 2023
@danielsakhapov
Copy link
Author

@BorisChiou @nmoucht

@BorisChiou
Copy link
Member

Thanks. The parsing test and computed value test are out-of-date of course. I'm also trying to add more tests for shapes/coord-box in parsing-valid and computed tests.

@nt1m
Copy link
Member

nt1m commented May 23, 2023

The margin-box one looks OK. I'm not sure about the omitting the position part. @emilio @Loirooriol, should we always serialize the position here?

@BorisChiou
Copy link
Member

About the serialization, the current spec requires us to serialize <position> always, https://drafts.csswg.org/css-shapes-1/#basic-shape-serialization. However, there is a spec issue talking about this: w3c/csswg-drafts#8695.

@Loirooriol
Copy link

https://drafts.csswg.org/css-shapes-1/#funcdef-basic-shape-ellipse

The position argument defines the center of the ellipse. This defaults to center if omitted.

https://drafts.csswg.org/css-backgrounds-3/#valdef-background-position-center

center: Computes to 50%

https://drafts.csswg.org/cssom/#serializing-css-values

If component values can be omitted or replaced with a shorter representation without changing the meaning of the value, omit/replace them.

So it seems to me that ellipse(50% 60%) should NOT serialize as ellipse(50% 60% at 50% 50%).
In fact, ellipse(50% 60% at 50% 50%) should serialize as ellipse(50% 60%).

Same for the circle(100px) above.

@emilio
Copy link

emilio commented May 23, 2023

Agreed with @Loirooriol, that test fix is wrong.

@emilio
Copy link

emilio commented May 23, 2023

(in principle, at least).

The spec says:

omitting components when possible without changing the meaning

Which seems true in this case. But then in the non-normative example it says:

Omitting components means that some default values do not show up in the serialization. But since always uses the 2- or 4-value form, a default is not omitted.

I don't quite understand why we shouldn't omit it, but I'm really jet-lagged so maybe I'm not parsing that right.

@BorisChiou
Copy link
Member

BTW, the spec of offset-path says:

If circle() or ellipse() is used, and an explicit center position is not given, they default to using the offset starting position, rather than their standard default.

So now I feel weird if we serialize it as 50% 50% if the user doesn't give the center position. We do not use 50% 50% as default value for offset-path. Serializing it as 50% 50% may mislead people.

@BorisChiou
Copy link
Member

cc @tabatkins

@BorisChiou
Copy link
Member

I filed a spec for this: w3c/fxtf-drafts#514.

@foolip
Copy link
Member

foolip commented May 25, 2023

Does the current spec text require some specific behavior, or is it just unclear about the behavior? I'm wondering if we should wait until w3c/fxtf-drafts#514 is revolved before making any test changes, or if it can be done in two stages.

@BorisChiou
Copy link
Member

Maybe just accept both cases for now. e.g.

// https://github.com/w3c/csswg-drafts/issues/8695
test_valid_value("offset-path", "circle()", ["circle()", "circle(at 50% 50%)"]);

Or something like this.

@BorisChiou
Copy link
Member

OK, the comment in w3c/csswg-drafts#8695 (comment) makes sense to me. We should omit at <position> if it was not given. And preserve the at <position> even if it specifies the default value, for the same reason.

@danielsakhapov
Copy link
Author

@BorisChiou @nt1m updated the test, please, have a look

@BorisChiou
Copy link
Member

Mind updating this as well?

replace
test_valid_value("offset-path", "circle(100px)", "circle(100px at 50% 50%)");
with
test_valid_value("offset-path", "circle(100px)");

For this proposal: "We definitely need to omit the at <position> when it was omitted by the author, for offset-path to work correctly", from w3c/csswg-drafts#8695 (comment)

@danielsakhapov
Copy link
Author

done

@nt1m
Copy link
Member

nt1m commented May 26, 2023

The test looks good to me now.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 26, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) Comes from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
aarongable pushed a commit to chromium/chromium that referenced this issue May 30, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150434}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150434}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2023
1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150434}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 13, 2023
… a=testonly

Automatic update from web-platform-tests
Change WPT test for offset-path parsing

1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150434}

--

wpt-commits: 77e63377e0476fe25da513b11b78f1525c5c91ee
wpt-pr: 40120
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Jun 14, 2023
… a=testonly

Automatic update from web-platform-tests
Change WPT test for offset-path parsing

1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150434}

--

wpt-commits: 77e63377e0476fe25da513b11b78f1525c5c91ee
wpt-pr: 40120
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jun 16, 2023
… a=testonly

Automatic update from web-platform-tests
Change WPT test for offset-path parsing

1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruudchromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1150434}

--

wpt-commits: 77e63377e0476fe25da513b11b78f1525c5c91ee
wpt-pr: 40120

UltraBlame original commit: 6af80ffc63102ca16a772daca958c3ad783d4eab
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jun 16, 2023
… a=testonly

Automatic update from web-platform-tests
Change WPT test for offset-path parsing

1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruudchromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1150434}

--

wpt-commits: 77e63377e0476fe25da513b11b78f1525c5c91ee
wpt-pr: 40120

UltraBlame original commit: 6af80ffc63102ca16a772daca958c3ad783d4eab
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jun 16, 2023
… a=testonly

Automatic update from web-platform-tests
Change WPT test for offset-path parsing

1) As per https://drafts.csswg.org/css-box-4/#typedef-coord-box
<coord-box> can't be defined with margin-box.

2) "at <position" changes come from
w3c/csswg-drafts#8695 (comment)

Resolved here: web-platform-tests/interop#340

Change-Id: I6fe865d5248c7004257cd17669353d810f6e3d09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551246
Reviewed-by: Anders Hartvoll Ruud <andruudchromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1150434}

--

wpt-commits: 77e63377e0476fe25da513b11b78f1525c5c91ee
wpt-pr: 40120

UltraBlame original commit: 6af80ffc63102ca16a772daca958c3ad783d4eab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Motion Path test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

7 participants