Bug 55675 – Changing svg transform attribute to the same value triggers repaint

• Component: SVG
• Status: RESOLVED WONTFIX

[Patrick R. Gansterer]

When you have <rect fill="red" transform="scale(2)" /> and set the _fill_ attribute to "red" again it doesn't trigger a repaint.
If you set the _transform_ attribute of the rect to "scale(2)" again, it requests a repaint.

[Patrick R. Gansterer]

Created attachment 84772 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 84772 [details]
Patch

Are you sure it's worth the gain for the common-case, where the attribute indeed changed to a new value? Looks like an expensive test for a corner case...

[Patrick R. Gansterer]

Created attachment 84879 [details]
Alternative Patch (depends on bug 55829)

(In reply to comment #2)
> Are you sure it's worth the gain for the common-case, where the attribute indeed changed to a new value? Looks like an expensive test for a corner case...
I'm not 100% happy with the patch either, but I was my first try to fix this problem. I also added a a change comparison in the setAttribute directly, but many unit LayoutTest failed. This patch disables the whole svgAttributeChanged method, if the attribute stays the same but requires bug 55829.

[WebKit Review Bot]

Attachment 84879 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8105082

[Build Bot]

Attachment 84879 [details] did not build on win:
Build output: http://queues.webkit.org/results/8101158

[Collabora GTK+ EWS bot]

Attachment 84879 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8106078

[Early Warning System Bot]

Attachment 84879 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8098361

[WebKit Review Bot]

Attachment 84879 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8103166

[WebKit Review Bot]

Attachment 84879 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8105104

[Eric Seidel (no email)]

svgAttributeChanged is kinda a disaster anyway.  I can't remember why it's needed.

[Nikolas Zimmermann]

(In reply to comment #10)
> svgAttributeChanged is kinda a disaster anyway.  I can't remember why it's needed.
You're saying this since ages, it's not a disaster in anyway, but just fine.
I explained several times why it's needed (unify SVG DOM _and_ XML DOM changes).

[Adam Roben (:aroben)]

Comment on attachment 84772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84772&action=review

I don't understand why the code was moved from svgAttributeChanged to parseMappedAttribute. Your ChangeLog should probably explain this.

> Source/WebCore/ChangeLog:5
> +        Chaning svg transform attribute to the same value triggers repaint

Typo: Chaning

[Adam Roben (:aroben)]

Comment on attachment 84879 [details]
Alternative Patch (depends on bug 55829)

View in context: https://bugs.webkit.org/attachment.cgi?id=84879&action=review

> Source/WebCore/svg/SVGElement.cpp:386
> -    if (attr->name() != HTMLNames::styleAttr)
> +    if (type != ChangedAttributeToSameValue && attr->name() != HTMLNames::styleAttr)

Does HTMLElement or Element do this?

[Eric Seidel (no email)]

Just noticed this nearly 12-month-old bug.  Nico: Thoughts on this change?  You're much more familiar with SVG these days than I.

[Eric Seidel (no email)]

Comment on attachment 84879 [details]
Alternative Patch (depends on bug 55829)

With no response, and no answer to Adam's question, marking this r-.  Feel free to mark this r? again with questions answered.

[Dirk Schulze]

We do no longer have AttributeChangedType. Transforms should be handled by CSS in the future anyway.