Closed
Bug 448831
Opened 16 years ago
Closed 16 years ago
DOM changes to filters don't work
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
People
(Reporter: longsonr, Assigned: longsonr)
References
Details
(Keywords: regression, testcase, verified1.9.1)
Attachments
(3 files, 4 obsolete files)
change the href on an image filter and nothing happens. Setting it to a relative URL and expecting it to resolve via xml:base doesn't work either.
Attachment #331998 -
Flags: superreview?(roc)
Attachment #331998 -
Flags: review?(roc)
+nsIntRect +nsSVGFEFloodElement::ComputeChangeBBox(const nsTArray<nsIntRect>& aSourceChangeBoxes, + const nsSVGFilterInstance& aInstance) +{ + return GetMaxRect(); +} Why have you added this? feFlood doesn't have any inputs so this should always return an empty rect (which is what the default implementation does). same for nsSVGFETurbulenceElement and nsSVGFEImageElement. Why not merge GetSrc and LoadImage somehow, since we always call GetSrc to use with LoadImage.
Assignee | ||
Comment 2•16 years ago
|
||
(In reply to comment #1) > Why have you added this? feFlood doesn't have any inputs so this should always > return an empty rect (which is what the default implementation does). same for > nsSVGFETurbulenceElement and nsSVGFEImageElement. > When you change a filter's attributes via the DOM you get this call stack... nsSVGFilterProperty::DoUpdate nsSVGFilterProperty::UpdateRect nsSVGFilterFrame::GetInvalidationRegion instance->ComputeOutputDirtyRect ComputeResultChangeBoxes info->mFE->ComputeChangeBBox as you say returns an empty rect info->mResultChangeBox is now an empty rect aDirtyRect is now an empty rect so no refresh happens The included reftests demonstrate this issue. > Why not merge GetSrc and LoadImage somehow, since we always call GetSrc to use > with LoadImage. > Good idea, done.
Attachment #331998 -
Attachment is obsolete: true
Attachment #332038 -
Flags: superreview?(roc)
Attachment #332038 -
Flags: review?(roc)
Attachment #331998 -
Flags: superreview?(roc)
Attachment #331998 -
Flags: review?(roc)
Assignee | ||
Comment 3•16 years ago
|
||
Assignee | ||
Comment 4•16 years ago
|
||
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 332038 [details] [diff] [review] merged GetSrc and LoadImage I do think this is wrong now but I don't know what the right thing to do is. I'll break out the other changes into a different fix.
Attachment #332038 -
Flags: superreview?(roc)
Attachment #332038 -
Flags: review?(roc)
Assignee | ||
Updated•16 years ago
|
Attachment #332038 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
Raised bug 448938 for the href issues. It seems that there are two ways filters change. Either their input changes which is catered for or there are DOM changes to the filter attributes which seems to be optimised out at the moment.
Summary: dynamic changes to filters without an in1 don't work → DOM changes to filters don't work
nsSVGFilterFrame::GetInvalidationRegion tells us how much to repaint when the source changes. We need a new method that tells us how much to repaint when the filter itself changes, and then call that method instead in the right places.
Flags: wanted1.9.1?
Assignee | ||
Comment 8•16 years ago
|
||
Here's a stab at it.
Attachment #332899 -
Flags: superreview?(roc)
Attachment #332899 -
Flags: review?(roc)
GetCoveredRegion shouldn't need an aRect parameter --- just pass null for aDirtyInputRect, ComputeCoveredRect isn't going to use it anyway. Otherwise, looks great.
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #332899 -
Attachment is obsolete: true
Attachment #333370 -
Flags: superreview?(roc)
Attachment #333370 -
Flags: review?(roc)
Attachment #332899 -
Flags: superreview?(roc)
Attachment #332899 -
Flags: review?(roc)
Sorry to mention this now, but I just realized I have an alternative patch from my branch which should also fix the bug. How do you feel about this? This patch also prepares some stuff so that filters can be used with non-SVG sources.
Attachment #333383 -
Flags: superreview?(mats.palmgren)
Attachment #333383 -
Flags: review?(longsonr)
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 333383 [details] [diff] [review] alternative fix >- // Compute filter effects region as per spec >- if (units == nsIDOMSVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) { >- if (!bbox) >- return NS_OK; >+ gfxRect filterArea = nsSVGUtils::GetRelativeRect(units, >+ &filter->mLengthAttributes[nsSVGFilterElement::X], bbox, aTarget); >+ filterArea.RoundOut(); There is no GetRelativeRect method in my copy of nsSVGUtils I think you need to submit another patch with that included. >+nsRect >+nsSVGFilterFrame::GetSourceForInvalidArea(nsIFrame *aTarget, const nsRect& aRect) This has no callers. Is this part of the non-SVG preparation. >+nsRect >+nsSVGFilterFrame::GetFilterBBox(nsIFrame *aTarget, const nsRect *aSourceBBox) The only caller to this passes null as aSourceBBox. Were you intending to use this argument at some later date? It should go if not. >+{ >+ nsRect result; >+ >+ nsAutoPtr<nsSVGFilterInstance> instance; >+ nsresult rv = CreateInstance(aTarget, nsnull, >+ nsnull, nsnull, aSourceBBox, getter_Transfers(instance)); >+ if (NS_SUCCEEDED(rv)) { >+ if (!instance) { >+ // The filter draws nothing, so the bbox is empty. >+ result = nsRect(); >+ } else { >+ // We've passed in the source's dirty area so the instance knows about it. >+ // Now we can ask the instance to compute the area of the filter output >+ // that's dirty. This comment is wrong. Too much cut and paste here. >diff --git a/layout/svg/base/src/nsSVGFilterFrame.h b/layout/svg/base/src/nsSVGFilterFrame.h >--- a/layout/svg/base/src/nsSVGFilterFrame.h >+++ b/layout/svg/base/src/nsSVGFilterFrame.h >@@ -33,48 +33,86 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef __NS_SVGFILTERFRAME_H__ > #define __NS_SVGFILTERFRAME_H__ > > #include "nsRect.h" >+#include "nsSVGUtils.h" Can't you get away with adding class nsSVGRenderState; etc? >+ >+nsresult >+nsSVGFilterInstance::ComputeSourceNeededRect(nsIntRect* aDirty) The only caller to this is the method above that has no callers itself.
Sorry, this is trying to be too clever. I should just submit a single patch that does the clip-path/mask/filter integration and that includes these changes. I'll do that tomorrow.
Assignee | ||
Updated•16 years ago
|
Attachment #333383 -
Flags: review?(longsonr) → review-
Depends on: 450340
Updated•16 years ago
|
Attachment #333383 -
Flags: superreview?(mats.palmgren)
Comment 14•16 years ago
|
||
Comment on attachment 333383 [details] [diff] [review] alternative fix I guess this patch is obsolete (now part of bug 450340?)
Assignee | ||
Comment 15•16 years ago
|
||
bug 450340 would fix this yes; and I think roc would rather get that one through review than this one.
Assignee | ||
Updated•16 years ago
|
Attachment #333370 -
Attachment is obsolete: true
Attachment #333370 -
Flags: superreview?(roc)
Attachment #333370 -
Flags: review?(roc)
Assignee | ||
Comment 16•16 years ago
|
||
Fixed by check in for bug 450340 Reftests added in 6554dc6c2dfe
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
Comment 17•15 years ago
|
||
verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre ID:20090414035212
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•