Closed Bug 875069 Opened 6 years ago Closed 6 years ago

nsSVGPathGeometryFrame::NotifySVGChanged needs to take account of stroke-width and vector-effect

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

nsSVGPathGeometryFrame::NotifySVGChanged needs to take account of stroke-width, since it too can have a percentage value. In that case we need to reflow if the coordinate context changes in order to update the frame's mRect (since it takes account of stroke).

Nobody appears to have noticed yet, but this is a regression from bug 869611. I guess percentage values for stroke-width are pretty rare since having them resolve against the viewport width isn't very useful.
Attached patch patch (obsolete) — Splinter Review
Attachment #752958 - Flags: review?(dholbert)
Do we need to reflow for a non-scaling-stroke, if the viewport gets smaller?
Even if the stroke width is not a percentage.
In the case of non-percentage stroke-width we do need some extra logic for non-scaling-stroke, but for transform changes rather than viewport size changes. Good catch.
Attached patch patchSplinter Review
Attachment #752958 - Attachment is obsolete: true
Attachment #752958 - Flags: review?(dholbert)
Attachment #753091 - Flags: review?(dholbert)
Comment on attachment 753091 [details] [diff] [review]
patch

> == stroke-width-percentage-01.svg pass.svg
>+== stroke-width-percentage-02.svg stroke-width-percentage-02-03-ref.svg
>+== stroke-width-percentage-03.svg stroke-width-percentage-02-03-ref.svg

file-naming nit: so, I've never seen this "02-03-ref" convention before... I actually thought it was a typo at first, and it doesn't seem particularly scalable (e.g. if you add a new test -04 which also matches this reference, then you're left with stroke-width-percentage-02-03-04-ref.svg).

I think the more common (and my preferred) convention for testcases sharing a reference is to name them like so:
  stroke-width-percentage-02a.svg
  stroke-width-percentage-02b.svg
  stroke-width-percentage-02-ref.svg

Would you mind renaming the reftest files along those lines?

[in a meeting, so I haven't done a full review yet, but that's just the first thing I noticed]
Comment on attachment 753091 [details] [diff] [review]
patch

>diff --git a/layout/reftests/svg/non-scaling-stroke-03-ref.svg b/layout/reftests/svg/non-scaling-stroke-03-ref.svg
>+<svg xmlns="http://www.w3.org/2000/svg" class="reftest-wait">
>+  <line x1="100" y1="100" x2="100" y2="200" stroke="blue" stroke-width="10"/>
>+</svg>

This file (the reference case, w/ no scripting) doesn't want "reftest-wait".

>diff --git a/layout/reftests/svg/stroke-width-percentage-02.svg b/layout/reftests/svg/stroke-width-percentage-02.svg
>+    <!-- Don't give the lines x/y attributes percentages since that
>+         may trigger reflow in which case this test wouldn't be
>+         testing what we intend!
>+    -->
>+    <line x1="50" x2="50" y2="50" stroke="blue" stroke-width="100%"/>

s/the lines/the line's/

Also: stroke-width-percentage-02.svg and stroke-width-percentage-03.svg both pass in my unpatched build right now, so I'm not sure they're actually testing what you wanted them to be testing.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Also: stroke-width-percentage-02.svg and stroke-width-percentage-03.svg both
> pass in my unpatched build right now, so I'm not sure they're actually
> testing what you wanted them to be testing.

Hmm, looks like it only fails when the viewport change is because of a window resize. I guess that's because in nsSVGOuterSVGFrame/nsSVGInnerSVGFrame::AttributeChanged we call nsSVGUtils::ScheduleReflowSVG, and since that currently uses NS_FRAME_IS_DIRTY, we end up reflowing the entire SVG tree which hides this issue.

Not sure offhand how to do reftests with a window resize, but I'll take a look tomorrow.
(In reply to Jonathan Watt [:jwatt] from comment #8)
> Not sure offhand how to do reftests with a window resize, but I'll take a
> look tomorrow.

You should be able to use a shim html file with <iframe src="myStuff.svg">, and then resize the iframe.

Here's an example, in a flexbox reftest:
The testcase (from the perspective of reftest.list):
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/flexbox/flexbox-dyn-changeFrameWidth-1.xhtml
The helper-file:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/flexbox/flexbox-dyn-changeFrameWidth-1-iframe.html
The reference:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/flexbox/flexbox-dyn-changeFrameWidth-1-ref.xhtml
Comment on attachment 753091 [details] [diff] [review]
patch


>diff --git a/layout/reftests/svg/stroke-width-percentage-02.svg b/layout/reftests/svg/stroke-width-percentage-02.svg
>+<svg xmlns="http://www.w3.org/2000/svg" class="reftest-wait">
>+  <title>Testcase percentage stroke-width repaints after viewport size change</title>

One last test nit: I think you want s/Testcase/Test/ there. (Same goes for the -03 version of this test.)
Comment on attachment 753091 [details] [diff] [review]
patch

OK, now looking at the actual code. :)  The code-changes make sense.

However -- with the current patch, the commenting in this function seems (to me at least) to have gotten a bit disorganized. (Particularly the pre-existing large comment at the top, in the new context.  That comment describes one specific case that we have to check for, and then it calls out a general class of things that we *don't* have to handle, and then there are another few blocks of comments before we finally get to the specific case that the initial comment described.)

Maybe restructure the commenting here to look something like this:
  // Changes to our ancestor will require us to reflow under the
  // following conditions:
  //  (a) If our coordinate context changed, and either our geometry or
  //      our stroke-width are dependent on that, then our frame needs
  //      to be resized. (because stroke size affects frame size)
  //  (b) If the transform changed, and we've got non-scaling stroke,
  //      then our frame needs to be resized. (for the same reason)

  [ the code ]

  // (comment about how we don't need to notify rendering observers)

or something like that.  That'd make this function a lot more readable, IMHO.

r=me with the above comments addressed.
Attachment #753091 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a0a73f1116

I've made all the changes that you requested except for the comment change mentioned in comment 11. That block at the top is a standard block repeated in multiple places. It's intent is purely to let readers know that rendering observers do not need to be notified, and why. The fact that it mentions things that we _do_ need to do was just an effort to contrast the rendering observer rules with other things. It wasn't intended to talk about those things themselves.

I think maybe we should revisit that comment to only talk about the rendering observer thing, but change that across all those standard comments.
Summary: nsSVGPathGeometryFrame::NotifySVGChanged needs to take account of stroke-width → nsSVGPathGeometryFrame::NotifySVGChanged needs to take account of stroke-width and vector-effect
(In reply to Jonathan Watt [:jwatt] from comment #12)
> I've made all the changes that you requested except for the comment change
> mentioned in comment 11. That block at the top is a standard block repeated
> in multiple places

Really? I only see it in this one place, with this query (from the beginning of that comment):
http://mxr.mozilla.org/mozilla-central/search?string=%20Changes%20to%20our%20ancestors%20may

...and this query (from later in the comment):
http://mxr.mozilla.org/mozilla-central/search?string=ancestor+changes+cannot+affect
(Regardless, though, I'm on board with simplifying that comment to just mention rendering observers, as you suggest, so that chunk can get its point across and then we can get to more clearly documenting the purpose of the code that comes right after it.  That basically matches what I was going for in comment 11, except with the final " // (comment about how we don't need to notify rendering observers)" bumped up to be the first thing, I guess.)

Could you at least file a followup to simplify that documentation, if you're not going to do it as part of this bug, so we don't forget about it?
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #15)
> tracking, regression (perf)

(technically, this wasn't a perf regression -- it was a rendering regression, which happened to be triggered by a perf-improvement patch.)
https://hg.mozilla.org/mozilla-central/rev/98a0a73f1116
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Really? I only see it in this one place

Huh. Could have sworn that was a standard comment.

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Could you at least file a followup to simplify that documentation, if you're
> not going to do it as part of this bug, so we don't forget about it?

Sure thing. I filed bug 875870.
Comment on attachment 753091 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 869611
User impact if declined: some broken SVG
Testing completed (on m-c, etc.): landed on m-c before last aurora branch
Risk to taking this patch (and alternatives if risky): very low - this patch makes us do some more (necessary) invalidation after the slightly over aggressive optimization from bug 869611
String or IDL/UUID changes made by this patch: none
Attachment #753091 - Flags: approval-mozilla-beta?
Comment on attachment 753091 [details] [diff] [review]
patch

[Triage Comment]
Attachment #753091 - Flags: approval-mozilla-beta? → approval-mozilla-aurora+
Thanks, Ryan.
You need to log in before you can comment on or make changes to this bug.