Closed Bug 1385239 Opened 7 years ago Closed 7 years ago

incorrect SVG mask with transformed/filtered graphic object

Categories

(Core :: SVG, defect, P3)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 - wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: kpreisert, Assigned: u459114)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

Attached image test.svg —
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open the attached SVG file in Firefox and compare it to other browsers.


Actual results:

It shows a red background and the text, and a little gray box in the top left corner.


Expected results:

The gray box should be positioned such that the text is directly on top of it
[Tracking Requested - why for this release]: SVG rendering regression


1st regression window(no gray box is drawn):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7d9fb1f4c14ec4e5ed9c8b575e70c2f39a57c983&tochange=655021ab4e0729879f20ad05979e9dad7fb0b24e

Regressed by: Bug 1224207

2nd regression window(a little gray box is drawn on the top left corner):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cc308a73ad05ab7b11872d8640f9711a99775094&tochange=5f8ed78861489be8477b1689afb361c3750ff263

Regressed by: Bug 1348430


:@cjku,
your patch seems to cause this problem.
Blocks: 1348430, 1224207
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(cku)
Look like we do not pass this translation into filter after my change in those two bugs.
<g transform="translate(10, 50)">

I will look into it. Thank you for reporting this issue.
Assignee: nobody → cku
Flags: needinfo?(cku)
Priority: -- → P3
Attached image test-2.svg (obsolete) —
Attached image test-2.svg —
Expected behavior: lime background color.
Actual behavior: red background color
Attachment #8899755 - Attachment is obsolete: true
Summary: incorrect SVG mask with transformed text → incorrect SVG mask with transformed/filtered graphic object
Attachment #8900108 - Flags: review?(mstange)
Attachment #8900588 - Flags: review?(mstange)
Taking just the translation from mPaintTransform seems like it's not the right thing to do but happens to work in this case. Please describe the various transforms that are involved here in more detail and try to find a way to use mPaintTransform that uses the whole mPaintTransform instead of just its translation component; or alternatively, find a good justification for why just taking the translation is the right thing to do.
Attachment #8900108 - Attachment is obsolete: true
Attachment #8900108 - Flags: review?(mstange)
Attachment #8901178 - Flags: review?(mstange)
Attachment #8901179 - Flags: review?(mstange)
Comment on attachment 8901178 [details]
Bug 1385239 - Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame.

https://reviewboard.mozilla.org/r/172652/#review178196
Attachment #8901178 - Flags: review?(mstange) → review+
Thanks! I'll probably review this patch next week.
Comment on attachment 8901179 [details]
Bug 1385239 - Part 2. Remove aTransform parameter from PaintFilteredFrame.

https://reviewboard.mozilla.org/r/172654/#review182758

I prefer this patch very much to the previous one. Thanks! And thanks again for the detailed explanation.

::: layout/svg/nsSVGUtils.cpp:858
(Diff revision 3)
> +    // Take out css-to-dev-px scaling from the current transform of 'target'
> +    // since nsFilterInstance::PaintFilteredFrame does not expect it.

Could you rephrase the comment like this? (Is what I'm saying there correct?)

'target' is currently scaled such that its user space units are CSS pixels (SVG user space units). But PaintFilteredFrame expects it to be scaled in such a way that its user space units are device pixels. So we have to adjust the scale.

::: layout/svg/nsSVGUtils.cpp:862
(Diff revision 3)
> +
> +    // Take out css-to-dev-px scaling from the current transform of 'target'
> +    // since nsFilterInstance::PaintFilteredFrame does not expect it.
> +    gfxMatrix reverseScaleMatrix = nsSVGUtils::GetCSSPxToDevPxMatrix(aFrame);
> +    DebugOnly<bool> invertible = reverseScaleMatrix.Invert();
> +    target->SetMatrix(reverseScaleMatrix *aTransform *

missing space between * and aTransform
Attachment #8901179 - Flags: review?(mstange) → review+
Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

https://reviewboard.mozilla.org/r/171988/#review182760
Attachment #8900588 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45278d597320
Part 1. Pass gfxContext, instead of DrawTarget, into nsFilterInstance::PaintFilteredFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/9a6413d1fd0a
Part 2. Remove aTransform parameter from PaintFilteredFrame. r=mstange
https://hg.mozilla.org/integration/autoland/rev/114d87a3943b
Part 3. A test case of putting a translated filtered element inside an SVG mask. r=mstange
Can this ride the 57 train or should we consider it for Beta backport? Note that we're spinning the RC build for 56 next week.
Flags: needinfo?(cku)
Flags: in-testsuite+
Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1348430
[User impact if declined]: a transformed object inside an svg mask will be positioned at wrong place.
[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]: Part 1 ~ Part 3.
[Is the change risky?]: no
[Why is the change risky/not risky?]: Small change covered by a reftest.
[String changes made/needed]:NA
Flags: needinfo?(cku)
Attachment #8900588 - Flags: approval-mozilla-beta?
Comment on attachment 8900588 [details]
Bug 1385239 - Part 3. A test case of putting a translated filtered element inside an SVG mask.

Fix for SFG regression from 54, has new test coverage, let's uplift it for beta 12.
Attachment #8900588 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8901178 - Flags: approval-mozilla-beta+
Attachment #8901179 - Flags: approval-mozilla-beta+
See Also: → 1403057
You need to log in before you can comment on or make changes to this bug.