Closed Bug 939942 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- verified
firefox27 --- verified
firefox28 --- verified
b2g-v1.2 --- fixed

People

(Reporter: MSAOfficeWebApps, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(5 files, 5 obsolete files)

Attached file 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
Attached image 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).
Component: Untriaged → SVG
Keywords: regression, testcase
OS: Windows 8.1 → All
Product: Firefox → Core
Hardware: x86_64 → All
Version: 25 Branch → Trunk
Attached image 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
}
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)
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.
Assignee: nobody → longsonr
Attached patch patch (obsolete) — Splinter Review
Attachment #8335248 - Flags: review?(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?
Attachment #8335248 - Flags: review?(jwatt) → review-
> Do we need to use nsChangeHint_NeedDirtyReflow in this case?

No idea, I'm leaving that to the bug filed against that.
Attached patch address review comments (obsolete) — Splinter Review
Attachment #8335248 - Attachment is obsolete: true
Attachment #8335381 - Flags: review?(jwatt)
Attachment #8335381 - Attachment is patch: true
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 on attachment 8335381 [details] [diff] [review]
address review comments

I'll cancel review for now until previous comment is addressed.
Attachment #8335381 - Flags: review?(jwatt)
Attached patch more review comments (obsolete) — Splinter Review
Attachment #8335381 - Attachment is obsolete: true
Attachment #8336355 - Flags: review?(jwatt)
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.
Attached image 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.
Attachment #8336433 - Attachment description: Workaround by using fill=transparent instead of fill=none → test-workaround.svg
(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 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.
Attachment #8336355 - Flags: review?(jwatt) → review-
Attached patch updated (obsolete) — Splinter Review
Attachment #8336355 - Attachment is obsolete: true
Attachment #8336895 - Flags: review?(jwatt)
Attached patch updated (obsolete) — Splinter Review
Need to reflow if stroke-opacity changes from <=0 to > 0
Attachment #8336895 - Attachment is obsolete: true
Attachment #8336895 - Flags: review?(jwatt)
Attachment #8336907 - Flags: review?(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.
Attachment #8336907 - Flags: review?(jwatt)
Attachment #8336907 - Attachment is obsolete: true
Attachment #8337050 - Flags: review?(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).
Attachment #8337050 - Flags: review?(jwatt) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/9bd5d4cb820e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 942783
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
Attachment #8337050 - Flags: approval-mozilla-beta?
Attachment #8337050 - Flags: approval-mozilla-aurora?
Attachment #8337050 - Flags: approval-mozilla-beta?
Attachment #8337050 - Flags: approval-mozilla-beta+
Attachment #8337050 - Flags: approval-mozilla-aurora?
Attachment #8337050 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: needs landing on beta/aurora using hg qimport
Keywords: verifyme
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.
Blocks: 935404
Removing the keyword as per comment 29.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.