Closed Bug 997735 Opened 8 years ago Closed 8 years ago

Fix origin confusion with filters applied to innerSVG, svg:use or foreignObject


(Core :: SVG, defect)

Not set





(Reporter: mstange, Assigned: mstange)




(6 files, 1 obsolete file)

Attached image testcase
Painting and invalidation of filtered and masked SVG container frames is broken when the container (e.g. inner SVG, "use", or foreignObject) has its own offset. The code is confused between user space and frame space and the conversions between the spaces are different in different places.
As far as I understand things, these testcases should all look the same:


They don't.

Webkit/Blink and IE11 agree with me on all counts except for foreignObject: IE doesn't support foreignObject at all, and it looks like filters on foreignObject are just broken in Webkit/Blink.
Attached patch part 1: reftestSplinter Review
Comment 1 in reftest form. Please review this first; if the test is wrong, the patches will be wrong, too.
Attachment #8408245 - Flags: review?(jwatt)
There are three spaces that nsSVGIntegrationUtils needs to convert between. I'll call them "frame space", "bounding box frame space" and "user space".

"Bounding box frame space" has its origin at the top left of the union of a frame's border boxes over all continuations.

For SVG frames, "frame space" and "bounding box frame space" are the same because SVG frames don't have multiple continuations.

For non-SVG frames, "bounding box frame space" and "user space" are the same.

However, for SVG frames, "bounding box frame space" and "user space" are different! For example, for a <rect x="100" y="100">, the point 0,0 in frame space is at the rect's top left corner, but the point 0,0 in user space is 100,100 pixels away from the rect's corner.

nsSVGIntegrationUtils::GetOffsetToUserSpace took the non-SVG viewpoint, but it's misleading for SVG frames.
Attachment #8408248 - Flags: review?(jwatt)
This function calculates the offset between "bounding box frame space" and "user space" for SVG frames. For non-SVG frames it returns no offset.

It's crucial that this is consistent with what nsSVGUtils::GetBBox does. 

nsFilterInstance has several methods that are called by consumers before the actual painting, e.g. to calculate post filter extents or invalidation regions. Those nsFilterInstance APIs have their input and output values in "bounding box frame space" coordinates, but if the filter units are "objectBoundingBox", then those methods also do calculations involving the result of nsSVGUtils::GetBBox. So a consistent conversion is very important.
Attachment #8408254 - Flags: review?(jwatt)
Missed the header file hunk.
Attachment #8408248 - Attachment is obsolete: true
Attachment #8408248 - Flags: review?(jwatt)
Attachment #8408256 - Flags: review?(jwatt)
This makes the offset consistent between filter painting preparation and the calculations inside nsFilterInstance.

It doesn't make any changes to nsSVGUtils::PaintFrameWithEffects, so turning off svg.display-lists.painting.enabled will fail some of the reftests. I tried to fix it but I didn't know what I was doing, so I'm leaving out that part for now.
Attachment #8408257 - Flags: review?(jwatt)
Not very closely related to this bug but necessary for fixing the invalidation part of the reftests: Without this patch, when changing the x/y attributes of svg:use, innerSVG and foreignObject, we were relying on the transform changes of the children to trigger the right invalidations. However, changes to those attributes can also change the filter region. And there's a difference between moving children in a fixed filter region and moving the filter region along with the children: In the first case, we wouldn't need to invalidate anything outside the old filter region, because those parts of the children would be clipped away anyway. But when the filter region changes, we need to invalidate both the old and the new filter region. Also, when the filter has primitives without inputs, e.g. flood or turbulence, the filtered frame needs to be invalidate even if it has no children.
Attachment #8408267 - Flags: review?(jwatt)
Reftests are green with these patches.
> bool
>  nsSVGIntegrationUtils::HitTestFrameForEffects(nsIFrame* aFrame, const nsPoint& aPt)
>  {
   nsPoint toUserSpace;
   if (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT) {
+    // XXXmstange Isn't this wrong for svg:use and innerSVG frames?

nsSVGIntegrationUtils was supposed to be for SVG effects on html frames i.e. it isn't supposed to be called if you have a use or inner SVG frame. SVG frames should use nsSVGEffects. Are we getting mixed up now?
We're getting thoroughly mixed up. Since SVG uses display lists now, SVG frames with effects will always go through nsSVGIntegrationUtils instead of nsSVGUtils. I think we need to unify nsSVGIntegrationUtils and nsSVGUtils at some point.
Looks like jwatt is on holiday starting today. Robert, would you like to review these patches, or should I ask roc instead?
You should ask roc.
Attachment #8408245 - Flags: review?(jwatt) → review?(roc)
Attachment #8408254 - Flags: review?(jwatt) → review?(roc)
Attachment #8408256 - Flags: review?(jwatt) → review?(roc)
Attachment #8408257 - Flags: review?(jwatt) → review?(roc)
Attachment #8408267 - Flags: review?(jwatt) → review?(roc)
Comment on attachment 8408245 [details] [diff] [review]
part 1: reftest

Review of attachment 8408245 [details] [diff] [review]:

::: layout/reftests/invalidation/filter-userspace-offset.svg
@@ +145,5 @@
> +  },
> +  timeout: function () {
> +    setTimeout(updateOffset, 500);
> +  }
> +})[options.updateOffsetOn]();

This is somewhat obscure. Instead, just define three separate functions and when setting up "options", set options.updateOffset to the desired function, and call options.updateOffset() here.
Attachment #8408245 - Flags: review?(roc) → review+
Comment on attachment 8408257 [details] [diff] [review]
part 4: Use consistent offset both in nsSVGIntegrationUtils::PaintFramesWithEffects and in nsFilterInstance::GetUserSpaceToFrameSpaceInCSSPxTransform

Review of attachment 8408257 [details] [diff] [review]:

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +473,5 @@
>    }
> +  // After applying only "offsetToBoundingBox", aCtx would have its origin at
> +  // the top left corner of aFrame's bounding box (over all continuations).
> +  // However, SVG painting needs the origin to be at located the origin of the

"to be located at"
Attachment #8408257 - Flags: review?(roc) → review+
Blocks: 999964
You need to log in before you can comment on or make changes to this bug.