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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Do we need to reflow for a non-scaling-stroke, if the viewport gets smaller?

Comment 3

[Robert Longson [:longsonr]]

Even if the stroke width is not a percentage.

Comment 4

[Jonathan Watt [:jwatt]]

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.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Daniel Holbert [: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 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Jonathan Watt [:jwatt]]

(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.

Comment 9

[Daniel Holbert [:dholbert]]

(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 10

[Daniel Holbert [:dholbert]]

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 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Jonathan Watt [:jwatt]]

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.

Comment 13

[Daniel Holbert [:dholbert]]

(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

Comment 14

[Daniel Holbert [:dholbert]]

(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?

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking, regression (perf)

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] [: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.)

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/98a0a73f1116

Comment 18

[Jonathan Watt [:jwatt]]

(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 19

[Jonathan Watt [:jwatt]]

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

Comment 20

[Alex Keybl [:akeybl]]

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

[Triage Comment]

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9356e183b1c8

Comment 22

[Jonathan Watt [:jwatt]]

Thanks, Ryan.