RESOLVED FIXED198746
[cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with transform
https://bugs.webkit.org/show_bug.cgi?id=198746
Summary [cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with t...
Fujii Hironori
Reported 2019-06-10 23:11:39 PDT
Created attachment 371826 [details] test case [cairo][SVG] If clipPath has multiple elements, clip-path doesn't work with opacity
Attachments
test case (846 bytes, text/html)
2019-06-10 23:11 PDT, Fujii Hironori
no flags
WIP (2.98 KB, patch)
2019-06-12 02:25 PDT, Carlos Garcia Campos
no flags
WIP (3.21 KB, patch)
2019-06-12 05:11 PDT, Carlos Garcia Campos
no flags
WIP (3.18 KB, patch)
2019-06-12 05:15 PDT, Carlos Garcia Campos
no flags
Patch (8.91 KB, patch)
2019-06-12 06:04 PDT, Carlos Garcia Campos
no flags
Patch (21.25 KB, patch)
2019-06-12 19:05 PDT, Fujii Hironori
fujii.hironori: commit-queue-
Patch (9.29 KB, patch)
2019-06-12 19:08 PDT, Fujii Hironori
no flags
Carlos Garcia Campos
Comment 1 2019-06-11 07:35:31 PDT
The problem here is the translation (transform="translate(200,0)") in the second case. I've managed to fix this by using a cairo_pattern_t created for the given cairo_image_t and setting a transformation matrix. But that broke other test cases attached to bug #198701, so it needs some more work. It seems the matrix I'm using only works when current transformation matrix doesn't have a translation already. I'll continue investigating tomorrow.
Fujii Hironori
Comment 2 2019-06-11 23:09:50 PDT
My patch in Bug 198779 comment 2 solves this issue only in non high DPI display, but not fixed in high DPI display. Funny. It seems there is one more coordinate system bug around us.
Carlos Garcia Campos
Comment 3 2019-06-12 02:25:29 PDT
Created attachment 371938 [details] WIP This fixes this test case without breaking the ones attached to bug #198701. It makes the code a lot simpler too. HiDPI is a different issue, we need to apply somehow the device scale factor to the mask surface. This patch only needs to include a test. Fujii why are you using html tests instead of svg?
Fujii Hironori
Comment 4 2019-06-12 03:28:04 PDT
Comment on attachment 371938 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=371938&action=review LGTM > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:86 > + cairo_matrix_invert(&matrix); You can remove this cairo_matrix_invert by negating. cairo_matrix_init_translate(&matrix, -rect.x(), -rect.y());
Carlos Garcia Campos
Comment 5 2019-06-12 04:45:40 PDT
(In reply to Fujii Hironori from comment #4) > Comment on attachment 371938 [details] > WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371938&action=review > > LGTM It breaks other tests, though. I think I have a new patch that would also fix bug #198779, running the tests now. > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:86 > > + cairo_matrix_invert(&matrix); > > You can remove this cairo_matrix_invert by negating. > cairo_matrix_init_translate(&matrix, -rect.x(), -rect.y()); Ok, even simpler indeed.
Carlos Garcia Campos
Comment 6 2019-06-12 05:11:24 PDT
Created attachment 371942 [details] WIP This patch fixes this test case and also the HiDPI one with no regressions in layout tests. It just needs a couple of new tests.
Carlos Garcia Campos
Comment 7 2019-06-12 05:15:03 PDT
Created attachment 371944 [details] WIP I forgot to remove the matrix_invert call
Carlos Garcia Campos
Comment 8 2019-06-12 06:04:50 PDT
Carlos Garcia Campos
Comment 9 2019-06-12 06:41:02 PDT
*** Bug 198779 has been marked as a duplicate of this bug. ***
Carlos Garcia Campos
Comment 10 2019-06-12 06:43:43 PDT
Radar WebKit Bug Importer
Comment 11 2019-06-12 06:44:16 PDT
Truitt Savell
Comment 12 2019-06-12 09:06:45 PDT
The new test svg/clip-path/clip-hidpi.svg Added in https://trac.webkit.org/changeset/246350/webkit is timing out. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fclip-path%2Fclip-hidpi.svg This test is effecting Mac wk1. It looks like EWS caught this timeout on Mac wk1 and Mac debug but was not allowed to finish running. It looks like this test should be corrected. Our policies state we should roll this out until a functioning test can be landed with the patch.
Truitt Savell
Comment 13 2019-06-12 09:18:23 PDT
Truitt Savell
Comment 14 2019-06-12 09:22:02 PDT
Reverted r246350 for reason: r246350 Introduced a failing and timing out test svg/clip-path/clip-hidpi.svg Committed r246354: <https://trac.webkit.org/changeset/246354>
Ryan Haddad
Comment 15 2019-06-12 09:40:14 PDT
(In reply to Truitt Savell from comment #12) > The new test svg/clip-path/clip-hidpi.svg > > Added in https://trac.webkit.org/changeset/246350/webkit > > is timing out. History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=svg%2Fclip-path%2Fclip-hidpi.svg > > This test is effecting Mac wk1. It looks like EWS caught this timeout on Mac > wk1 and Mac debug but was not allowed to finish running. It looks like this > test should be corrected. Our policies state we should roll this out until a > functioning test can be landed with the patch. This timeout was also seen on Win10: https://build.webkit.org/results/Apple%20Win%2010%20Release%20(Tests)/r246350%20(2551)/results.html
Michael Catanzaro
Comment 16 2019-06-12 10:17:08 PDT
Reopening due to the rollout.
Fujii Hironori
Comment 17 2019-06-12 19:05:39 PDT
Fujii Hironori
Comment 18 2019-06-12 19:08:01 PDT
Created attachment 372015 [details] Patch * Fixed timing out issue by using ->. * Fixed the bug id in ChangeLog
WebKit Commit Bot
Comment 19 2019-06-12 21:10:39 PDT
Comment on attachment 372015 [details] Patch Clearing flags on attachment: 372015 Committed r246391: <https://trac.webkit.org/changeset/246391>
WebKit Commit Bot
Comment 20 2019-06-12 21:10:41 PDT
All reviewed patches have been landed. Closing bug.
Fujii Hironori
Comment 21 2019-06-12 21:18:04 PDT
Great work! Good job, KaL. I'm still confused by transform matrix hell, but your solution looks consistent.
Fujii Hironori
Comment 22 2019-06-28 00:27:49 PDT
(In reply to Fujii Hironori from comment #18) > * Fixed timing out issue by using ->. Oops, my bad. This is a just syntax error. Filed in Bug 199313.
Note You need to log in before you can comment on or make changes to this bug.