Last Comment Bug 736791 - SVG: getAttribute('transform') does not report Y value when set with setTranslate(X,Y) where X=0
: SVG: getAttribute('transform') does not report Y value when set with setTrans...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Robert Longson
:
Mentors:
Depends on:
Blocks: 602759
  Show dependency treegraph
 
Reported: 2012-03-17 17:23 PDT by chris74avl
Modified: 2012-03-19 10:19 PDT (History)
6 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Firefox_setTranslate_getAttribute_Bug.svg (1.36 KB, image/svg+xml)
2012-03-17 17:23 PDT, chris74avl
no flags Details
patch (1.98 KB, patch)
2012-03-18 03:12 PDT, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
patch 2 (1.93 KB, patch)
2012-03-18 06:23 PDT, Robert Longson
jwatt: review+
Details | Diff | Splinter Review

Description chris74avl 2012-03-17 17:23:12 PDT
Created attachment 606915 [details]
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 2012-03-17 18:38:45 PDT
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 2012-03-18 03:12:49 PDT
Created attachment 606956 [details] [diff] [review]
patch
Comment 3 Jonathan Watt [:jwatt] 2012-03-18 03:15:47 PDT
Comment on attachment 606956 [details] [diff] [review]
patch

Nice.
Comment 4 Jonathan Watt [:jwatt] 2012-03-18 03:17:42 PDT
Thanks for the bug report, chris74avl@gmail.com!
Comment 5 Jonathan Watt [:jwatt] 2012-03-18 03:21:05 PDT
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] 2012-03-18 03:23:48 PDT
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 2012-03-18 03:36:44 PDT
(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] 2012-03-18 03:40:08 PDT
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 2012-03-18 03:41:08 PDT
And we have the same issue with scale. Changing things may break sites that use CSS selectors.
Comment 11 Jonathan Watt [:jwatt] 2012-03-18 05:31:18 PDT
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] 2012-03-18 05:35:00 PDT
(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 2012-03-18 06:23:41 PDT
Created attachment 606966 [details] [diff] [review]
patch 2

How about this?
Comment 14 Robert Longson 2012-03-18 06:58:42 PDT
(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] 2012-03-18 10:42:11 PDT
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 17 Phil Ringnalda (:philor, back in August) 2012-03-18 13:14:26 PDT
https://hg.mozilla.org/mozilla-central/rev/a9851e467a09
Comment 18 Matt Brubeck (:mbrubeck) 2012-03-19 10:19:07 PDT
https://hg.mozilla.org/mozilla-central/rev/0ea5739ba637

Note You need to log in before you can comment on or make changes to this bug.