Closed Bug 1085092 Opened 6 years ago Closed 6 years ago

SVG Polygons are not closed

Categories

(Core :: SVG, defect)

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 + fixed

People

(Reporter: dev, Assigned: jwatt)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image polygon-star.svg
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141018030201

Steps to reproduce:

Open an SVG document with a polygon with 3 or more points and colored stroke. (I've attached one with a 10-point star.)


Actual results:

The polygon isn't closed - there is no stroke between the first and last points of the polygon.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Component: Untriaged → SVG
Product: Firefox → Core
Assignee: nobody → jwatt
Blocks: 1072339
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e31ac0e3997
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140929061957
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa1aac2d413
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140929063357
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e31ac0e3997&tochange=6fa1aac2d413
[Tracking Requested - why for this release]:
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Attached patch patch (obsolete) — Splinter Review
Attachment #8508744 - Flags: review?(longsonr)
Comment on attachment 8508744 [details] [diff] [review]
patch

I don't really like this. How about a common method that does the first bit i.e.

ConstructPath(whatever)
{
   const SVGPointList &points = mPoints.GetAnimValue();
 
   if (points.IsEmpty()) {

     return nullptr;
   }
 
   aBuilder->MoveTo(points[0]);
   for (uint32_t i = 1; i < points.Length(); ++i) {
    aBuilder->LineTo(points[i]);
   }
}

And then implement BuildPath in SVGPolygonElement with a close and in SVGPolylineElement without a close.
Attachment #8508744 - Flags: review?(longsonr) → review-
It would seem wrong to have a ConstructPath() method for polygon that doesn't do the close. I considered implementing BuildPath separately for both classes, but it seemed silly to have the code duplication. If this were a more complex part of the code I would, but since it's not this seemed best to me. It's also nice that it highlights the difference between the two types.
ConstructPath would be in the base nsPolyElement.cpp. Or I'd be fine just duplicating the code.
(In reply to Robert Longson from comment #6)
> ConstructPath would be in the base nsPolyElement.cpp.

Yes, I understood that. My point is that the "close" is an integral part of the path, so having a ConstructPath() method that doesn't insert the close would be bad IMO.
Attached patch patchSplinter Review
I guess that leaves duplicating the code.
Attachment #8508744 - Attachment is obsolete: true
Attachment #8508772 - Flags: review?(longsonr)
Attachment #8508772 - Flags: review?(longsonr) → review+
https://hg.mozilla.org/mozilla-central/rev/5b17709f660b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
jwatt: does this need uplifting to 35?  I noticed someone asking about this bug on Twitter just now. Thanks!
Flags: needinfo?(jwatt)
Comment on attachment 8508772 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1074294
[User impact if declined]: polygons rendered incorrecty (unclosed)
[Describe test coverage new/current, TBPL]: patch includes reftest
[Risks and why]: change is just to polygon/polyline code and it's pretty simple
[String/UUID change made/needed]: none
Flags: needinfo?(jwatt)
Attachment #8508772 - Flags: approval-mozilla-beta?
Attachment #8508772 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.