828246 - stroke-width="0" is ignored on SVG text

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[wwn]

Attachment: svg_text_in_ff18.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

Since Firefox 18 SVG some text elements are displayed completely different to FF17 and other browsers.





Expected results:

The rendering / formatting should be the same like in previous versions.

svg_text_in_ff18.zip contains screenshots for FF18 and Chrome (displays well...displays like FF17) and the svg file.

Comment 1

[Robert Longson [:longsonr]]

I don't think the testcase displays the same thing as the screenshots. Could you upload all the documents as individual separate attachments please and make sure that the svg file corresponds to the screenshots.

Comment 2

[Robert Longson [:longsonr]]

Also if this is a regression then finding a regression range would help a lot if you're willing to do that.

https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/

type about:buildconfig in the address bar in the last good and subsequent first bad build and post the Built from link please.

Comment 3

[wwn]

(In reply to Robert Longson from comment #1)
> I don't think the testcase displays the same thing as the screenshots. Could
> you upload all the documents as individual separate attachments please and
> make sure that the svg file corresponds to the screenshots.

Hello Robert,

The svg is a small part of it all. It's one of the shapes with a name inside. I thought it's enough code. I'll add the complete svg tomorrow here. 

I'm currently not in the office but for demonstrating how about screen sharing via Skype? so I could login to our intranet and show you the bug.

best wishes
W.

Comment 4

[wwn]

Attachment: complete SVG and screenshots FF18 and Chrome

This file replaces svg_text_in_ff18.zip

Comment 5

[Robert Longson [:longsonr]]

Please attach individual files rather than zipping things up.

Also the svg file is invalid as it has undefined namespaces.

Comment 6

[Robert Longson [:longsonr]]

Attachment: reporters testcase fixed to be valid SVG

Comment 7

[Robert Longson [:longsonr]]

Attachment: reduced testcase

It seems to come down to stroke-width being ignored if it is exactly zero. In this case the stroke is drawn when it shouldn't be. The line below has a very tiny stroke width for comparison.

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

Comment 9

[Robert Longson [:longsonr]]

I don't need a regression range any more.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 701134 [details] [diff] [review]
patch

>diff --git a/layout/reftests/svg/reftest.list b/layout/reftests/svg/reftest.list
>--- a/layout/reftests/svg/reftest.list
>+++ b/layout/reftests/svg/reftest.list
>@@ -287,16 +287,17 @@ fuzzy-if(cocoaWidget&&layersGPUAccelerat
> == text-style-01e.svg text-style-01-ref.svg
> == thin-stroke-01.svg pass.svg
>+== zero-stroke-01.svg pass.svg
> == tspan-dxdy-01.svg tspan-dxdy-ref.svg

Nit: This leaves this chunk of reftest.list out of alphabetical order... not a big deal though.  It's OK by me, if you think it makes more sense with this reftest up there.

>diff --git a/layout/svg/nsSVGGlyphFrame.cpp b/layout/svg/nsSVGGlyphFrame.cpp
> bool
> nsSVGGlyphFrame::SetupCairoStroke(gfxContext *aContext,
>                                   gfxTextObjectPaint *aOuterObjectPaint,
>                                   SVGTextObjectPaint *aThisObjectPaint)
> {
>-  const nsStyleSVG *style = GetStyleSVG();
>-  if (style->mStroke.mType == eStyleSVGPaintType_None) {
>+  if (!nsSVGUtils::HasStroke(this, aOuterObjectPaint)) {
>     aThisObjectPaint->SetStrokeOpacity(0.0f);
>     return false;

Is the (preexisting) SetStrokeOpacity() line actually useful?  It seems like we should guard any actual stroke-painting with a HasStroke call, and we shouldn't need to make it transparent on top of that.

>+++ b/layout/svg/nsSVGPathGeometryFrame.cpp
>-  if (nsSVGUtils::HasStroke(this, objectPaint)) {
>-    nsSVGUtils::SetupCairoStrokeHitGeometry(this, gfx, objectPaint);
>-    if (nsSVGUtils::SetupCairoStrokePaint(this, gfx, objectPaint)) {
>-      gfx->Stroke();
>-    }
>+  if (nsSVGUtils::SetupCairoStroke(this, gfx, objectPaint)) {
>+    gfx->Stroke();
>   }

What is this change doing? It doesn't seem to change our behavior on the included reftest.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> What is this change doing? It doesn't seem to change our behavior on the
> included reftest.

[/me glances at the SetupCairoStroke impl]

oh, never mind -- looks like just code-simplification, no functional change.  Cool.

So the only thing I'm wondering about is whether we need that SetStrokeOpacity().

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 701134 [details] [diff] [review]
patch

So: r=me with the SetStrokeOpacity line removed (if it's unneeded) or commented to explain why we need it

Comment 13

[Robert Longson [:longsonr]]

Lets see how we get on without it then

https://tbpl.mozilla.org/?tree=Try&rev=3037fb074c84

Comment 14

[Daniel Holbert [:dholbert]]

One more nit: the commit message ("Fix stroke-width = 0") could use a bit of finessing. :) Perhaps "Prevent stroke from drawing, on SVG text with a stroke-width of 0"?

Other than that, the Try run is looking good, so I think this is OK to land!

It makes sense that the SetStrokeOpacity() call wouldn't make a difference, too, because one level up the stack, we have:
>  951   if (SetupCairoStroke(aContext, aOuterObjectPaint, thisObjectPaint)) {
>  952     toDraw = DrawMode(toDraw | gfxFont::GLYPH_STROKE);
>  953   }
https://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGGlyphFrame.cpp#951

Assuming that |toDraw| behaves as I'd expect it to, it looks like we'll skip stroke-drawing if long as SetupCairoStroke() returns false -- so the stroke opacity shouldn't matter in that case.

(Edwin: please comment if you know of a reason we should keep that SetStrokeOpacity() line.)

Comment 15

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6711fcba29

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b6711fcba29

Comment 17

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> (Edwin: please comment if you know of a reason we should keep that
> SetStrokeOpacity() line.)

SVG glyphs are able to inherit stroke and fill paints and opacities from the outer text object. |aThisObjectPaint| is the object carrying this style information into gfxFont.

Since paints and opacities can be mixed and matched (i.e. an author can say "fill: context-fill; fill-opacity: context-stroke-opacity", say), I figured context-stroke-opacity should be 0 when the stroke-width of the context text element is 0.

There should have been a reftest that covered this.

Comment 18

[Robert Longson [:longsonr]]

This landed with that line removed and no reftests failed.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #17)
> I figured
> context-stroke-opacity should be 0 when the stroke-width of the context text
> element is 0.

(nit: presumably you mean "...when the 'stroke' of the context text element is 'none'" -- since that's what this code originally checked.  longsonr only just recently added the check for stroke-width=0 check, to fix this bug here)

Anyway -- I'm not sure I agree with that dependency.  Note that "stroke:none" and "stroke-width:0" are just two of many things that would prevent stroke from being painted on the context text element. Here are some others, off the top of my head:
  stroke-width: <arbitrarily-small-value>;
  stroke: <transparent paint-server>;
  stroke-dasharray: 0 1;
Assuming that *those* things don't affect the "context-stroke-opacity" behavior, I don't see why we should have special behavior for "stroke:none" and "stroke-width:0".

If I'm misunderstanding, or if you disagree, could you perhaps file a bug with a testcase that demonstrates the issue?

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 701134 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 719286
User impact if declined: some SVG text displayed stroked when they should not be
Testing completed (on m-c, etc.): landed with test
Risk to taking this patch (and alternatives if risky): pretty low as the only other thing that can be affected is SVG glyphs in truetype and they are still disabled behind a pref
String or UUID changes made by this patch: none

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 701134 [details] [diff] [review]
patch

Approving for all branches now, to give this as much bake time as possible with the Aurora/Beta audiences.

Comment 22

[Daniel Holbert [:dholbert]]

Edwin: I'm intending to land this patch on aurora & beta branches (including the SetStrokeOpacity() removal).

Before I do that -- do you have any more thoughts on the dropped SetStrokeOpacity() line?  From your last comment, it sounded like you're thinking that removing it would break stuff -- do you still think that?

If it's likely to break stuff, then we could easily backport a tweaked version of this bug's patch that preserves the SetStrokeOpacity() line.  However, if you don't object, I'd prefer to backport the patch in its entirety, for consistency. (and also because, to the extent that the SetStrokeOpacity call has any effect at all, I think it introduces inconsistency & is undesirable, per comment 19)

Comment 23

[Robert Longson [:longsonr]]

Isn't SVG glyphs in opentype preffed off. In which case it's not likely to break much even if it's wrong.

Comment 24

[Daniel Holbert [:dholbert]]

Ah, you're right:

> 251 pref("gfx.font_rendering.opentype_svg.enabled", false);
https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#251

I'll go ahead & land on branches, then.

Comment 25

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a34152bf0975
https://hg.mozilla.org/releases/mozilla-beta/rev/fa6c6d7413a2

Comment 27

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 19b4 Win 7, Ubuntu 12.04 and Mac OS X 10.7.5