Closed Bug 1343664 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: mpk, Assigned: u459114)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files)

simple testcase
321 bytes, image/svg+xml
Details
Bug 1343664 - Part 1. Correct transform matrix.
59 bytes, text/x-review-board-request
mstange
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1343664 - Part 2. Reftest.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Part 2 on Aurora
2.22 KB, patch
Details | Diff | Splinter Review
Attached image 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").
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: ? → +
I will try to fix it in this week
Assignee: nobody → cku
Flags: needinfo?(cku)
Attachment #8843872 - Flags: review?(mstange)
Attachment #8843873 - Flags: review?(mstange)
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 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 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 on attachment 8843873 [details]
Bug 1343664 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/117498/#review123114
Attachment #8843873 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/fe02ae88f611
https://hg.mozilla.org/mozilla-central/rev/44e9eace508a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)
Attached patch Part 2 on Aurora — — Splinter Review 
Flags: needinfo?(cku)

    
    

    
  
Hi Tomcat,
Please use attachment 8849488 [details] [diff] [review] instead. Sorry for inconvenience.

    
    

    
  
ni in case.
Flags: needinfo?(cbook)

          You need to log in
          before you can comment on or make changes to this bug.