1117514 - Lines with zero length dashes not drawn

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jan Tošovský]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141229214612

Steps to reproduce:

Open http://drifted.in/other/dash_array.svg


Actual results:

There are missing dots in the image. 


Expected results:

When round linecap is set, there IMO should be dots visible even for zero length dashes. It is a regression. It worked in previous versions - at least in 2013 when I added my comment to the similar Chrome issue: https://code.google.com/p/chromium/issues/detail?id=162424

Comment 1

[Robert Longson [:longsonr]]

I see a line on Firefox 34 but not on trunk. A regression range would help. If you want to assist with that Jan, then there's a tool to assist with that http://mozilla.github.io/mozregression/

Comment 2

[Robert Longson [:longsonr]]

Could be bug 1075399 since that landed for Firefox 35.

Comment 3

[Robert Longson [:longsonr]]

Attachment: round.txt

Comment 4

[Robert Longson [:longsonr]]

Attachment: round.txt

Comment 5

[Ada [:adalucinet]]

Regression range:

--- (m-c):
Last good revision: 7c24470b6b3a (2014-09-30)
First bad revision: 14665b1de5ee (2014-10-01)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c24470b6b3a&tochange=14665b1de5ee 

---(m-i):
Last good revision: 3b427c2ee147
First bad revision: 0ade54570f25
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b427c2ee147&tochange=0ade54570f25 

Also reproducible on Nightly 37.0a1 (Build ID: 20150104030205) - Mac OS X 10.9.5. and Ubuntu 14.04 32bit

Comment 6

[Robert Longson [:longsonr]]

Thanks for the range Alexandra. The regression is from https://hg.mozilla.org/integration/mozilla-inbound/rev/cd87b4cb26c1

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8543708 [details] [diff] [review]
round.txt

>+  // If the sum of the values is zero,
>+  // then the stroke is rendered as if a value of none were specified

In its current form I'm not sure this comment adds anything since it seems to just say what the code plainly does.

>+  if (aStyleSVG->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_BUTT &&
>+      (totalLengthOfDashes <= 0 || totalLengthOfGaps <= 0)) {
>+    return (totalLengthOfGaps > 0) ? eNoStroke : eContinuousStroke;
>   }

This isn't quite correct. In the case of |totalLengthOfGaps <= 0| we want to return eContinuousStroke regardless of the value of stroke-linecap (or else we'll regress bug 609361). It's only the case where |totalLengthOfDashes <= 0| that we want to check the caps. Maybe change the code to:

  // Stroking using dashes is much slower than stroking a continuous line
  // (see bug 609361 comment 40), and much, much slower than not stroking the
  // line at all. Here we check for cases when the dash pattern causes the
  // stroke to essentially be continuous or to be nonexistent in which case
  // we can avoid expensive stroking operations (the underlying platform
  // graphics libraries don't seem to optimize for this).
  if (totalLengthOfDashes <= 0 && totalLengthOfGaps <= 0) {
    return eNoStroke;
  }
  if (totalLengthOfGaps <= 0) {
    return eContinuousStroke;
  }
  if (totalLengthOfDashes <=0) {
    // We can only return eNoStroke if the value of stroke-linecap isn't
    // adding caps to zero length dashes.
    if (aStyleSVG->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_BUTT) {
      return eNoStroke;
    }
  }

I actually wonder if your test passes for all platforms though. It wouldn't surprise me if the underlying platforms have different behavior. (If so, I'd also be happy with the above code with the NS_STYLE_STROKE_LINECAP_BUTT check dropped.)

Comment 8

[Robert Longson [:longsonr]]

Attachment: addresss review comments

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8546518 [details] [diff] [review]
addresss review comments

r+, thanks!

FWIW I personally prefer:

  if (totalLengthOfDashes <=0 ) {
    // We can only return eNoStroke if the value of stroke-linecap isn't
    // adding caps to zero length dashes.
    if (aStyleSVG->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_BUTT) {
      return eNoStroke;
    }
  }

Since the compiler should do the same thing and it feels a bit clearer to me, but not a big deal by any means.

Comment 10

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ccc90058eb

Comment 11

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29229def6b2e

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29229def6b2e