736791 - SVG: getAttribute('transform') does not report Y value when set with setTranslate(X,Y) where X=0

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[chris74avl]

Attachment: Firefox_setTranslate_getAttribute_Bug.svg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

Retrieved the "transform" attribute on an SVG element with getAttribute('transform') after setting it with setTranslate(X,Y) where X=0.

Full example and documentation in attached file.


Actual results:

It reports "translate(0)".


Expected results:

It should report "translate(0 Y)".

Comment 1

[Alice0775 White]

Regression window:(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/c722928d8b69
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110926 Firefox/9.0a1 ID:20110926030901
Fails:
http://hg.mozilla.org/mozilla-central/rev/44ef245b8706
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110926 Firefox/9.0a1 ID:20110926073617
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c722928d8b69&tochange=44ef245b8706


Regression window:(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/22ae18b4d013
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110925 Firefox/9.0a1 ID:20110925120518
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/dbde35bf04e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110925 Firefox/9.0a1 ID:20110925140618
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22ae18b4d013&tochange=dbde35bf04e9

Suspected: Bug 602759

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 606956 [details] [diff] [review]
patch

Nice.

Comment 4

[Jonathan Watt [:jwatt]]

Thanks for the bug report, [e-mail]!

Comment 5

[Jonathan Watt [:jwatt]]

Actually, Robert, can you make it |if (mMatrix.x0 != mMatrix.y0 || mMatrix.x0 != 0.0f || mMatrix.y0 != 0.0f)|. That's more likely to give the format that was originally input, I think.

Comment 6

[Jonathan Watt [:jwatt]]

Ultimately we should carry around info on whether the user specified a single or double number, but we need to do that for other transform types too, and that would be a change for trunk, not branches.

Comment 7

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> Ultimately we should carry around info on whether the user specified a
> single or double number, but we need to do that for other transform types
> too, and that would be a change for trunk, not branches.

I don't want to do that in this bug as it would make things inconsistent. Should rotate(30, 0, 0) stay as it is too? If so it should all be done in one bug and not this one. If you want that change you should raise a new bug and we should consider all cases. You might also want to see what Opera/Webkit/IE9 does and try for consistency.

Comment 8

[Jonathan Watt [:jwatt]]

Oh, yeah, I wasn't meaning that part should be done in this bug, or that you should do the work.

Comment 9

[Robert Longson [:longsonr]]

And we have the same issue with scale. Changing things may break sites that use CSS selectors.

Comment 10

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a9851e467a09

Comment 11

[Jonathan Watt [:jwatt]]

You seemed to be replying to comment 6 (since that's the comment you quoted and replied to earlier, and you start your next comment with "And..."), but since you pushed without the changes requested in comment 5 I assume that's the one you are objecting to? I still think we should do that though.

The check in the old code was |if (dy != 0.0f)|:

  https://bugzilla.mozilla.org/attachment.cgi?id=555284&action=diff

So, unless the transform was translate(0), it would _always_ serialize single numbers to a double number. That also seems consistent with the SVG I've seen, which tend to have things like translate(10,10) (whereas, yes, for scale it is common to use a single number for both x and y, as in scale(10)). So for compatibility with what we used to do, and what I've seen in SVG in the wild, I'd prefer to treat translate() and scale() differently and favor the double number format for 'translate()'.

Note that the old check would also incorrectly serialize double numbers of the form translate(X,0) to the single number translate(X), but your x!=y check would fix that.

So for me I'd be happy with:

  if (mMatrix.x0 != mMatrix.y0 || mMatrix.y0 != 0.0f)

(Forget the additional |mMatrix.y0 != 0.0f| since, on reflection, that would be redundant.)

A comment explaining that the reason for the |mMatrix.y0 != 0.0f| check for translate() is because for translate() it's more common for SVG to use double number format, even whet both components are the same, would also be appropriate.

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> (Forget the additional |mMatrix.y0 != 0.0f|

That should have been x0, not y0.

Comment 13

[Robert Longson [:longsonr]]

Attachment: patch 2

How about this?

Comment 14

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> You seemed to be replying to comment 6 (since that's the comment you quoted
> and replied to earlier, and you start your next comment with "And..."), but
> since you pushed without the changes requested in comment 5 I assume that's
> the one you are objecting to? I still think we should do that though.
> 
> The check in the old code was |if (dy != 0.0f)|:

So that's what I'm going back to.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 606966 [details] [diff] [review]
patch 2

Ah, the part where I said "the old check would also incorrectly serialize double numbers" was actually due to missing that the spec says that means y is zero (as opposed to it having the same value as x). So yes, we should definitely go back to the old pre-bug 602759 behavior then.

So that we're less likely to misunderstand this code in future, can you also add this comment just before the |if|:

  // The spec say that if Y is not provided, it is assumed to be zero.

Comment 16

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea5739ba637

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a9851e467a09

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0ea5739ba637