Bug 34328 – Manipulating SVG element attributes in Javascript does not work as expected

• Component: SVG
• Status: RESOLVED FIXED

[Mohannad Hussain]

Our application is almost pure Javascript, where we do tons of SVG work. However, we find that manipulating attributes of SVG in Javascript either makes no difference, or renders the image wrong (wrong size, wrong clipping...etc).

In the URL example, http://www.go2mk.com/temporary/Webkit_SVG_Bug/webkit-bug-demo.xhtml, I am removing the viewBox attribute and nothing happens on the screen. The work around I discovered that works is to find the div containing the SVG and do div.innerHTML = div.innerHTML!! Removing the node completely, replacing it with another, then placing it back seems to work too.

We have seen this problem with many attributes other than viewBox, like the x,y coordinates as an example. Functions like forceRedraw() seem to make no difference. 

We can confirm this problem is happening on Windows and Mac. We have both Chrome and Safari running webkit 532.5 and 531.21.8 respectively.

This could be related to issue #16491, but I am not 100% sure.

Thanks for your help!

[Rob Buis]

Created attachment 56900 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 56900 [details]
Patch

Hi Rob,

in general the patch looks fine, though some comments, regarding code & test:
We're not adding new tests using the "manual style", but instead make use of make-js-test-wrappers.
You should add a test under svg/custom/script-tests/foo.js, and use make-js-test-wrappers to let it create svg/custom/foo.html for you.
This way you dynamically create the testcase, and use the existing nice framework for logging / verifying (shouldBe, etc..).
Can you have a look and change that?

Some notes on the code:
* Can you make parseViewBox take a "const String&" parameter, that would save the need to pass in two pointers. Sounds safer to encapsulate that code in parseViewBox, instead of letting the callers do that.
* I'd suggest to change parseViewBox to take a "FloatRect&" parameter instead of four individual floats. That would make the code more readable:
FloatRect viewBox;
if (!attr->value().isNull()) {
    if (!parseViewBox(document, attr->value(), viewBox))
        return true;
}
setViewBoxBaseValue(viewBox);

Thanks in advance,
Niko

[Rob Buis]

Hi Niko,

(In reply to comment #2)
> (From update of attachment 56900 [details])
> Hi Rob,
> 
> in general the patch looks fine, though some comments, regarding code & test:
> We're not adding new tests using the "manual style", but instead make use of make-js-test-wrappers.

[snip]

Thanks for the review, I'll look into improving the patch using these hints.
For the record, I wanted to add a comment that from the original bug report only
the viewBox problem could be reproduced, I am not sure what x,y coord problems the original reporter meant, but we can always add new bug reports for that later.
Cheers,

Rob.

[Mohannad Hussain]

Hi Niko and Rob,

At the time when I reported the bug, it seemed like anytime we change any DOM attributes of SVG elements in Javascript, nothing gets updated on the screen to reflect our changes. The xy coords I mentioned are the simply the ones like you'd have in <svg x="50%" y="50%" ....etc.

Like I mentioned in my original description, we also noticed that the SVG objects had functions like forceRedraw() but they didn't do anything either.

I hope I explained it a bit better this time, if not just let me know!

Mohannad

[Rob Buis]

Hello Mohannad

Nice to see such a quick reply. I'll try to do the same now :)

(In reply to comment #4)
> Hi Niko and Rob,
> 
> At the time when I reported the bug, it seemed like anytime we change any DOM attributes of SVG elements in Javascript, nothing gets updated on the screen to reflect our changes. The xy coords I mentioned are the simply the ones like you'd have in <svg x="50%" y="50%" ....etc.

Ok, I 'll test that later on.

> Like I mentioned in my original description, we also noticed that the SVG objects had functions like forceRedraw() but they didn't do anything either.

Looks unimplemented, with a reference to bug 11275. Is probably easy, but I don't know if it should layout + redraw or only redraw.

> I hope I explained it a bit better this time, if not just let me know!

Yes, thanks!
Cheers,

Rob.

[Rob Buis]

Created attachment 57127 [details]
Patch

[Rob Buis]

(In reply to comment #6)
> Created an attachment (id=57127) [details]
> Patch

I did a similar test for changing width on the <svg>, it did reflect the removeAttribute
call into the relevant DOM value. Let me know if such a test is needed in this patch and/or
whether it is covered elsewhere.
Cheers,

Rob.

[Rob Buis]

Created attachment 57423 [details]
Patch

[Rob Buis]

(In reply to comment #8)
> Created an attachment (id=57423) [details]
> Patch

I made this new patch since I noticed the old one broke viewSpec. Basically it relied
on the c pointer to be updated after parsing the viewBox. I also included a changed (improved!) result.
And now there are no regressions anymore.
Cheers,

Rob.

[Dirk Schulze]

Comment on attachment 57423 [details]
Patch

Patch looks great. Just some style issues:

WebCore/svg/SVGFitToViewBox.cpp:58
 +      float x = 0.0f, y = 0.0f, w = 0.0f, h = 0.0f;
WebKit style wants newlines for every declaration. Can you rename w and h to width and height? This would match the style in other code parts.

WebCore/svg/SVGFitToViewBox.cpp:99
 +          FloatRect r;
Can you please name the rect viewBox, like everywhere else?

WebCore/svg/SVGFitToViewBox.h:47
 +      bool parseViewBox(Document*, const String& s, FloatRect& viewBox);
The names s and viewBox are not needed here.

WebCore/svg/SVGViewSpec.cpp:53
 +      FloatRect r;
Rename r to viewBox and the current viewBox to maybe viewBoxString.

WebCore/svg/SVGViewSpec.cpp:104
 +                  FloatRect r;
dito

r- because of the style issues.

[Rob Buis]

Hello Dirk,

Thanks for the review!

(In reply to comment #10)
> (From update of attachment 57423 [details])
> Patch looks great. Just some style issues:
> 
> WebCore/svg/SVGFitToViewBox.cpp:58
>  +      float x = 0.0f, y = 0.0f, w = 0.0f, h = 0.0f;
> WebKit style wants newlines for every declaration. Can you rename w and h to width and height? This would match the style in other code parts.
> 
> WebCore/svg/SVGFitToViewBox.cpp:99
>  +          FloatRect r;
> Can you please name the rect viewBox, like everywhere else?
> 
> WebCore/svg/SVGFitToViewBox.h:47
>  +      bool parseViewBox(Document*, const String& s, FloatRect& viewBox);
> The names s and viewBox are not needed here.
> 
> WebCore/svg/SVGViewSpec.cpp:53
>  +      FloatRect r;
> Rename r to viewBox and the current viewBox to maybe viewBoxString.
> 
> WebCore/svg/SVGViewSpec.cpp:104
>  +                  FloatRect r;
> dito
> 
> r- because of the style issues.

Ok, that looks reasonable. I wonder if the normal webkit style check script caught some of that, but I did not pay attention to that anyway when I was
running the upload script. Looks like I can fix above tonight and then the patch should be landeable.
Cheers,

Rob.

[Rob Buis]

Created attachment 57480 [details]
Patch

[Dirk Schulze]

Comment on attachment 57480 [details]
Patch

Thanks for the changes. r=me.

[Dirk Schulze]

(In reply to comment #13)
> (From update of attachment 57480 [details])
> Thanks for the changes. r=me.

Rob, did you commit your patch?

[Rob Buis]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 57480 [details] [details])
> > Thanks for the changes. r=me.
> 
> Rob, did you commit your patch?

Moin Dirk,

Yes, please have a quick look in the ChangeLog or git log to be sure :) IIRC I wanted last week to wait a few days to see if there were any regressions and then forgot to close this. So feel free to close provided I did commit it :)
Cheers,

Rob.

[Dirk Schulze]

Comment on attachment 57480 [details]
Patch

Ahh found it. Got landed in 
http://trac.webkit.org/changeset/60501

[Dirk Schulze]

All patches landed. Closing bug now.