Closed
Bug 997735
Opened 10 years ago
Closed 10 years ago
Fix origin confusion with filters applied to innerSVG, svg:use or foreignObject
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mstange, Assigned: mstange)
References
Details
Attachments
(6 files, 1 obsolete file)
5.63 KB,
image/svg+xml
|
Details | |
10.02 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
3.99 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
11.71 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
3.22 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
As far as I understand things, these testcases should all look the same:
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=flood-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&filter=flood-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&filter=flood-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&filter=flood-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=matrix-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&filter=matrix-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&filter=matrix-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&filter=matrix-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=flood-userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&filter=flood-userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&filter=flood-userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&filter=flood-userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=matrix-userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&filter=matrix-userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&filter=matrix-userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&filter=matrix-userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&mask=boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&mask=boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&mask=boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&mask=boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&mask=userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=use&mask=userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=innerSVG&mask=userSpace-atZero&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=foreignObject&mask=userSpace-at100&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=matrix-fillPaint-boundingBox&updateOffsetOn=initial
> https://bug997735.bugzilla.mozilla.org/attachment.cgi?id=8408236&offsetContainer=rect&filter=matrix-fillPaint-userSpace-at100&updateOffsetOn=initial
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.
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Missed the header file hunk.
Attachment #8408248 -
Attachment is obsolete: true
Attachment #8408248 -
Flags: review?(jwatt)
Attachment #8408256 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
Reftests are green with these patches.
Comment 9•10 years ago
|
||
> 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?
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Looks like jwatt is on holiday starting today. Robert, would you like to review these patches, or should I ask roc instead?
Comment 12•10 years ago
|
||
You should ask roc.
Assignee | ||
Updated•10 years ago
|
Attachment #8408245 -
Flags: review?(jwatt) → review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8408254 -
Flags: review?(jwatt) → review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8408256 -
Flags: review?(jwatt) → review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8408257 -
Flags: review?(jwatt) → review?(roc)
Assignee | ||
Updated•10 years ago
|
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+
Attachment #8408254 -
Flags: review?(roc) → review+
Attachment #8408256 -
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+
Attachment #8408267 -
Flags: review?(roc) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Green reftest try run at https://tbpl.mozilla.org/?tree=Try&rev=bc8df20e8a6b https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f94f19e05b https://hg.mozilla.org/integration/mozilla-inbound/rev/77a55da66827 https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd9986a84c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bd85e791c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d27ef04d88d
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f8f94f19e05b https://hg.mozilla.org/mozilla-central/rev/77a55da66827 https://hg.mozilla.org/mozilla-central/rev/fcd9986a84c5 https://hg.mozilla.org/mozilla-central/rev/f5bd85e791c3 https://hg.mozilla.org/mozilla-central/rev/8d27ef04d88d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•