939942 - Bug: SVG Path is not rendered properly when stroke attribute is changed from "none" to a color through Javascript. Regression in version 25.0.1

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Microsoft Office Web Apps]

Attachment: SVGStrokeBugFF.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025151332

Steps to reproduce:

See attached file:
   1. An SVG viewport is setup with a path object. Stroke is set to "none".
   2. User clicks a button, triggering a javascript function that sets the stroke to "black".


Notes:
1. This example works properly in Firefox 22-24: this is a recent regression in version 25.
2. If the stroke starts as something visible (i.e. "green") it will properly change to "black".
3. The Firefox Web Inspector shows that the path's stroke has updated to "black", but does not show a dotted line around where the path should be.





Actual results:

Nothing happens


Expected results:

Path appears, outlined in black

Comment 1

[Marco Perez [:mpk]]

Attachment: test.svg

Confirming for current trunk builds (tested on FreeBSD 9-stable).
There are 3 clickable circles in this testcase:

 - red)     this doesn't seem to work         (other border, initially invisible)
 - yellow)  works but border looks "ugly"     (same  border, initially invisible) 
 - green)   this seems to work as expected    (same  border, initially   visible)

The yellow circle behaves very strange (try e.g. multiple triple-clicks).

Comment 2

[Marco Perez [:mpk]]

Attachment: screenshot

The yellow or green circles may have an irregular border. If you can't reproduce it at once try again after resizing your browser window.

Comment 3

[Daniel Holbert [:dholbert]]

Regression range (for both testcases):
{
Last good nightly: 2013-05-25
First bad nightly: 2013-05-26

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a2f7a45819a&tochange=0fed3377c839
}

Comment 4

[Daniel Holbert [:dholbert]]

Looks like a regression from bug 854765, bug 875037, or bug 875175.

I'm leaning towards bug 875175, since that's invalidation-related, and the second testcase here (w/ circles) is pretty clearly an invalidation bug. (Its glitches fix themselves if I force a repaint by e.g. switching tabs)

Comment 5

[Jonathan Watt [:jwatt]]

Drive-by comment, without having looked at the bugs in the regression range: probably what's happening here is that the overflow rects are empty if there is no stroke or fill. Presumably one of the bugs in range was one of the changes to stop reflowing for certain DOM changes, and now we rely just on a repaint. That's not good enough in the case that we change from no stroke to having stroke; we need to update overflow rects as well as repaint.

Comment 6

[Robert Longson [:longsonr]]

Attachment: patch

Comment 7

[Jonathan Watt [:jwatt]]

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

Thanks for looking at this. The patch needs some work still though.

The main thing is that the code that decides whether to update the bounds needs to be (and stay) in sync with nsSVGUtils::HasStroke. It's not just none to non-none (and vice versa) changes that can cause this.

Please add comments to your new function and to nsSVGUtils::HasStroke noting that changes to one need to be kept in sync with the other.

Please also add similar tests that check stroke-opacity going from 0 to non-zero, and stroke-width going from 0 to non-zero.

Do we need to use nsChangeHint_NeedDirtyReflow in this case?

Comment 8

[Robert Longson [:longsonr]]

> Do we need to use nsChangeHint_NeedDirtyReflow in this case?

No idea, I'm leaving that to the bug filed against that.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 10

[Jonathan Watt [:jwatt]]

We'll want this patch on branches, so please move the unnecessary NS_CombineHint changes to a separate patch.

I'm not really keen on having the logic that corresponds to nsSVGUtils::HasStroke scattered around. How about adding this inline static member to nsStyleSVG:

  /**
   * Returns true if the stroke is not "none" and the stroke-opacity is greater
   * than zero. This ignores mStrokeWidthFromObject since invalidation for
   * that is handled elsewhere.
   */
  static bool HasStroke(const nsStyleSVG* aStyle)
  {
    return aStyle->mStroke.mType != eStyleSVGPaintType_None &&
           aStyle->mStrokeOpacity > 0;
  }

Then add an appropriate check in nsStyleSVG::CalcDifference along the lines of:

  if (HasStroke(this) != HasStroke(aOther)) {
    // comment
    reflow hints here
  }

Also you can change nsSVGUtils::HasStroke to use nsStyleSVG::HasStroke, reducing the risk that these parts of the code will get out of sync, and eliminating the need for comments about keeping them in sync.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8335381 [details] [diff] [review]
address review comments

I'll cancel review for now until previous comment is addressed.

Comment 12

[Robert Longson [:longsonr]]

Attachment: more review comments

Comment 13

[Glen van de Mosselaer]

Please note:

1) Changing the zoom level (ctrl+, ctrl-) temporarily fixes the rendering issue (On Win7 x64 at least).

2) I've also found that in v25.0.1, paths with no stroke and no fill (to be altered later via javascript) are drawn offscreen or not at all (not sure), and will not appear when altered. Changing the zoom level corrects this too.

Comment 14

[Glen van de Mosselaer]

Attachment: test-workaround.svg

As a temporary workaround, using fill="transparent" and stroke="transparent" seems to resolve the issue somewhat.

In my software, I still see path fill clipping issues using this workaround - but paths with both fill and stroke set to none are much better behaved and appear on-screen.

Comment 15

[Robert Longson [:longsonr]]

(In reply to Glen van de Mosselaer from comment #13)
>
> 2) I've also found that in v25.0.1, paths with no stroke and no fill (to be
> altered later via javascript) are drawn offscreen or not at all (not sure),
> and will not appear when altered. Changing the zoom level corrects this too.

The patch covers that case too.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8336355 [details] [diff] [review]
more review comments

As discussed on IRC, let's do something more along the lines of:

if (HasStroke(this) != HasStroke(&aOther) ||
    (!HasStroke(this) && HasFill(this) != HasFill(&aOther))) {
  // Frame bounds and overflow rects depend on whether we "have" fill or
  // stroke. Whether we have stroke or not just changed, or else we have no
  // stroke (in which case whether we have fill or not is significant to frame
  // bounds) and whether we have fill or not just changed. In either case we
  // need to reflow so the frame rect is updated.
  // XXXperf this is a waste on non nsSVGPathGeometryFrames.
  ...reflow hints
}

That keeps everything related to this "frame bounds depend of whether we 'have' stroke/fill" together, which I think is important for code readability.

Comment 17

[Robert Longson [:longsonr]]

Attachment: updated

Comment 18

[Robert Longson [:longsonr]]

Attachment: updated

Need to reflow if stroke-opacity changes from <=0 to > 0

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 8336907 [details] [diff] [review]
updated

Review of attachment 8336907 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good, but I'll like to have a look at the patch addressing the comments below.

::: layout/style/nsStyleStruct.h
@@ +2392,5 @@
> +   * than zero. This ignores stroke-widths as that depends on the context.
> +   */
> +  bool HasStroke() const {
> +    return mStroke.mType != eStyleSVGPaintType_None && mStrokeOpacity > 0;
> +  }

Please put a blank line between the start of the comments for these two methods  the preceding curly brace while you're here.

@@ +2398,5 @@
> +   * Returns true if the fill is not "none" and the fill-opacity is greater
> +   * than zero.
> +   */
> +  bool HasFill() const {
> +    return mFill.mType != eStyleSVGPaintType_None && mFillOpacity > 0;

This change in not compatible with what nsSVGPathGeometryFrame::GetBBoxContribution does (which is what nsSVGPathGeometryFrame::ReflowSVG uses to calculate its frame rect):

http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGPathGeometryFrame.cpp?mark=432-432,439-439#428

It only takes mFill.mType into account, not mFillOpacity. I'd be very happy to change that in a follow-up patch, but for now you should maintain compatibility with what the existing code does, especially as you're changing nsSVGTextFrame2 and nsSVGUtils to use this.

::: layout/svg/nsSVGUtils.cpp
@@ -1246,5 @@
>      return false;
>    }
> -  if (style->mFill.mType == eStyleSVGPaintType_None ||
> -      style->mFillOpacity <= 0 ||
> -      !HasStroke(aFrame)) {

Given the change requested to HasFill, this won't be the same logic. I know this looks really safe, but we still have some weird dependencies on behavior and I'd rather not take the risk on branches. Happy to take this as a follow-up patch for Nightly.

@@ +1496,5 @@
>  nsSVGUtils::SetupCairoStrokePaint(nsIFrame *aFrame, gfxContext* aContext,
>                                    gfxTextContextPaint *aContextPaint)
>  {
>    const nsStyleSVG* style = aFrame->StyleSVG();
> +  if (!style->HasStroke())

Ditto. This would now take account of stroke-opacity whereas previously it did not.

Comment 20

[Robert Longson [:longsonr]]

Attachment: Removed all the bits you objected to

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 8337050 [details] [diff] [review]
Removed all the  bits you objected to

Review of attachment 8337050 [details] [diff] [review]:
-----------------------------------------------------------------

I only objected to two sections in nsSVGUtils.cpp, but you've removed pretty much all your changes to nsSVGTextFrame2.cpp and nsSVGUtils.cpp, and not changed HasFill to remove the mFillOpacity check (meaning that the nsStyleStruct change isn't in sync with nsSVGPathGeometryFrame::ReflowSVG/GetBBoxContribution as explained in comment 19). Anyway, I guess this version of the patch is now acceptable since all it means is that we'll reflow for fill opacity changes that cause the "has fill" state to change when we don't have any stroke. In other words we may occasionally do more work than we need to, but I doubt it matters much (and if we take fill opacity into account for frame bounds in future we'd need to do this anyway).

Comment 22

[Jonathan Watt [:jwatt]]

More importantly this patch no longer changes behavior other than the behavior that _needs_ to change to fix the bug, which is important to keep risk to a minimum for a patch that we want to uplift.

Thanks, Robert.

Comment 23

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd5d4cb820e

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9bd5d4cb820e

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8337050 [details] [diff] [review]
Removed all the  bits you objected to

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 942783
User impact if declined: broken SVG
Testing completed (on m-c, etc.): landed on m-i and merged to m-c, with tests
Risk to taking this patch (and alternatives if risky): very low - simply invalidates more frequently (when needed)
String or IDL/UUID changes made by this patch: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/938330ba1786
https://hg.mozilla.org/releases/mozilla-beta/rev/11d2cadb72f8

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/11d2cadb72f8

Comment 29

[Petruta Horea [:phorea]]

Tested using the testcases from attachments. Considering that the issue is also covered by automated tests, I'm setting these Firefox versions to verified.

Verified as fixed using Fx 26 beta 8 (20131125215016), latest Aurora (20131126004001), latest Nightly (20131126030201) on Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OS 10.8.5.

Comment 30

[Petruta Horea [:phorea]]

Removing the keyword as per comment 29.