Created attachment 99692 [details] Screenshot from the bug in four browser (Opera, Firefox, QtTestBrowser, Chrome) When an animated SVG uses the FEMorphology filter (erode or dilate) and the animation is done by JavaScript, the Y-radius does not change in some cases. The changes related to X-radius works without any problem. The issue should be somewhere around the event handling. Because if the radius attribute is set once, the svg does not update the value of Y-radius, but if the radius is set twice, the svg is updated first time with a wrong Y-radius and second with the correct one. for example: -if set the attribute of the filter twice the animation works as well example: (javascript) * dilate.setAttribute("radius",r); * dilate.setAttribute("radius",r); -but in this case not works: * dilate.setAttribute("radius", "1,7"); * dilate.setAttribute("radius", r + ",1"); the value of the X-radius at first is 1, after updated to 'r' and the value of the Y-radius at first is 7, but not updated to 1
Created attachment 99693 [details] Animated FEMorphology SVG that show the bug on webkit-based browsers Animated FEMorphology SVG that show the bug on webkit-based browsers. The SVG looks different in Opera or Firefox browser.
I discovered the reason of this bug, I fix it soon.
Great that you work on that!
I can confirm this bug with Chrome and QtTestBrowser as well.
The "Animated FEMorphology SVG that show the bug on webkit-based browsers" attachment ( https://bug-63930-attachments.webkit.org/attachment.cgi?id=99693 ) is not working at all, neighter in WebKit (here it results in this bug: "This page contains the following errors: error on line 2 at column 93: Space required after the Public Identifier Below is a rendering of the page up to the first error.") nor in FireFox 5 (here: XML-Verarbeitungsfehler: Syntax-Fehler Adresse: https://bug-63930-attachments.webkit.org/attachment.cgi?id=99693 Zeile Nr. 2, Spalte 96:<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"> -----------------------------------------------------------------------------------------------^ ). I think the attachment is broken.
Created attachment 99811 [details] Pixel difference in FEMorphology effect
Thanks, really wrong, I will fix it.
Nice catch! Can we add a Layout test for this?
Created attachment 99813 [details] Animated FEMorphology SVG that show the bug on webkit-based browsers
Created attachment 100943 [details] Pixel difference in FEMorphology effect Andras is on holiday, so I have updated the patch and the pixel test on Mac.
> Andras is on holiday, so I have updated the patch and the pixel test on Mac. We have to update this tests on the following platforms as well: ./chromium-linux/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png ./chromium-mac-leopard/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png ./chromium-mac/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png ./chromium-win/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png ./gtk/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png ./mac-leopard/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png I am CC'ing several people to do the update on their platforms as well.
Comment on attachment 100943 [details] Pixel difference in FEMorphology effect View in context: https://bugs.webkit.org/attachment.cgi?id=100943&action=review > Source/WebCore/svg/SVGFEMorphologyElement.cpp:132 > + if (attrName == SVGNames::radiusAttr) { > + bool isRadiusXChanged = morphology->setRadiusX(radiusX()); > + bool isRadiusYChanged = morphology->setRadiusY(radiusY()); > + return isRadiusXChanged || isRadiusYChanged; > + } Please put a comment here why this form is needed.
Created attachment 100960 [details] Pixel difference in FEMorphology effect A comment is added, and the ChangeLog is modified as well.
Comment on attachment 100943 [details] Pixel difference in FEMorphology effect View in context: https://bugs.webkit.org/attachment.cgi?id=100943&action=review > Source/WebCore/ChangeLog:11 > + If the X-radius attribute of FEMorphology filter is changed, the setRadiusX() will return true > + and the right side of || (OR) operator will be ignored. So right operand, > + the setRadiusY() won't be called and the Y-radius attribute won't updated neighter. > + To resolve this problem, we need enforce evaluate of both operand. This alone makes no sense without having read "return (morphology->setRadiusX(radiusX()) || morphology->setRadiusY(radiusY()));". I'd include this here, to make it easy understandable.
> This alone makes no sense without having read "return (morphology->setRadiusX(radiusX()) || morphology->setRadiusY(radiusY()));". > I'd include this here, to make it easy understandable. This is the previous changelog from a previous patch :P
Comment on attachment 100960 [details] Pixel difference in FEMorphology effect Clearing flags on attachment: 100960 Committed r91067: <http://trac.webkit.org/changeset/91067>
All reviewed patches have been landed. Closing bug.