Closed Bug 895285 Opened 11 years ago Closed 11 years ago

Firefox 22 SVG viewBox configure with jquery


(Core :: SVG, defect)

22 Branch
Windows 7
Not set



Tracking Status
firefox22 --- wontfix
firefox23 + verified
firefox24 + verified
firefox25 + verified


(Reporter: artem, Assigned: heycam)



(Keywords: regression)


(2 files, 1 obsolete file)

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 -

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);
Regression range:

Maybe this bug:
Robert Longson — Bug 785606 - Support viewBox=none from SVG 1.2 Tiny r=jwatt
Component: Untriaged → SVG
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
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?
Assignee: nobody → longsonr
Flags: needinfo?(longsonr)
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.
Assignee: longsonr → cam
Flags: needinfo?(longsonr)
document.querySelector("svg").viewBox.baseVal is null after the viewBox="" attribute is re-set to "0 0 10000 10000", which seems wrong.
Attached image test
Here's a reduced test case.  It should alert "[object SVGRect]" but alerts "null".
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #778775 - Flags: review?(jwatt)
Attached patch patchSplinter Review
With fewer unnecessary changes.
Attachment #778775 - Attachment is obsolete: true
Attachment #778775 - Flags: review?(jwatt)
Attachment #778779 - Flags: review?(jwatt)
Blocks: 785606
Comment on attachment 778779 [details] [diff] [review]

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.
Attachment #778779 - Flags: review?(jwatt) → review+
Pff. Splinter really sucked there. Obviously I mean before the line:

  if (mHasBaseVal && viewBox == mBaseVal) {
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
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?
Flags: needinfo?(cam) → needinfo?(jwatt)
Comment on attachment 778779 [details] [diff] [review]

[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
Attachment #778779 - Flags: approval-mozilla-beta?
Attachment #778779 - Flags: approval-mozilla-aurora?
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 on attachment 778779 [details] [diff] [review]

I'll update the status flags to reflect this approval, given low risk and the additional explanation we can take this to Beta.
Attachment #778779 - Flags: approval-mozilla-beta?
Attachment #778779 - Flags: approval-mozilla-beta+
Attachment #778779 - Flags: approval-mozilla-aurora?
Attachment #778779 - Flags: approval-mozilla-aurora+
Thanks for landing, Ryan.
Flags: needinfo?(jwatt)
(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.
Verified fixed FF 24b4 Win 7 x64.
Keywords: verifyme
Verified fixed FF 25.0a2 (2013-09-13), 26.0a1 (2013-09-12) Win 7 x64
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.