Translated elements with filters don't work any more

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: longsonr, Assigned: roc)

Tracking

({regression, testcase})

Trunk
x86
Windows Vista
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

10 years ago
Summary: some morphology filters don't work any more → some filters don't work any more
(Reporter)

Comment 2

10 years ago
I see missing filters in the examples. One the top left filter is displayed. In the example-01-b I only see a grey box.

A regression range might help.
Keywords: qawanted
(Reporter)

Comment 4

10 years ago
Something goes wrong when the filter is transformed. I.e. having the use within a <g transform="...".
Flags: wanted1.9.1?
Flags: blocking1.9.1?
(Reporter)

Updated

10 years ago
Duplicate of this bug: 459675
(Reporter)

Updated

10 years ago
Summary: some filters don't work any more → Translated elements with filters don't work any more
(Reporter)

Comment 6

10 years ago
Any ideas what's happening here roc?
Nothing springs to mind. Got a reduced testcase?
(Reporter)

Comment 8

10 years ago
Is the one in bug 459675 any good?
That one passes for me on Mac. The w3.org tests fail though.
(Reporter)

Comment 10

10 years ago
Created attachment 342970 [details]
reduced testcase

If I take the path that is within the morphologySource <g>, replace the use with it and stick the filter directly on the path I do see something so this is about as small as it gets I think.
(Reporter)

Comment 11

10 years ago
Created attachment 343298 [details] [diff] [review]
WIP


This makes filters display and returns us to the way we processed filters before bug 450340 i.e. passing null as the dirtyRect but it's wallpaper.

clearly there is a problem in nsSVGFilterInstance::BuildSourceImages in the calculation of the dirty rect. Some translation is missing. Can you see what it is roc?
Attachment #343298 - Flags: review?(roc)
(Reporter)

Comment 12

10 years ago
Not so much a review request as a cry for help ;-)
(Reporter)

Updated

10 years ago
Blocks: 450340
Keywords: qawanted
Created attachment 343308 [details]
Slightly simpler testcase
In nsSVGFilterInstance::BuildSourceImages, we render the source <g>, but that fails to paint the <rect> child because in nsSVGUtils::PaintChildWithEffects the <rect>'s frame's mRect is tested against the dirty rect and they don't intersect. The problem is, mRect is relative to the root <svg>, but aDirty is in "user space" for the frame.
Created attachment 343349 [details] [diff] [review]
fix

-- Transform the dirty rect from user space CSS pixels to outer-SVG device pixels
-- Instead of using SetOverrideCTM, just set the transform the gfxContext. By doing this in the nsSVGFilterInstance, we can remove the transform parameter from nsSVGFilterPaintCallback::Paint. This works because SetMatrixPropagation(PR_FALSE) is guaranteed to be in effect (it actually happens in nsAutoFilterInstance::nsAutoFilterInstance in nsSVGFilterFrame). The reason I'm doing this here is that I think the current code is buggy, since it doesn't do SetOverrideCTM(nsnull) and an associated NotifySVGChanged, which it really should.

Also with the latter change, there are no more real users of SetOverrideCTM and we can remove that stuff completely! I'll file a separate bug for that cleanup.
Assignee: nobody → roc
Attachment #343298 - Attachment is obsolete: true
Attachment #343349 - Flags: superreview?(mats.palmgren)
Attachment #343349 - Flags: review?(longsonr)
Attachment #343298 - Flags: review?(roc)
Comment on attachment 343349 [details] [diff] [review]
fix

sr=mats

In nsSVGFilterPaintCallback.h - you forgot to remove "@param aTransform ..."
In nsSVGUtils.cpp - please end comment sentences with a period.
Attachment #343349 - Flags: superreview?(mats.palmgren) → superreview+
(Reporter)

Comment 17

10 years ago
Comment on attachment 343349 [details] [diff] [review]
fix


I've moved the good bits of my original patch to bug 460210
Attachment #343349 - Flags: review?(longsonr) → review+
Pushed dfdda0399888, thanks
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Blocks: 460551
(Reporter)

Updated

10 years ago
Blocks: 456695
(Reporter)

Updated

10 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?
You need to log in before you can comment on or make changes to this bug.