Closed Bug 1099197 Opened 5 years ago Closed 5 years ago

simple SVG: path not displayed in Firefox 35

Categories

(Core :: SVG, defect)

35 Branch
defect
Not set

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)

Attached image red-bar.svg
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
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
Ever confirmed: true
Keywords: regression, testcase
Blocks: 1077745
WRT Bug 932762, comment 3 - disabling preference svg.path-caching.enabled makes no difference.
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).
Attached patch poly.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8527320 - Flags: review?(jwatt)
Attached patch poly.txt (obsolete) — Splinter Review
Attachment #8527320 - Attachment is obsolete: true
Attachment #8527320 - Flags: review?(jwatt)
Attachment #8527322 - Flags: review?(jwatt)
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.
Attached patch poly.txt (obsolete) — Splinter Review
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 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.
(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]);
    }
  }
(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.
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.
Attached patch poly.txtSplinter Review
Attachment #8527351 - Attachment is obsolete: true
Attachment #8527351 - Flags: review?(jwatt)
Attachment #8527611 - Flags: review?(jwatt)
Comment on attachment 8527611 [details] [diff] [review]
poly.txt

Thanks, Robert!
Attachment #8527611 - Flags: review?(jwatt) → review+
Since we merged today, marking for 37 as well which is where this will first land.
https://hg.mozilla.org/mozilla-central/rev/4fcd95ddb540
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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?
Attachment #8527611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
Requested approval over in that bug.
Flags: needinfo?(jwatt)
Thanks for landing this Jonathan.
QA Whiteboard: [good first verify]
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.