Closed
Bug 1099197
Opened 10 years ago
Closed 9 years ago
simple SVG: path not displayed in Firefox 35
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | + | verified |
firefox36 | + | verified |
People
(Reporter: mephinet, Assigned: longsonr)
References
Details
(Keywords: regression, testcase, Whiteboard: [bugday-20141217])
Attachments
(2 files, 3 obsolete files)
1.85 KB,
image/svg+xml
|
Details | |
3.17 KB,
patch
|
jwatt
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20141114004002 Steps to reproduce: The attached SVG is a simple red block, which is part of our company logo. It displayed fine in Firefox 24.6, but no longer shows at all in Firefox 35.0a2. Also displays fine in Inkscape (which was used to edit the file). Actual results: SVG is completely blank Expected results: SVG should show a red bar.
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 Might be related to #1076435
Assignee | ||
Updated•10 years ago
|
Component: Untriaged → SVG
Product: Firefox → Core
Regression range: good=2014-10-05 bad=2014-10-06 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4b3d816c21fd&tochange=0ed32d9a42d6 Suspected bugs: Jonathan Watt — Bug 932762, part 1 - Get rid of nsSVGPathGeometryElement::CreatePathBuilder and make BuildPath's aBuilder argument non-optional. r=longsonr Jonathan Watt — Bug 1077745 - Get rid of the scaling hack and GetCanvasTM() call in nsSVGPathGeometryFrame::GetBBoxContribution. r=longsonr
Status: UNCONFIRMED → NEW
status-firefox34:
--- → unaffected
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Ever confirmed: true
Keywords: regression,
testcase
WRT Bug 932762, comment 3 - disabling preference svg.path-caching.enabled makes no difference.
Comment 4•10 years ago
|
||
Darn. Backing out bug 1077745 now would involve backing out a bunch of other stuff too. I'm also not sure that that (or hacking something in) is worth it for cairo backed DrawTargets (Linux, XP and more recent Windows machines with blacklisted drivers) since we're quite close to switching to Skia I believe (bug 687187). That will also improve performance on such machines (as will bug 627699, which I think we're also close on).
Assignee | ||
Comment 5•9 years ago
|
||
Assignee: nobody → longsonr
Attachment #8527320 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8527320 -
Attachment is obsolete: true
Attachment #8527320 -
Flags: review?(jwatt)
Attachment #8527322 -
Flags: review?(jwatt)
Comment 7•9 years ago
|
||
Ah, great! I think it's a bit more efficient to use min/max variables and code like this: if (x < xmin) { xmin = x; } else if (x > xmax) { xmax = x; } Also I think we should return tight bounds when possible, which means transforming the points before setting the min/max variables.
Assignee | ||
Comment 8•9 years ago
|
||
I had tight bounds by only supporting rectilinear transforms. I guess its easy enough to support any transform though.
Attachment #8527322 -
Attachment is obsolete: true
Attachment #8527322 -
Flags: review?(jwatt)
Attachment #8527351 -
Flags: review?(jwatt)
Comment 9•9 years ago
|
||
Comment on attachment 8527351 [details] [diff] [review] poly.txt Review of attachment 8527351 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/svg/nsSVGPolyElement.cpp @@ +131,5 @@ > + aBounds->SetEmpty(); > + return true; > + } > + > + if (aStrokeWidth > 0) { Can you add a comment here explaining that this is because we don't handle stroke-miterlimit etc. yet. @@ +138,5 @@ > + > + if (aTransform.IsRectilinear()) { > + // We can avoid transforming each point and just transform the result > + Point min(points[0].mX, points[0].mY); > + Point max(min); Ah, I had in mind four separate floats like: Float xmin, xmax, ymin, ymax; If you want to use two Points I think they would be better named topLeft and bottomRight.
Comment 10•9 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #9) > Ah, I had in mind four separate floats like: Actually, to avoid code duplication I think it would be better to add a ExpandToEnclose(Point& aPoint) method to BaseRect. (We could use that in a bunch of other places too.) You could then write: if (aTransform.IsRectilinear()) { // We can avoid transforming each point and just transform the result, // which is important for very long point lists. Rect bounds(points[0], Size()); for (uint32_t i = 1; i < points.Length(); ++i) { bounds.ExpandToEnclose(points[i]); } *aBounds = aTransform.TransformBounds(bounds); } else { *aBounds = Rect(aTransform * points[0], Size()); for (uint32_t i = 1; i < points.Length(); ++i) { aBounds->ExpandToEnclose(aTransform * points[i]); } }
Comment 11•9 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #10) > Actually, to avoid code duplication I think it would be better to add a > ExpandToEnclose(Point& aPoint) method to BaseRect. I'm a Moz2D peer BTW, so I can give you r+ for that in the same review request.
Comment 12•9 years ago
|
||
Note than SVGPoint has a cast operator to Moz2D Point, so you shouldn't need to explicitly create a Point from an SVGPoint's members.
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8527351 -
Attachment is obsolete: true
Attachment #8527351 -
Flags: review?(jwatt)
Attachment #8527611 -
Flags: review?(jwatt)
Comment 14•9 years ago
|
||
Comment on attachment 8527611 [details] [diff] [review] poly.txt Thanks, Robert!
Attachment #8527611 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 15•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0bf49552edec
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fcd95ddb540
Flags: in-testsuite-
Comment 17•9 years ago
|
||
Since we merged today, marking for 37 as well which is where this will first land.
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
tracking-firefox37:
--- → +
https://hg.mozilla.org/mozilla-central/rev/4fcd95ddb540
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Updated•9 years ago
|
status-firefox37:
affected → ---
tracking-firefox37:
+ → ---
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8527611 [details] [diff] [review] poly.txt Approval Request Comment [Feature/regressing bug #]: bug 1077745 [User impact if declined]: some SVG polygons/polylines with large coordinates will not render on some platforms [Describe test coverage new/current, TBPL]: existing mochitest coverage should cover it (no new tests added) [Risks and why]: any errors in the patch would affect SVG polygon bounding box calculations and might lead to polygons/polylines not being rendered. [String/UUID change made/needed]: none
Attachment #8527611 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8527611 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•9 years ago
|
||
Backed out for bustage. https://treeherder.mozilla.org/ui/logviewer.html#?job_id=421538&repo=mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/7242a447378b
Assignee | ||
Comment 22•9 years ago
|
||
We'd need to land bug 1080688 on Aurora. Jonathan, do you want to do that to fix this for 35?
Depends on: 1080688
Flags: needinfo?(longsonr) → needinfo?(jwatt)
Assignee | ||
Comment 25•9 years ago
|
||
Thanks for landing this Jonathan.
Assignee | ||
Updated•9 years ago
|
Keywords: branch-patch-needed
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 26•9 years ago
|
||
Reproduced with Nightly 2014-11-04 with the instruction from comment 0 and on Windows 7 x64. Verified as fixed with Firefox 35 beta 4 (Build ID: 20141216120925) and latest Aurora 36.0a2 (Build ID: 20141217004003) Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 [bugday-20141217]
Whiteboard: [bugday-20141217]
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•