Closed Bug 769942 Opened 10 years ago Closed 10 years ago

DLBI broke invalidation of foreignObjects in filters


(Core :: SVG, defect)

Not set





(Reporter: jwatt, Assigned: jwatt)



(Keywords: regression)


(1 file)

The DLBI patches broke invalidation of foreignObjects that are under an SVG filter, so it seems we don't have tests for that. Basically what nsSVGForeignObjectFrame::GetInvalidRegion does is wrong. It simply transforms the fO's bounds into nsSVGOuterSVGFrame space and invalidates that area, completely ignoring any filters along the parent chain.

This seems to have happened because the nsSVGUtils::FindFilterInvalidation call in the original patch got changed to an nsSVGUtils::GetPostFilterVisualOverflowRect call after I killed off FindFilterInvalidation. Unfortunately, calling GetPostFilterVisualOverflowRect on the bounds transformed into nsSVGOuterSVGFrame space wasn't the right thing to replace it with though, since that skips filters along the way, only taking account of any filter that may be on the outer-<svg> itself. We should call nsSVGUtils::InvalidateBounds on the nsSVGForeignObjectFrame instead.
Attached patch patchSplinter Review
Attachment #638136 - Flags: review?(longsonr)
I'm leaving on vacation for a week in a few hours, so I'll probably not be able to land this. In fact it might be a good idea to wait and see if DLBI sticks before landing this so that if it needs to be backed out it doesn't add to the pain.

Matt, if you do get backed out, feel free to just directly integrate the changes and test from this patch directly into your own patch, rather than committing it as a separate patch. (If that makes things easier.)
Can we get into InvalidateIfChildrenDirty if the foreignObject doesn't have any children? Do we have anything with a foreignObject that doesn't have children where we get an invalidation coming through?
The 'kid' in InvalidateIfChildrenDirty is the anonymous child that is always there to wrap any real children that may exist.
To be clear, the anonymous child is always there, whether or not there are any real descendants.
Keywords: regression
Attachment #638136 - Flags: review?(longsonr) → review+
Checked in the test as:
Target Milestone: --- → mozilla17
Closed: 10 years ago
Resolution: --- → FIXED
Should this bug have been closed?
Yes. Thanks, Ryan.
To be clear, the regression test is what Matt wanted checked in. He will/has just incorporated the fixes from the patch into his own patches as appropriate.
Attachment #638136 - Flags: checkin?
You need to log in before you can comment on or make changes to this bug.