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

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wnickl, Assigned: longsonr)

Tracking

({regression, testcase})

18 Branch
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 affected, firefox19+ verified, firefox20+ fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Posted file svg_text_in_ff18.zip (obsolete) —
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Component: Untriaged → SVG
Product: Firefox → Core
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Flags: needinfo?(wnickl)
(Reporter)

Comment 3

6 years ago
(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.
Flags: needinfo?(wnickl)
(Reporter)

Comment 4

6 years ago
This file replaces svg_text_in_ff18.zip
(Assignee)

Comment 5

6 years ago
Please attach individual files rather than zipping things up.

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

Updated

6 years ago
Flags: needinfo?(wnickl)
(Assignee)

Comment 6

6 years ago
Attachment #699700 - Attachment is obsolete: true
Flags: needinfo?(wnickl)
(Assignee)

Comment 7

6 years ago
Posted image 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.
Attachment #700914 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

6 years ago
Summary: SVG text element style is destroyed → stroke-width="0" is ignored.
(Assignee)

Comment 8

6 years ago
Posted patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #701134 - Flags: review?(dholbert)
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Blocks: 719286
(Assignee)

Updated

6 years ago
(Assignee)

Updated

6 years ago
Keywords: regression, testcase
(Assignee)

Comment 9

6 years ago
I don't need a regression range any more.
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.
(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 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
Attachment #701134 - Flags: review?(dholbert) → review+
(Assignee)

Comment 13

6 years ago
Lets see how we get on without it then

https://tbpl.mozilla.org/?tree=Try&rev=3037fb074c84
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.)
Summary: stroke-width="0" is ignored. → stroke-width="0" is ignored on SVG text
https://hg.mozilla.org/mozilla-central/rev/8b6711fcba29
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(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.
(Assignee)

Comment 18

6 years ago
This landed with that line removed and no reftests failed.
(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?
(Assignee)

Comment 20

6 years ago
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
Attachment #701134 - Flags: approval-mozilla-beta?
Attachment #701134 - Flags: approval-mozilla-aurora?
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.
Attachment #701134 - Flags: approval-mozilla-beta?
Attachment #701134 - Flags: approval-mozilla-beta+
Attachment #701134 - Flags: approval-mozilla-aurora?
Attachment #701134 - Flags: approval-mozilla-aurora+
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)
(Assignee)

Comment 23

6 years ago
Isn't SVG glyphs in opentype preffed off. In which case it's not likely to break much even if it's wrong.
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.

Updated

6 years ago
Duplicate of this bug: 832358
Verified fixed FF 19b4 Win 7, Ubuntu 12.04 and Mac OS X 10.7.5
You need to log in before you can comment on or make changes to this bug.