1049256 - Convert SVG hit-testing to act on an SVG user space point instead of calling nsSVGUtils::GetCanvasTM()

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should convert SVG hit-testing to act on an SVG user space point instead of calling nsSVGUtils::GetCanvasTM() for several reasons:

 * transforming the hit point as we pass it down the tree is cheaper than
   calling GetCanvasTM() at the leaves (at least in the case that the cached
   matrices used by GetCanvasTM() are invalidated, which is often, and we
   want to get rid of NotifySVGChanged() anyway)

 * what space nsISVGChildFrame::GetFrameForPoint()'s aPoint is in is hard to
   get your head round right now since it depends on whether display lists are
   being used, and if they are it's a weird relative-to-the-root-with-
   any-intermediate-transforms-ignored space point

 * GetCanvasTM is itself a horrible thing that makes reasoning about the code a
   nightmare, especially in the painting paths - we want to kill GetCanvasTM
   which includes getting rid of the hit-testing path consumers
   (this also getting us closer to getting rid of NotifySVGChanged)

The choice to have aPoint in the frame's element's SVG user space is not arbitrary. As well as making GetFrameForPoint implementations easier to reason about, it is compatible with both display-list and non-display-list code paths. In the display-list paths we can't include transform attributes, for example, since nsDisplayTransform has already accounted for them. We don't want aPoint to be at the other side of the space-established-by-our-parent-to-space-established-by-us-for-our-children transform though, since we want to check hits against the viewport established by <svg> elements, for example.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - convert SVG hit-testing to act on an SVG user space point instead of calling nsSVGUtils::GetCanvasTM()

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - kill FOR_HIT_TESTING

Part 1 is a bit gnarly to review, sorry. I split this part out to try and somewhat separate things to make it a bit easier.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 1 - convert SVG hit-testing to act on an SVG user space point instead of calling nsSVGUtils::GetCanvasTM()

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 2 - kill FOR_HIT_TESTING

Comment 5

[Jonathan Watt [:jwatt]]

Another plus point to add to those is comment 0 is that this gets non-display-list hit-testing very close to completely working again. More specifically it gets:

  ./mach mochitest-plain --setpref svg.display-lists.hit-testing.enabled=false content/svg

passing, except for four of the sub-tests in content/svg/content/test/test_text_selection.html (without these patches 9 of them fail).

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 8468796 [details] [diff] [review]
part 1 - convert SVG hit-testing to act on an SVG user space point instead of calling nsSVGUtils::GetCanvasTM()

>-nsSVGUtils::HitTestChildren(nsIFrame *aFrame, const nsPoint &aPoint)
>+nsSVGUtils::HitTestChildren(nsSVGDisplayContainerFrame* aFrame,
>+                            const gfxPoint& aPoint)
> {
>+  // First we transform aPoint into the coordinate space established by aFrame
>+  // for its children (e.g. take account of any 'viewBox' attribute):
>+  gfxPoint point = aPoint;
>+  gfxMatrix m;
>+  if (aFrame->GetContent()->IsSVG()) { // must check before cast
>+    m = static_cast<const nsSVGElement*>(aFrame->GetContent())->
>+          PrependLocalTransformsTo(gfxMatrix(), nsSVGElement::eChildToUserSpace);
>+  }
>+  if (!m.IsIdentity()) {
>+    if (!m.Invert()) {
>+      return nullptr;
>+    }
>+    point = m.Transform(point);
>+  }

The identity check and code can go inside the SVG check as if it's not SVG, m must be identity.

>+      // GetFrameForPoint() expects a point in its frame's SVG user space, so
>+      // we need to convert to that space:
>+      gfxPoint p = point;
>+      gfxMatrix m;
>+      if (content->IsSVG()) { // must check before cast
>+        m = static_cast<const nsSVGElement*>(content)->
>+              PrependLocalTransformsTo(gfxMatrix(), nsSVGElement::eUserSpaceToParent);
>+      }
>+      if (!m.IsIdentity()) {
>+        if (!m.Invert()) {
>+          continue;
>+        }
>+        p = m.Transform(p);
>+      }

ditto

r=longsonr with above addressed

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f0ee96a8e12
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8518bbdd3f0

Thanks, Robert!

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f0ee96a8e12
https://hg.mozilla.org/mozilla-central/rev/f8518bbdd3f0