Closed Bug 1486952 Opened Last year Closed Last year
Click/mouseover breaks when SVG shape is moved using a transformation
872 bytes, text/html
46 bytes, text/x-phabricator-request
|Details | Review|
1.85 KB, text/html
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.
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.
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).
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.
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.)]
(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.
Hi Jonathan - any thoughts here?
Sorry, Tom. It's not going to be until tomorrow that I get to this.
Jonathan, could we get a status update with regards to 65? Thanks
Thanks for the reminder, Pascal.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/1c8f22180210 Update the overflow of the anonymous child when the viewBox changes. r=jwatt
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
Attachment #9008407 - Attachment description: Bug 1486952 - Update the overflow of the anonymous child when the viewBox changes. r?jwatt → Bug 1486952 - Update overflow on the correct svg frame(s) when the viewBox changes. r?jwatt
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/5283c43535ad Update overflow on the correct svg frame(s) when the viewBox changes. r=jwatt
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.
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.
You need to log in before you can comment on or make changes to this bug.