SVG filters seem to mess with the transformation matrix origin of their target element

RESOLVED FIXED in Firefox 54

Status

()

Core
SVG
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: Marco Perez, Assigned: cjku)

Tracking

({regression, testcase})

Trunk
mozilla55
regression, testcase
Points:
---

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54+ fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

9 months ago
Created attachment 8842598 [details]
simple testcase

Applying a filter to an SVG element seems to alter its transformation origin (center coordinates of the transformation). This is a regression from between 2017-01-05 and 2017-02-25. I'll try to narrow down the regression window as soon as I find some time. But if someone beats me to it, be my guest. :-)

The two circles in the testcase should both be drawn in the center of the image. The green one on top of the red one, so red should actually be hidden (except for its "corona").

Comment 1

9 months ago
regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7d9fb1f4c14ec4e5ed9c8b575e70c2f39a57c983&tochange=655021ab4e07
Blocks: 1224207
tracking-firefox54: --- → ?
Flags: needinfo?(cku)
Removing the "regressionwindow-wanted" keyword since the regression window was provided in comment 1.
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
Keywords: regressionwindow-wanted
Tracking 54+ for this SVG regression.
tracking-firefox54: ? → +
(Assignee)

Comment 4

9 months ago
I will try to fix it in this week
Assignee: nobody → cku
Flags: needinfo?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1341672
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8843872 - Flags: review?(mstange)
Attachment #8843873 - Flags: review?(mstange)

Comment 9

9 months ago
mozreview-review
Comment on attachment 8843873 [details]
Bug 1343664 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/117498/#review119486

::: layout/reftests/svg/filters/filter-transform-01.svg:15
(Diff revision 2)
> + <circle r="15" fill="red" filter="url(#blur)" transform="skewX(0) skewY(0)"/>
> + <circle cx="-10" cy="-10" r="15" fill="red" filter="url(#blur)" transform="translate(10, 10)"/>
> + <circle cx="-10000" cy="-10000" r="15" fill="red" filter="url(#blur)" transform="translate(10000, 10000)"/>
> + <circle r="10" fill="red" filter="url(#blur)" transform="scale(1.5, 1.5)"/>
> +
> + <!-- Cicles above should be completely covered by the next one. -->

You want is "The circles" rather than "Cicles"
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

9 months ago
mozreview-review-reply
Comment on attachment 8843873 [details]
Bug 1343664 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/117498/#review119486

> You want is "The circles" rather than "Cicles"

Fixed. Thanks.

Comment 13

8 months ago
mozreview-review
Comment on attachment 8843872 [details]
Bug 1343664 - Part 1. Correct transform matrix.

https://reviewboard.mozilla.org/r/117496/#review123100

Sorry, it took me quite a while to explain to myself that this order of matrix operation was correct. I am convinced now.

Here's how I explained it to myself:
If you have a vector in filter space, and want to transform it into DrawTarget device space, then the calculation that is going to be performed is:
vectorDev = vectorFilter * translationInFilterSpace * scaleFromFilterSpaceToUseSpace * userSpaceToDeviceSpaceDTTransform,
so our adjustments need to be multiplied from the left to the DT's old matrix.
Attachment #8843872 - Flags: review?(mstange) → review+

Comment 14

8 months ago
mozreview-review
Comment on attachment 8843873 [details]
Bug 1343664 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/117498/#review123114
Attachment #8843873 - Flags: review?(mstange) → review+
status-firefox55: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

8 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe02ae88f611
Part 1. Correct transform matrix. r=mstange
https://hg.mozilla.org/integration/autoland/rev/44e9eace508a
Part 2. Reftest. r=mstange

Comment 18

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe02ae88f611
https://hg.mozilla.org/mozilla-central/rev/44e9eace508a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 19

8 months ago
Comment on attachment 8843872 [details]
Bug 1343664 - Part 1. Correct transform matrix.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1224207
[User impact if declined]: A graphic object with filter applied may be painted on the wrong place if that object also has transform attribute.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no.
[Why is the change risky/not risky?]: change is simple, all reftest are pass.
[String changes made/needed]: N/A

Please considerate uplift both Part 1 and Part 2 patch.
Attachment #8843872 - Flags: approval-mozilla-aurora?
Comment on attachment 8843872 [details]
Bug 1343664 - Part 1. Correct transform matrix.

Fix an SVG regression. Aurora54+.
Attachment #8843872 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
cjku: can you take a look at the reftests like Tomcats-MacBook-Pro-820:mozilla-central Tomcat$ vim layout/reftests/svg/filters/reftest.list
Tomcats-MacBook-Pro-820:mozilla-central Tomcat$ hg revert -a
forgetting layout/reftests/svg/filters/filter-transform-01.svg
reverting layout/reftests/svg/filters/reftest.list
Tomcats-MacBook-Pro-820:mozilla-central Tomcat$ hg resolve --mark
(no more unresolved files)
continue: hg graft --continue
Flags: needinfo?(cku)
(Assignee)

Comment 22

8 months ago
Created attachment 8849488 [details] [diff] [review]
Part 2 on Aurora
Flags: needinfo?(cku)
(Assignee)

Comment 23

8 months ago
Hi Tomcat,
Please use attachment 8849488 [details] [diff] [review] instead. Sorry for inconvenience.
(Assignee)

Comment 24

8 months ago
ni in case.
Flags: needinfo?(cbook)
 https://hg.mozilla.org/releases/mozilla-aurora/rev/8d9943dc44631dd0f8766b54d485b32e40f595c0 landed the test part thanks cjku!
Flags: needinfo?(cbook)

Comment 26

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1a53aec4fb0d
status-firefox54: affected → fixed
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.