Bug 34328 - Manipulating SVG element attributes in Javascript does not work as expected
Summary: Manipulating SVG element attributes in Javascript does not work as expected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://www.go2mk.com/temporary/Webkit...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-29 06:26 PST by Mohannad Hussain
Modified: 2010-06-08 07:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.23 KB, patch)
2010-05-24 10:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2010-05-26 13:06 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.93 KB, patch)
2010-05-30 11:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (12.18 KB, patch)
2010-05-31 10:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mohannad Hussain 2010-01-29 06:26:33 PST
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!
Comment 1 Rob Buis 2010-05-24 10:01:56 PDT
Created attachment 56900 [details]
Patch
Comment 2 Nikolas Zimmermann 2010-05-26 05:18:25 PDT
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
Comment 3 Rob Buis 2010-05-26 05:30:21 PDT
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.
Comment 4 Mohannad Hussain 2010-05-26 05:54:43 PDT
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
Comment 5 Rob Buis 2010-05-26 06:18:59 PDT
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.
Comment 6 Rob Buis 2010-05-26 13:06:39 PDT
Created attachment 57127 [details]
Patch
Comment 7 Rob Buis 2010-05-26 13:19:14 PDT
(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.
Comment 8 Rob Buis 2010-05-30 11:21:08 PDT
Created attachment 57423 [details]
Patch
Comment 9 Rob Buis 2010-05-30 11:23:18 PDT
(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.
Comment 10 Dirk Schulze 2010-05-30 23:31:36 PDT
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.
Comment 11 Rob Buis 2010-05-31 00:35:41 PDT
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.
Comment 12 Rob Buis 2010-05-31 10:50:11 PDT
Created attachment 57480 [details]
Patch
Comment 13 Dirk Schulze 2010-06-01 06:33:00 PDT
Comment on attachment 57480 [details]
Patch

Thanks for the changes. r=me.
Comment 14 Dirk Schulze 2010-06-08 06:46:56 PDT
(In reply to comment #13)
> (From update of attachment 57480 [details])
> Thanks for the changes. r=me.

Rob, did you commit your patch?
Comment 15 Rob Buis 2010-06-08 06:56:06 PDT
(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.
Comment 16 Dirk Schulze 2010-06-08 07:00:03 PDT
Comment on attachment 57480 [details]
Patch

Ahh found it. Got landed in 
http://trac.webkit.org/changeset/60501
Comment 17 Dirk Schulze 2010-06-08 07:00:43 PDT
All patches landed. Closing bug now.