stroke-linecap:square doesn't show a line cap for zero-length path in SVG

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Darxus, Assigned: jwatt)

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 wanted)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: 

No blue square.

Reproducible: Always
(Reporter)

Updated

7 years ago
Blocks: 554013
Version: unspecified → Trunk

Comment 1

7 years ago
Does cairo's behavior for LINE_CAP_SQUARE just not match the one the SVG spec calls for here?
Status: UNCONFIRMED → NEW
Component: General → SVG
Ever confirmed: true
QA Contact: general → general
Summary: (ietestcenter) SVG 41/56: Test passes if there is a blue circle, a blue square, and no red on the page. → stroke-linecap:square doesn't show a line cap for zero-length path

Comment 2

7 years ago
No it doesn't. You have to invent an orientation for the square so it's a bit artificial.

Updated

7 years ago
Duplicate of this bug: 608751
(Assignee)

Comment 4

7 years ago
This causes us to fail:

http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/painting-control-04-f.html
status2.0: --- → wanted
(Assignee)

Updated

7 years ago
Blocks: 512501

Comment 5

7 years ago
I'm seeing this bug in 2d canvas with ctx.lineCap="square". Should I open a separate bug?

Comment 6

7 years ago
Perhaps you should try seeing whether cairo is willing to change?
(In reply to comment #6)
> Perhaps you should try seeing whether cairo is willing to change?

I'd like to see a justification for cairo having square caps for zero-length paths. Adobe Acrobat doesn't have square caps for zero-length paths which is where cairo's current behavior comes from.

Comment 8

7 years ago
Mikko, the canvas thing is almost certainly a duplicate of this bug.  As long as you're willing to track this bug and retest once it's fixed, probably no separate bug needed...
Summary: stroke-linecap:square doesn't show a line cap for zero-length path → stroke-linecap:square doesn't show a line cap for zero-length path in SVG and canvas
(Assignee)

Comment 9

7 years ago
(In reply to comment #7)
> I'd like to see a justification for cairo having square caps for zero-length
> paths. Adobe Acrobat doesn't have square caps for zero-length paths which is
> where cairo's current behavior comes from.

For consistency with cairo's /round/ caps behavior?

Alternatively, to allow a major cairo consumer (us) to conform to the specs for one of the open web languages we support?
(Assignee)

Comment 10

7 years ago
Created attachment 505290 [details] [diff] [review]
patch 1: preliminary patch to add explicit 'segStart'
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #505290 - Flags: review?(longsonr)
(Assignee)

Comment 11

7 years ago
Created attachment 505292 [details] [diff] [review]
patch 2: preliminary patch to stop hardcoding segment arg count
Attachment #505292 - Flags: review?(longsonr)
(Assignee)

Comment 12

7 years ago
Created attachment 505294 [details] [diff] [review]
patch 3: work around lack of cairo support

I'm not particularly in love with these hacks, but I don't think it's too bad.

Jeff, can you comment on the "XXX tinyAdvance should probably depend on the CTM" comment in this patch?
Attachment #505294 - Flags: review?(longsonr)
(Assignee)

Updated

7 years ago
OS: Linux → All
Hardware: x86_64 → x86

Comment 13

7 years ago
Comment on attachment 505292 [details] [diff] [review]
patch 2: preliminary patch to stop hardcoding segment arg count

Why not just increment i by SVGPathSegUtils::ArgCountForType(segType) at the end directly rather than having a separate argCount variable?
Attachment #505292 - Flags: review?(longsonr) → review+
(Assignee)

Comment 14

7 years ago
Because I need that argCount variable in the subsequent patch.
(Assignee)

Comment 15

7 years ago
Err, or I was in an earlier incarnation of the third patch. That's no longer the case in the current version, so I'll change it as you suggest.
Attachment #505294 - Flags: feedback?(jmuizelaar)

Comment 16

7 years ago
Looking at bug 584623, it seems like linecap round has issues too. Should patch 3 do linecap round too?

The issue with linecap round is that cairo doesn't think there is a bounding box. This lead to the fix in bug Bug 510177. This fix should be reverted if we can get a proper solution to the linecap round issue.
(Assignee)

Comment 17

7 years ago
I just commented on bug 584623 - it's WFM in beta 9. I can add a test for linecap round similar to the one included in patch 3 though.

Updated

7 years ago
Attachment #505290 - Flags: review?(longsonr) → review+

Comment 18

7 years ago
Comment on attachment 505294 [details] [diff] [review]
patch 3: work around lack of cairo support

>+static PRBool IsMoveto(PRUint16 aSegType)

Nit: I realise you just moved this but IsMoveTo would be nicer :-) If you look at the comments further on though you might
not need to move this at all.

>+  // XXX tinyAdvance should probably depend on the CTM

I doubt it, we made that mistake with radial gradients and then had to change the delta not to depend on the CTM.

>+  double tinyAdvance = 0.002;

static const double. Capitalised?

I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 = 0.00390625
which is the smallest difference cairo can detect isn't it.

>+#define MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS                     \
>+  do {                                                                        \
>+    if (capsAreSquare && !subpathHasLength && subpathContainsNonArc &&        \
>+        SVGPathSegUtils::IsValidType(prevSegType) &&                          \
>+        (!IsMoveto(prevSegType) ||                                            \
>+         segType == nsIDOMSVGPathSeg::PATHSEG_CLOSEPATH)) {                   \
>+      ApproximateZeroLengthSubpathSquareCaps(segStart, aCtx);                 \
>+    }                                                                         \
>+  } while(0)

What's the do ... while(0) for? I thought you only needed that for macros that might be defined as nothing sometimes.

Could this not be an inline function instead?

>+    if (!IsMoveto(segType) && segEnd != segStart ||
>+        SVGPathSegUtils::IsQuadraticType(segType) && segEnd != cp1 ||
>+        SVGPathSegUtils::IsCubicType(segType) && (segEnd != cp1 || segEnd != cp2)) {
>+      subpathHasLength = PR_TRUE;
>+    }

Since the code above is a big switch on segType, couldn't you set this in the appropriate switch options?

>+    if (!IsMoveto(segType) && !IsArc(segType)) {
>+      subpathContainsNonArc = PR_TRUE;

ditto.

>diff --git a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>--- a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>@@ -446,16 +446,22 @@ nsSVGPathGeometryFrame::Render(nsSVGRend
>   default:
>     gfx->SetAntialiasMode(gfxContext::MODE_COVERAGE);
>     break;
>   }
> 
>   /* save/restore the state so we don't screw up the xform */
>   gfx->Save();
> 
>+  // Hack to let SVGPathData::ConstructPath know if we have square caps:

Omit "Hack to".

>+  const nsStyleSVG* style = GetStyleSVG();
>+  if (style->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_SQUARE) {
>+    gfx->SetLineCap(gfxContext::LINE_CAP_SQUARE);
>+  }
>+

Why is this only in nsSVGPathGeometryFrame::Render? Doesn't it need to be in GeneratePath? What about GetFrameForPoint, UpdateCoveredRegion. You should add a mochitest for GetFrameForPoint or add to the existing GetFrameForPoint mochitest. You'd need some kind of dynamic reftest where you move shapes about to prove that UpdateCoveredRegion works and you're not leaving artifacts.
Attachment #505294 - Flags: review?(longsonr) → review-
(Assignee)

Comment 19

7 years ago
(In reply to comment #18)
> Comment on attachment 505294 [details] [diff] [review]
> >+static PRBool IsMoveto(PRUint16 aSegType)
> 
> Nit: I realise you just moved this but IsMoveTo would be nicer :-)

I deliberately didn't uppercase the "t" in "to" for consistency with the SVG spec.

> >+  // XXX tinyAdvance should probably depend on the CTM
> 
> I doubt it, we made that mistake with radial gradients and then had to change
> the delta not to depend on the CTM.

This case is pretty different, and we definitely depend on the CTM (zoom out and squares stop showing, zoom in and they still show). I think the patch coming up handles this correctly.

> >+  double tinyAdvance = 0.002;
> 
> static const double. Capitalised?

const, sure, but why static? Not sure about the capitalized either, since we don't tend to do that, do we(?) (and it is only a four line function).

> I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 =
> 0.00390625
> which is the smallest difference cairo can detect isn't it.

I'd guess it was working for me because 0.002 is just over half of 0.00390625, so it was being rounded up. This code has now changed though.

> What's the do ... while(0) for? I thought you only needed that for macros that
> might be defined as nothing sometimes.

So that I could add a semicolon after uses of the macro, which feels better.

> Could this not be an inline function instead?

It could, but it would take seven arguments so probably doesn't generate much less code and would be less readable.

> Since the code above is a big switch on segType, couldn't you set this in the
> appropriate switch options?

Yeah, okay. I was just trying not to clutter all the switch statements, but I guess that's what I should do.

> Omit "Hack to".

Hmm, I think it is a hack really, especially after it's moved to GeneratePath as you rightly point out it should be.
(Assignee)

Comment 20

7 years ago
Created attachment 506285 [details] [diff] [review]
patch 3: work around lack of cairo support
Attachment #505294 - Attachment is obsolete: true
Attachment #506285 - Flags: review?(longsonr)
Attachment #506285 - Flags: feedback?(jmuizelaar)
Attachment #505294 - Flags: feedback?(jmuizelaar)
(Assignee)

Updated

7 years ago
Attachment #506285 - Attachment is patch: true
Attachment #506285 - Attachment mime type: application/octet-stream → text/plain

Comment 21

7 years ago
> > >+  double tinyAdvance = 0.002;
> > 
> > static const double. Capitalised?
> 
> const, sure, but why static? Not sure about the capitalized either, since we
> don't tend to do that, do we(?) (and it is only a four line function).

static data is only initialised once. You get better performance.

Everything else seems fine.

Updated

7 years ago
Attachment #506285 - Flags: review?(longsonr) → review+
(Assignee)

Comment 22

7 years ago
Is that really true for |double|? I'd assumed that the bit pattern for the double value would just have been hardcoded into the binary implementation of the function generated by the compiler and no double initializer code would be invoked for it.

Anyway, now that the variable is a gfxSize instead of a double it can't be static:

https://developer.mozilla.org/index.php?title=en/C%2b%2b_Portability_Guide#Don%27t_use_static_constructors

Probably the only rational from that section that applies is the "may confuse leak tools" one which may not apply in this case, but we should probably follow the letter of the law anyway?
(Assignee)

Comment 23

7 years ago
Comment on attachment 506285 [details] [diff] [review]
patch 3: work around lack of cairo support

Requesting approval in advance of Jeff's feedback on the implementation of ApproximateZeroLengthSubpathSquareCaps. I think it's actually okay, but in the worst case we'll just fail to draw caps under certain transforms or something, and if there's a problem like that we can retrospectively fix it.
Attachment #506285 - Flags: approval2.0?
(Assignee)

Comment 24

7 years ago
Note the approval request is for all three patches. Maybe plusing needs to be marked on all three explicitly?
(Assignee)

Comment 25

7 years ago
Created attachment 506407 [details] [diff] [review]
patch 3: work around lack of cairo support

Of course we can't set subpathHasLength blindly or else that variable should really be named "lastSegHasLength", which is not what we want.
Attachment #506285 - Attachment is obsolete: true
Attachment #506407 - Flags: feedback?(jmuizelaar)
Attachment #506407 - Flags: approval2.0?
Attachment #506285 - Flags: feedback?(jmuizelaar)
Attachment #506285 - Flags: approval2.0?
(Assignee)

Comment 26

7 years ago
Modulo reftest failures due to random orange and the problem addressed in comment 25, the patches pass try server.
Comment on attachment 506407 [details] [diff] [review]
patch 3: work around lack of cairo support

Let's take this after FF4.
Attachment #506407 - Flags: approval2.0? → approval2.0-
Whiteboard: not-ready
(Assignee)

Comment 28

6 years ago
Created attachment 527021 [details] [diff] [review]
patch 3: work around lack of cairo support

unbitrot
Attachment #506407 - Attachment is obsolete: true
Attachment #527021 - Flags: feedback?(jmuizelaar)
Attachment #506407 - Flags: feedback?(jmuizelaar)
(Assignee)

Comment 29

6 years ago
http://hg.mozilla.org/mozilla-central/rev/cbaa924ffeca
http://hg.mozilla.org/mozilla-central/rev/ccd5d505ab00
http://hg.mozilla.org/mozilla-central/rev/5b78983c4822
Whiteboard: not-ready
(Assignee)

Updated

6 years ago
Attachment #527021 - Flags: feedback?(jmuizelaar)
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 657862
Depends on: 788862
(Assignee)

Comment 30

4 years ago
longsonr: do you remember why arc was excluded in this patch? (That is, why subpathContainsNonArc exists.)
Summary: stroke-linecap:square doesn't show a line cap for zero-length path in SVG and canvas → stroke-linecap:square doesn't show a line cap for zero-length path in SVG
You need to log in before you can comment on or make changes to this bug.