Skip to content

Conversation

nikolaszimmermann
Copy link
Contributor

@nikolaszimmermann nikolaszimmermann commented Nov 9, 2022

4b03741

[LBSE] Add transform related tests from LBSE downstream
https://bugs.webkit.org/show_bug.cgi?id=247668

Reviewed by Rob Buis.

Add more tests that were written in the past years and haven't
been upstreamed yet. They mostly cover laying out objects with tiny
dimensions, to assure LBSE renderers these kind of documents properly.
In fact it improves over the legacy engine, which had issues with these
kind of documents (see discussion of test results below).

svg/transforms/transform-origin-and-box-getCTM.html deserves to be a
WPT test, since currently all browser deviate. To my understanding
LBSE handles all combinations correctly, whereas the legacy engine
performed worst before.

Test discussion:

- svg/custom/circle-move-invalidation-small-viewBox.svg
  1) Purpose
  Copy from circle-move-invalidation.svg, but with a user coordinate system
  that is two orders of magnitudes smaller -- verify repainting works fine.

  2) Results
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.
  (Used to have a LBSE-only regression, that's why the test got added)

- svg/transforms/layout-tiny-elements-in-scaled-group.svg
  1) Purpose
  Verifies that using small numbers for e.g. rect dimensions works as expected.
  The test uses four rectangles, with a width/height of 0.0015 - which is an order
  of magnitude smaller than the LayoutUnit epsilon (~1/64 ~ 0.015625), enclosed
  by a group that scales the content by a factor 10000.

  2) Results
    o FF: Layout/rendering is broken - the blue rect covers the whole viewport.
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/layout-tiny-elements.svg, and
  svg/transforms/rotation-tiny-element.svg
  1) Purpose
  Same as layout-tiny-elements-in-scaled-group.svg, however not enclosing the
  rectangles in a scaled group, but transforming the rects themselves.

  2) Results
  Works fine across Chrome/SafariLegacy/SafariLBSE. FF shows the same
  broken behavior as for layout-tiny-elements-in-scaled-group.svg

- svg/transforms/nested-containers.svg, and
  svg/transforms/nested-transforms-rotation-origin-with-viewBox.svg, and
  svg/transforms/nested-transforms-rotation-origin.svg
  1) Purpose
  Basic reftests to check nesting containers with scale/rotation/translation works as intended.

  2) Results
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.

- svg/transforms/rotation-origin-in-small-units.svg
  1) Purpose
  Testing small units as rotation origins in transform attributes.

  2) Results
    o FF: Layout/rendering is broken - tilted, parallel red/yellow stripes + large blue rect appear.
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/rotation-tiny-element-in-group.svg,
  svg/transforms/translation-tiny-element.svg

  1) Purpose
  Similar to rotation-tiny-element.svg (however the rotation is applied on an enclosing container).

  2) Results
    o FF: Layout/rendering is broken - red rectangle covers the whole viewport (however at right origin, and rotated as expected for the rotation-* test).
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/transform-origin-and-box-getCTM.html, and
  svg/transforms/transform-origin-and-box.svg
  1) Purpose
  transform-origin-and-box.svg checks various transform-box / transform-origin permutations, combined with transform
  defined as SVG transform attribute. The *getCTM.html then queries the SVG JS exposed geometry (getCTM/getBBox/etc.).

  2) Results
  transform-origin-and-box.svg works fine.
  transform-origin-and-box-getCTM.html highlights a number of cross-browser differences:

    o FF: Fails getCTM() / getScreenCTM() (+ getAbsoluteBBoxById()) tests for rect7 / rect8 / rect9.
    o Chrome: Works as expected.
    o SafariLegacy: Fails getCTM() / getScreenCTM() (+ getAbsoluteBBoxById()) for rect3 / rect4 / rect5 / rect6 / rect9.
    o SafariLBSE: Fails all tests -- getCTM is currently not implemented in a LBSE-aware way (patch is ready to fix that -- then we'll pass all tests!)

- svg/transforms/transformed-child-in-container.svg, and
  svg/transforms/transformed-child-in-container-small-units.svg, and
  svg/transforms/transformed-container.svg, and
  svg/transforms/transformed-container-small-units.svg
  1) Purpose
  Basic reftests to check nesting transformed and untransformed containers works fine.
  Used for original LBSE bootstrapping process.

  2) Results:
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.

No change in functionality, only adding new tests.

* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-ventura-wk2-lbse-text/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/svg/custom/circle-move-invalidation-small-viewBox-expected.svg: Added.
* LayoutTests/svg/custom/circle-move-invalidation-small-viewBox.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-expected.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group-expected.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements.svg: Added.
* LayoutTests/svg/transforms/nested-containers-expected.svg: Added.
* LayoutTests/svg/transforms/nested-containers.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-expected.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-with-viewBox-expected.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-with-viewBox.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin.svg: Added.
* LayoutTests/svg/transforms/rotation-origin-in-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-origin-in-small-units.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-in-group-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-in-group.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element.svg: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-expected.svg: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-getCTM-expected.txt: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-getCTM.html: Added.
* LayoutTests/svg/transforms/transform-origin-and-box.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-small-units.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container.svg: Added.
* LayoutTests/svg/transforms/transformed-container-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-container-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-container-small-units.svg: Added.
* LayoutTests/svg/transforms/transformed-container.svg: Added.
* LayoutTests/svg/transforms/translation-tiny-element-expected.svg: Added.
* LayoutTests/svg/transforms/translation-tiny-element.svg: Added.

Canonical link: https://commits.webkit.org/256948@main

8b15230

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 🧪 merge ✅ 🛠 watch 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@nikolaszimmermann nikolaszimmermann self-assigned this Nov 9, 2022
@nikolaszimmermann nikolaszimmermann added SVG For bugs in the SVG implementation. WebKit Local Build labels Nov 9, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 9, 2022
@nikolaszimmermann nikolaszimmermann removed the merging-blocked Applied to prevent a change from being merged label Nov 10, 2022
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from b687575 to c6b4041 Compare November 10, 2022 23:29
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for c6b4041. Live statuses available at the PR page, #6292

@nikolaszimmermann
Copy link
Contributor Author

Rebased after the macOS Ventura pixel test baseline landed + fix EWS problems (forgot to skip new tests that only pass with LBSE activated on ios-wk2 / glib / ...)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 11, 2022
@nikolaszimmermann
Copy link
Contributor Author

Rebasing to retry the wincairo EWS....

@nikolaszimmermann nikolaszimmermann removed the merging-blocked Applied to prevent a change from being merged label Nov 11, 2022
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from c6b4041 to 3925c39 Compare November 11, 2022 13:08
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 12, 2022
@nikolaszimmermann nikolaszimmermann removed the merging-blocked Applied to prevent a change from being merged label Nov 17, 2022
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from 3925c39 to 4041b09 Compare November 17, 2022 11:38
@nikolaszimmermann
Copy link
Contributor Author

Please check again @rwlbuis.

@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from 4041b09 to a5000bf Compare November 17, 2022 21:13
@rwlbuis
Copy link
Contributor

rwlbuis commented Nov 18, 2022

Should (some of these) be WPT tests?

@nikolaszimmermann
Copy link
Contributor Author

Should (some of these) be WPT tests?

Possible, yes. However they need to be reworked for that and I have no capacity left for that at present.... I would still like to see them included in LBSE testing - we could create a tracking bug for tests worth upstreaming. WDYT?

@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from a5000bf to c26e42e Compare November 18, 2022 21:41
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 19, 2022
@nikolaszimmermann nikolaszimmermann removed the merging-blocked Applied to prevent a change from being merged label Nov 19, 2022
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from c26e42e to 34cc292 Compare November 19, 2022 23:27
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from 34cc292 to 8b15230 Compare November 22, 2022 15:05
Copy link
Contributor

@rwlbuis rwlbuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@rwlbuis
Copy link
Contributor

rwlbuis commented Nov 22, 2022

Should (some of these) be WPT tests?

Possible, yes. However they need to be reworked for that and I have no capacity left for that at present.... I would still like to see them included in LBSE testing - we could create a tracking bug for tests worth upstreaming. WDYT?

Yes, I guess a tracking bug for that would be good.

@nikolaszimmermann nikolaszimmermann added the merge-queue Applied to send a pull request to merge-queue label Nov 22, 2022
https://bugs.webkit.org/show_bug.cgi?id=247668

Reviewed by Rob Buis.

Add more tests that were written in the past years and haven't
been upstreamed yet. They mostly cover laying out objects with tiny
dimensions, to assure LBSE renderers these kind of documents properly.
In fact it improves over the legacy engine, which had issues with these
kind of documents (see discussion of test results below).

svg/transforms/transform-origin-and-box-getCTM.html deserves to be a
WPT test, since currently all browser deviate. To my understanding
LBSE handles all combinations correctly, whereas the legacy engine
performed worst before.

Test discussion:

- svg/custom/circle-move-invalidation-small-viewBox.svg
  1) Purpose
  Copy from circle-move-invalidation.svg, but with a user coordinate system
  that is two orders of magnitudes smaller -- verify repainting works fine.

  2) Results
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.
  (Used to have a LBSE-only regression, that's why the test got added)

- svg/transforms/layout-tiny-elements-in-scaled-group.svg
  1) Purpose
  Verifies that using small numbers for e.g. rect dimensions works as expected.
  The test uses four rectangles, with a width/height of 0.0015 - which is an order
  of magnitude smaller than the LayoutUnit epsilon (~1/64 ~ 0.015625), enclosed
  by a group that scales the content by a factor 10000.

  2) Results
    o FF: Layout/rendering is broken - the blue rect covers the whole viewport.
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/layout-tiny-elements.svg, and
  svg/transforms/rotation-tiny-element.svg
  1) Purpose
  Same as layout-tiny-elements-in-scaled-group.svg, however not enclosing the
  rectangles in a scaled group, but transforming the rects themselves.

  2) Results
  Works fine across Chrome/SafariLegacy/SafariLBSE. FF shows the same
  broken behavior as for layout-tiny-elements-in-scaled-group.svg

- svg/transforms/nested-containers.svg, and
  svg/transforms/nested-transforms-rotation-origin-with-viewBox.svg, and
  svg/transforms/nested-transforms-rotation-origin.svg
  1) Purpose
  Basic reftests to check nesting containers with scale/rotation/translation works as intended.

  2) Results
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.

- svg/transforms/rotation-origin-in-small-units.svg
  1) Purpose
  Testing small units as rotation origins in transform attributes.

  2) Results
    o FF: Layout/rendering is broken - tilted, parallel red/yellow stripes + large blue rect appear.
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/rotation-tiny-element-in-group.svg,
  svg/transforms/translation-tiny-element.svg

  1) Purpose
  Similar to rotation-tiny-element.svg (however the rotation is applied on an enclosing container).

  2) Results
    o FF: Layout/rendering is broken - red rectangle covers the whole viewport (however at right origin, and rotated as expected for the rotation-* test).
    o Chrome: Works as expected.
    o SafariLegacy: Empty document, nothing rendered.
    o SafariLBSE: Works as expected.

- svg/transforms/transform-origin-and-box-getCTM.html, and
  svg/transforms/transform-origin-and-box.svg
  1) Purpose
  transform-origin-and-box.svg checks various transform-box / transform-origin permutations, combined with transform
  defined as SVG transform attribute. The *getCTM.html then queries the SVG JS exposed geometry (getCTM/getBBox/etc.).

  2) Results
  transform-origin-and-box.svg works fine.
  transform-origin-and-box-getCTM.html highlights a number of cross-browser differences:

    o FF: Fails getCTM() / getScreenCTM() (+ getAbsoluteBBoxById()) tests for rect7 / rect8 / rect9.
    o Chrome: Works as expected.
    o SafariLegacy: Fails getCTM() / getScreenCTM() (+ getAbsoluteBBoxById()) for rect3 / rect4 / rect5 / rect6 / rect9.
    o SafariLBSE: Fails all tests -- getCTM is currently not implemented in a LBSE-aware way (patch is ready to fix that -- then we'll pass all tests!)

- svg/transforms/transformed-child-in-container.svg, and
  svg/transforms/transformed-child-in-container-small-units.svg, and
  svg/transforms/transformed-container.svg, and
  svg/transforms/transformed-container-small-units.svg
  1) Purpose
  Basic reftests to check nesting transformed and untransformed containers works fine.
  Used for original LBSE bootstrapping process.

  2) Results:
  Works fine across FF/Chrome/SafariLegacy/SafariLBSE.

No change in functionality, only adding new tests.

* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/mac-ventura-wk2-lbse-text/TestExpectations:
* LayoutTests/platform/mac-wk2/TestExpectations:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/svg/custom/circle-move-invalidation-small-viewBox-expected.svg: Added.
* LayoutTests/svg/custom/circle-move-invalidation-small-viewBox.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-expected.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group-expected.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements-in-scaled-group.svg: Added.
* LayoutTests/svg/transforms/layout-tiny-elements.svg: Added.
* LayoutTests/svg/transforms/nested-containers-expected.svg: Added.
* LayoutTests/svg/transforms/nested-containers.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-expected.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-with-viewBox-expected.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin-with-viewBox.svg: Added.
* LayoutTests/svg/transforms/nested-transforms-rotation-origin.svg: Added.
* LayoutTests/svg/transforms/rotation-origin-in-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-origin-in-small-units.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-in-group-expected.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element-in-group.svg: Added.
* LayoutTests/svg/transforms/rotation-tiny-element.svg: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-expected.svg: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-getCTM-expected.txt: Added.
* LayoutTests/svg/transforms/transform-origin-and-box-getCTM.html: Added.
* LayoutTests/svg/transforms/transform-origin-and-box.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container-small-units.svg: Added.
* LayoutTests/svg/transforms/transformed-child-in-container.svg: Added.
* LayoutTests/svg/transforms/transformed-container-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-container-small-units-expected.svg: Added.
* LayoutTests/svg/transforms/transformed-container-small-units.svg: Added.
* LayoutTests/svg/transforms/transformed-container.svg: Added.
* LayoutTests/svg/transforms/translation-tiny-element-expected.svg: Added.
* LayoutTests/svg/transforms/translation-tiny-element.svg: Added.

Canonical link: https://commits.webkit.org/256948@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch from 8b15230 to 4b03741 Compare November 22, 2022 21:10
@webkit-commit-queue
Copy link
Collaborator

Committed 256948@main (4b03741): https://commits.webkit.org/256948@main

Reviewed commits have been landed. Closing PR #6292 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 4b03741 into WebKit:main Nov 22, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 22, 2022
@nikolaszimmermann
Copy link
Contributor Author

Yes, I guess a tracking bug for that would be good.
Filed https://bugs.webkit.org/show_bug.cgi?id=248244.

@nikolaszimmermann nikolaszimmermann deleted the eng/LBSE-Add-transform-related-tests-from-LBSE-downstream branch November 22, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SVG For bugs in the SVG implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants