1486952 - Click/mouseover breaks when SVG shape is moved using a transformation

Status: VERIFIED FIXED

(Core :: SVG, defect, P3)

Description

[Lapo Luchini]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.57 Safari/537.36

Steps to reproduce:

https://github.com/Leaflet/Leaflet/issues/5773#issuecomment-359048426
This originated as a bug in Leaflet but has since been isolated to be specific to SVG in Firefox with a minimal test case that uses no more code form Leaflet.

Demo to reproduce the problem:
https://plnkr.co/edit/k1oSkW02QDei7KsNkDCo?p=preview

Automatic test:
https://plnkr.co/edit/dX5onFWJsg3G2Nrrdxhb?p=preview


Actual results:

Part or all of the SVG shape cannot be clicked, the mouse does not have the "hand" shape.


Expected results:

All of the SVG shape should be clickable and have the correct mouse cursor.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f2e79915832dbd98afafa91751bfad66eab77e13&tochange=0901f80a81dbc1073f3355016119586524cf81e9

Regressed by: Bug 828240

Comment 2

[Tom Klein]

Attachment: Reduced testcase: test hover after viewBox change

This reduced testcase has just an svg with fill on the rectangle set to change on hover (no additional divs or absolute positioning).  After one second the viewBox is updated, which currently breaks hover detection on the rectangle.

Comment 3

[Tom Klein]

Attachment: Bug 1486952 - Update overflow on the correct svg frame(s) when the viewBox changes. r?jwatt

Bug 828240 switched the children only transform from applying to each of the
anonymous child's children to applying directly to the anonymous child instead.
So now when the viewBox changes, we need to update (just) the overflow of the
anonymous child, where previously we were updating the overflows of each of the
children (which was having no effect I guess, since they no longer have the
child only transform applied to them).

Hit testing checks for hits using overflow, so was broken by the lack of
overflow update on the anonymous child (combined with the fact that the
anonymous child is now transformed by the children only transform).

Comment 4

[Tom Klein]

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

Comment 5

[Tom Klein]

I guess the child only transform still gets put on the children of an inner svg, so this patch may break hit testing if you change the viewBox on an inner svg.  I'll need to look into that later.

Comment 6

[Tom Klein]

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

Comment 7

[Tom Klein]

Attachment: Testcase for changing viewBox on inner and outer svgs

My original patch did break hit testing on children of an inner svg, since in that case the viewBox transform is on the children, and the patch changed behavior to update overflows on the inner svg, not its children.  The attached testcase has an outer and inner svg and changes the viewBox three times, as follows:

All versions pass on the first click of the attached testace: a _first_ change of viewBox on an inner svg.  That's because the first viewBox change on an inner svg triggers a reflow, from here:
https://dxr.mozilla.org/mozilla-central/rev/8265be8d02b079c1d6131a2505356f770bf564df/dom/svg/SVGViewportElement.cpp#167
The reason we have hadChildrenOnlyTransform != mHasChildrenOnlyTransform (this is for the inner svg element) is that we never call UpdateHasChildrenOnlyTransform() during the first reflow for an inner svg like we do for an outer svg frame here:
https://dxr.mozilla.org/mozilla-central/rev/8265be8d02b079c1d6131a2505356f770bf564df/layout/svg/nsSVGOuterSVGFrame.cpp#397

Let me know if that's worth filing a bug for (we're doing a reflow when we don't need to).


The second click on the attached testcase - a _second_ change of the viewBox on an inner svg - passes on trunk and with my new patch, but fails with my original patch (because that patch updates the inner svg's overflow, whereas it's the child rect that has the new viewBox transform and requires overflow updating).  I've added a mochitest testcase to catch that.


On trunk, the third click on the attached testcase - a change of the outer svg viewBox - should lead to a fail.  It passes with both versions of my patch since they both update overflow on the anonymous inner child, which is where the changed viewBox transform resides.


One other point I noticed, which I'm not sure is actually an issue or not:

Changing the viewBox triggers an nsChangeHint_ChildrenOnlyTransform restyle event, which triggers a call to GetFrameForChildrenOnlyTransformHint(nsIFrame* aFrame).  The doc for that function says that aFrame can be an ancestor of the frame that triggered the nsChangeHint_ChildrenOnlyTransform event.  Is it really possible to find the right inner or outer frame in general when that happens?  What if a <g> contains several inner svgs and aFrame is the <g> (if that can happen)?  How would it know which one the event occurred on?  Or what if the ancestor was an ancestor of the outer svg and the event occurred on an inner svg (if that can happen)?  I don't know how to trigger such things to happen in order to test, but if it can happen I think it would break hit testing on the inner svg children. I don't know how primary frames are determined and managed (are they updated dynamically during events?), so I don't really know if these are valid concerns or not.

Is that worth filing a bug for if it can happen?

[One last point maybe worth considering that I noticed in passing: we do send an nsChangeHint_UpdateOverflow restyle event along with the nsChangeHint_ChildrenOnlyTransform event when the viewport changes, here:
https://dxr.mozilla.org/mozilla-central/rev/eddbfdc38cbc0d25be44ea92588126db3d0b9a78/dom/svg/SVGViewportElement.cpp#170
But (I think) that leads to an overflow update on either the outer svg or the inner svg, neither of which benefits from an overflow update when the viewBox changes, so we're doing extra work for nothing there.  (I'd have to check all the details again to verify all that, but the fact that this bug exists seems to suggest that the nsChangeHint_UpdateOverflow in the outer svg case at least wasn't helping.)]

Comment 8

[Tom Klein]

(In reply to Tom Klein from comment #7)
> 
> [One last point maybe worth considering that I noticed in passing: we do
> send an nsChangeHint_UpdateOverflow restyle event along with the
> nsChangeHint_ChildrenOnlyTransform event when the viewport changes, here:
> https://dxr.mozilla.org/mozilla-central/rev/
> eddbfdc38cbc0d25be44ea92588126db3d0b9a78/dom/svg/SVGViewportElement.cpp#170
> But (I think) that leads to an overflow update on either the outer svg or
> the inner svg, neither of which benefits from an overflow update when the
> viewBox changes, so we're doing extra work for nothing there.
Actually we do (unsurprisingly...) need to do that, at least for inner svgs where overflow isn't hidden, for example, amongst other cases (I had been thinking that overflow for inner and outer svgs was always just the svg's (x,y,width,height), which is wrong).

> (I'd have to
> check all the details again to verify all that, but the fact that this bug
> exists seems to suggest that the nsChangeHint_UpdateOverflow in the outer
> svg case at least wasn't helping.)]
That was bad reasoning, sorry.

Comment 9

[Tom Klein]

Hi Jonathan - any thoughts here?

Comment 10

[Jonathan Watt [:jwatt]]

Sorry, Tom. It's not going to be until tomorrow that I get to this.

Comment 11

[Pascal Chevrel:pascalc]

Jonathan, could we get a status update with regards to 65? Thanks

Comment 12

[Jonathan Watt [:jwatt]]

Thanks for the reminder, Pascal.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c8f22180210
Update the overflow of the anonymous child when the viewBox changes. r=jwatt

Comment 14

[Cosmin Sabou [:CosminS]]

Backed out for mochitest failures on test_hit-testing-and-viewbox.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=1c8f22180210bf31334c61b0c884ab8611ba7c15&tochange=df7e7d55004ddbd34b26e1eab09bb0faa7500f5d&selectedJob=213174572

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=1c8f22180210bf31334c61b0c884ab8611ba7c15&tochange=df7e7d55004ddbd34b26e1eab09bb0faa7500f5d&selectedJob=213174572

Backout link: https://hg.mozilla.org/integration/autoland/rev/d43f2120961727a23c371b805e3f785d1387021c

Comment 15

[Tom Klein]

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

Comment 16

[Tom Klein]

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

Comment 17

[Tom Klein]

Finally noticed in the screenshots for the failing tests on treeherder that mochitests in automation (apparently) run in a 500x250 window; there's no such restriction locally, which is why my test passed locally but not in automation (I was testing points at x=500 and x=600). I halved all of the coordinates in my test, which seems to have made try happy, so I'll try to reland.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5283c43535ad
Update overflow on the correct svg frame(s) when the viewBox changes. r=jwatt

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5283c43535ad

Comment 20

[Ryan VanderMeulen [:RyanVM]]

We've lived with this long enough that I don't think letting it ride the 65 train is a big deal. Feel free to nominate it for Beta uplift if you feel strongly otherwise, though.

Comment 21

[ovidiu boca[:Ovidiu]]

I tested this on Windows 10 x64, Mac OS X 10.14 and Ubuntu 16.04 with FF Nightly 65.0a1(2018-11-27) and I can't reproduce the issue, based on that I will mark this as verified fixed.