Firefox 22 SVG viewBox configure with jquery

VERIFIED FIXED in Firefox 23

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: artem, Assigned: heycam)

Tracking

({regression})

22 Branch
mozilla25
x86_64
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox22 wontfix, firefox23+ verified, firefox24+ verified, firefox25+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
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
Status: UNCONFIRMED → NEW
status-firefox22: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
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)
(Assignee)

Comment 3

6 years ago
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)
(Assignee)

Comment 4

6 years ago
document.querySelector("svg").viewBox.baseVal is null after the viewBox="" attribute is re-set to "0 0 10000 10000", which seems wrong.
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 778767 [details]
test

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

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
Created attachment 778775 [details] [diff] [review]
patch
Attachment #778775 - Flags: review?(jwatt)
(Assignee)

Comment 8

6 years ago
Created attachment 778779 [details] [diff] [review]
patch

With fewer unnecessary changes.
Attachment #778775 - Attachment is obsolete: true
Attachment #778775 - Flags: review?(jwatt)
Attachment #778779 - Flags: review?(jwatt)

Updated

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

  if (mHasBaseVal && viewBox == mBaseVal) {
https://hg.mozilla.org/mozilla-central/rev/64b83068144f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: ? → ---
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Flags: needinfo?(cam)
(Assignee)

Comment 15

6 years ago
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)
(Assignee)

Comment 16

6 years ago
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
Attachment #778779 - Flags: approval-mozilla-beta?
Attachment #778779 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 17

6 years ago
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]
patch

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+
status-firefox23: wontfix → affected
tracking-firefox23: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/f93d5a353d66
https://hg.mozilla.org/releases/mozilla-beta/rev/70568b0ba5b6
status-firefox22: affected → wontfix
status-firefox23: affected → fixed
status-firefox24: affected → fixed
status-firefox25: affected → fixed
(Assignee)

Comment 20

5 years ago
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.
status-firefox23: fixed → verified
Verified fixed FF 24b4 Win 7 x64.
status-firefox24: fixed → verified
Keywords: verifyme
Verified fixed FF 25.0a2 (2013-09-13), 26.0a1 (2013-09-12) Win 7 x64
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.