The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: chris74avl, Assigned: Robert Longson)

Tracking

({regression})

Trunk
mozilla14
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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)".
(Reporter)

Updated

5 years ago
Summary: SVG: getAttribute('transform') does not report Y value when setTranslate(X,Y) with X=0 → SVG: getAttribute('transform') does not report Y value when set with setTranslate(X,Y) where X=0

Comment 1

5 years ago
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
Blocks: 602759
Status: UNCONFIRMED → NEW
Component: Untriaged → SVG
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Product: Firefox → Core
QA Contact: untriaged → general
Version: 11 Branch → 9 Branch
Version: 9 Branch → Trunk
(Assignee)

Comment 2

5 years ago
Created attachment 606956 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #606956 - Flags: review?(jwatt)
Comment on attachment 606956 [details] [diff] [review]
patch

Nice.
Attachment #606956 - Flags: review?(jwatt) → review+

Updated

5 years ago
Hardware: x86_64 → All
Thanks for the bug report, chris74avl@gmail.com!
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.
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.
(Assignee)

Comment 7

5 years ago
(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.
Oh, yeah, I wasn't meaning that part should be done in this bug, or that you should do the work.
(Assignee)

Comment 9

5 years ago
And we have the same issue with scale. Changing things may break sites that use CSS selectors.
(Assignee)

Comment 10

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a9851e467a09
Flags: in-testsuite+
Target Milestone: --- → mozilla14
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.
(In reply to Jonathan Watt [:jwatt] from comment #11)
> (Forget the additional |mMatrix.y0 != 0.0f|

That should have been x0, not y0.
(Assignee)

Comment 13

5 years ago
Created attachment 606966 [details] [diff] [review]
patch 2

How about this?
Attachment #606966 - Flags: review?(jwatt)
(Assignee)

Comment 14

5 years ago
(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 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.
Attachment #606966 - Flags: review?(jwatt) → review+
(Assignee)

Comment 16

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea5739ba637
https://hg.mozilla.org/mozilla-central/rev/a9851e467a09
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/0ea5739ba637
You need to log in before you can comment on or make changes to this bug.