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] Add new tests for <basic-shape> #327

Closed
danielsakhapov opened this issue May 4, 2023 · 14 comments
Closed

[motion-1] Add new tests for <basic-shape> #327

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

Comments

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

@BorisChiou @nmoucht FYI

@BorisChiou
Copy link
Member

Gecko doesn't support basic shape for motion, so I'm OK with landing these tests (and may verify again when I work on the gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1598156)

@gsnedders
Copy link
Member

In aggregate: this wpt.fyi query.

@foolip
Copy link
Member

foolip commented May 9, 2023

@gsnedders @nt1m can either of you get a position for WebKit on this?

@BorisChiou
Copy link
Member

I just noticed these two tests:

  1. offset-path-shape-circle-002.html
  2. offset-path-shape-ellipse-002.html

Their expectations (i.e. xxx-ref.html) seem incorrect to me.

Per css-shape spec (https://drafts.csswg.org/css-shapes-1/#typedef-shape-radius), for <shape-radius>:

Defines a radius for a circle or ellipse. If omitted it defaults to closest-side.

So they are equal to circle(closest-side) and ellipse(closest-side). The closest-side in the test for circle() is 200px, so the translate() in its ref file is something like translate(450px, 150px), right?

Also, I noticed there are some floating-point numbers used in the xxx-ref files, e.g. offset-path-shape-ellipse-003-ref.html. I may update thest tests to avoid using so many floating-point numbers in the xxx-ref files. Also, I may add more fuzzy expectations in these tests (for circle() and ellipse()) for Gecko.

(Note: I expect the fuzzy happens because Gecko uses a different way to handle sub-pixel anti-alias, and the arc calculation makes the angles of the tangent vectors have some minor differences. However, the visual results are almost identical.)

@danielsakhapov
Copy link
Author

@BorisChiou

Hi! Thanks for your comment!

  1. The nuance with this test is here:

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.

That's why the box ends up at the containing block's top left corner with zero radius, only moved by -50px to match it's center.

  1. If you could update tests to have integer numbers, it would be great! I just couldn't find anything integer to test rotation on the circle and ellipse.

  2. I'm absolutely fine with fuzziness in the motion path case!

@BorisChiou
Copy link
Member

BorisChiou commented May 19, 2023

I deleted my previous comment because now I understand the reason. For circle(), the default offset-position makes the center of the circle be top left, so the radius, closest-side, is 0px. That's why the final translate is translate(-50px, -50px).

@tabatkins
Copy link

No, not quite. It makes it be the top left of the transformed element (its pre-transform position). Now, in this case, that happens to also be the top left of the containing block, because that's just how the element is positioned. Then we resolve closest-side against the containing block and get a 0 radius.

If the square had a margin on it, or the container had padding, or anything to not make those two corners coincident, then we'd get a non-zero radius out of it.

(And btw, I had some pending edits from w3c/fxtf-drafts#504 that I've just applied, so it would be good to get some tests for that as well - offset-position can now be normal, which means you use the normal defaulting rules for an omitted position as specified by the Shapes spec.)

@BorisChiou
Copy link
Member

Thanks for the "NOTE" to explain the definition of the box for offset-position:auto in the spec. :)

@tabatkins
Copy link

If it makes you feel better, I actually wrote that text before seeing your comment; it just felt like something that should be made more explicit. ^_^

@nmoucht
Copy link

nmoucht commented Jun 7, 2023

@gsnedders @nt1m can either of you get a position for WebKit on this?

I don't have any objections to adding these tests.

@danielsakhapov
Copy link
Author

thanks all, closing the issue

@foolip
Copy link
Member

foolip commented Jun 8, 2023

I have labeled the 17 issues in web-platform-tests/wpt-metadata#4313. Note that offset-path-shape-circle-005.html and offset-path-shape-ellipse-005.html were not labeled, because they weren't discussed above.

aarongable pushed a commit to chromium/chromium that referenced this issue Jun 8, 2023
Resolved by: web-platform-tests/interop#327

Change-Id: Ic9ea1cccc454feb1c99f8dbe87357ef0a4b56ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601330
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155131}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2023
Resolved by: web-platform-tests/interop#327

Change-Id: Ic9ea1cccc454feb1c99f8dbe87357ef0a4b56ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601330
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155131}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2023
Resolved by: web-platform-tests/interop#327

Change-Id: Ic9ea1cccc454feb1c99f8dbe87357ef0a4b56ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601330
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155131}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 22, 2023
…rect, a=testonly

Automatic update from web-platform-tests
Remove wpt test for offset-path as incorrect

Resolved by: web-platform-tests/interop#327

Change-Id: Ic9ea1cccc454feb1c99f8dbe87357ef0a4b56ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601330
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155131}

--

wpt-commits: 12c162cd662feae268a9f69cc1f8ddde44cf86d8
wpt-pr: 40441
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Jun 22, 2023
…rect, a=testonly

Automatic update from web-platform-tests
Remove wpt test for offset-path as incorrect

Resolved by: web-platform-tests/interop#327

Change-Id: Ic9ea1cccc454feb1c99f8dbe87357ef0a4b56ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601330
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1155131}

--

wpt-commits: 12c162cd662feae268a9f69cc1f8ddde44cf86d8
wpt-pr: 40441
@BorisChiou
Copy link
Member

BTW, I just noticed offset-path-shape-xywh-002.html may be incorrect. It seems Blink uses the height of its reference box to resolve the percentage value of the width of the transformed box:
https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:third_party/blink/renderer/core/style/basic_shapes.cc;l=253;drc=80833aef0b4fff4978b49865bb95a7497f200c71.

Therefore, I'm updating this test file in Gecko's bug right now (https://bugzilla.mozilla.org/show_bug.cgi?id=1786160).

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