Closed
Bug 895285
Opened 11 years ago
Closed 11 years ago
Firefox 22 SVG viewBox configure with jquery
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
People
(Reporter: artem, Assigned: heycam)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
257 bytes,
image/svg+xml
|
Details | |
2.57 KB,
patch
|
jwatt
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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);
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
Comment 2•11 years ago
|
||
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•11 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•11 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•11 years ago
|
||
Here's a reduced test case. It should alert "[object SVGRect]" but alerts "null".
Assignee | ||
Comment 6•11 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 8•11 years ago
|
||
With fewer unnecessary changes.
Attachment #778775 -
Attachment is obsolete: true
Attachment #778775 -
Flags: review?(jwatt)
Attachment #778779 -
Flags: review?(jwatt)
![]() |
||
Comment 10•11 years ago
|
||
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+
![]() |
||
Comment 11•11 years ago
|
||
Pff. Splinter really sucked there. Obviously I mean before the line: if (mHasBaseVal && viewBox == mBaseVal) {
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/64b83068144f
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/64b83068144f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 14•11 years ago
|
||
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:
? → ---
Flags: needinfo?(cam)
Assignee | ||
Comment 15•11 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•11 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•11 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 18•11 years ago
|
||
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+
Updated•11 years ago
|
tracking-firefox23:
--- → +
Comment 19•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f93d5a353d66 https://hg.mozilla.org/releases/mozilla-beta/rev/70568b0ba5b6
Comment 21•11 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•