895285 - Firefox 22 SVG viewBox configure with jquery

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Artem]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

On Initial load i configure svg viewbox from 0 to 10000 in a div with width=600, height=600. Then draw Grid -all is fine.

Test page - http://test5.w510.com.ua/test22.html

Works fine in Google Chrome 28.0.1500.72
Works fine in Internet Explorer 10.0.9200.16635
Works fine in Opera 12.15
Works fine in Safari 5.1.7
Works fine in Firefox < 22 
DO NOT WORK in Firefox 22 






Actual results:

Then i call draw function again on Button Click event - and drawing go without scale factor.


Expected results:

draw must be done scaled.
Problem in 
svg.configure({viewBox: X + " " + Y + " " + ViewSize + " " + ViewSize, width: 600, height: 600}, true);

Comment 1

[Loic]

Regression range:
good=2013-02-27
bad=2013-02-28
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7632ab657e5&tochange=b0e08db3bc2a

Maybe this bug:
Robert Longson — Bug 785606 - Support viewBox=none from SVG 1.2 Tiny r=jwatt

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Robert - should we look at backing out bug 785606 from FF23 for this regression? What is the user impact here? 

Would you be able to provide a low risk forward fix for FF24?

Comment 3

[Cameron McCormack (:heycam)]

Robert is away until next month.

I don't think it is likely that people are relying on viewBox="none" yet.  I'd be OK with backing out in 23 and looking at a fix for 24.

Comment 4

[Cameron McCormack (:heycam)]

document.querySelector("svg").viewBox.baseVal is null after the viewBox="" attribute is re-set to "0 0 10000 10000", which seems wrong.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: test

Here's a reduced test case.  It should alert "[object SVGRect]" but alerts "null".

Comment 6

[Cameron McCormack (:heycam)]

The test relies on setting viewBox="" back to the same value it had before the attribute was removed.  We find the same value, so return early from nsSVGViewBox::SetBaseValueString, but we don't set mHasBaseVal before returning.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

With fewer unnecessary changes.

Comment 9

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=640d6cff7190

Comment 10

[Jonathan Watt [:jwatt]]

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

Review of attachment 778779 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks for the fix.

::: content/svg/content/src/nsSVGViewBox.cpp
@@ +161,5 @@
>  
>    nsresult rv = ToSVGViewBoxRect(aValue, &viewBox);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }

Please add the following comment before the |if|:

  // Comparison against mBaseVal is only valid if we currently have a base val.

Comment 11

[Jonathan Watt [:jwatt]]

Pff. Splinter really sucked there. Obviously I mean before the line:

  if (mHasBaseVal && viewBox == mBaseVal) {

Comment 12

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64b83068144f

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/64b83068144f

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

It's too late in FF23 to do the backout but if as you state in comment 3 it's not widely used yet and we shipped this regression in FF22 without much user complaint/fallout I think we can track this for uplift to FF24 and higher.  Please let me know if there's anything I'm missing that would impact our FF23 users on release if we do nothing here.

Comment 15

[Cameron McCormack (:heycam)]

Hmm.  It would be good to avoid having this broken in Release.  When I said I thought that it's not widely used yet, I was referring to the new viewBox="none" feature, which is the bug 785606 feature that would have been backed out.  The bug here is that setting the viewBox="" attribute to certain values will be ignored.  There is more chance of that being used, especially with toolkits like D3 that are using SVG these days.

I think at this stage landing the fix from this bug is a lot simpler than backing out bug 785606.  So if that's possible I would like to land it on 23.  Jonathan, what do you think?

Comment 16

[Cameron McCormack (:heycam)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 785606
User impact if declined: Certain dynamic SVG documents will not render properly
Testing completed (on m-c, etc.): Regression tests added, been on m-c for a few days, manually tested
Risk to taking this patch (and alternatives if risky): Low IMO
String or IDL/UUID changes made by this patch: N/A

Comment 17

[Artem]

My application draw different complex technical drawings of different size.
I'm sure there is some application that need this for zoom-in zoom-out feature.
This bug is critical for complex svg usage.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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

I'll update the status flags to reflect this approval, given low risk and the additional explanation we can take this to Beta.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f93d5a353d66
https://hg.mozilla.org/releases/mozilla-beta/rev/70568b0ba5b6

Comment 20

[Cameron McCormack (:heycam)]

Thanks for landing, Ryan.

Comment 21

[Paul Silaghi, QA [:pauly]]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Here's a reduced test case.  It should alert "[object SVGRect]" but alerts
> "null".
Verified fixed FF 23b10 Win 7 x64.

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 24b4 Win 7 x64.

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 25.0a2 (2013-09-13), 26.0a1 (2013-09-12) Win 7 x64