619959 - pointer-events:visiblePainted (and others) on a zero opacity stroke should dispatch mouse events

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

If a shape has

  stroke: black;
  stroke-opacity: 0;
  pointer-events: visiblePainted/painted;

then pointer events should be dispatched when mousing over the stroke.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Test

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: rename current test_pointer-events.html

(This is a file rename, so bugzilla's diff view probably won't work.)

Rational for move: I have a patch for this bug, and am still working on a fairly thorough test to ensure we're not missing any other cases. Basically the test will generate code to check fill/stroke hit testing for thousands of permutations of different values for pointer-events, fill, stroke, fill-opacity, stroke-opacity and opacity. It will do this for a basic graphics element (at least initially). As the more "canonical" test for pointer-events, this file should probably be the one called "test_pointer-events.html", and the existing one with extra tests for things like foreignObject should have a name indicating that it contains additional tests. As such I'd like to rename the existing test to keep its history, but I can't to that in the same patch as the one that adds the new test_pointer-events.html. Hence this rename needs to be a separate patch.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: rename current test_pointer-events.html + Makefile.in change

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 2: fix the bug and add an extensive test

The problem is that nsSVGPathGeometryFrame::mRect for the <rect> in question doesn't take stroke into account, so when we check mRect during hit testing we end up returning early at this line:

http://hg.mozilla.org/mozilla-central/annotate/2926ad5face7/layout/svg/base/src/nsSVGPathGeometryFrame.cpp#l163

The reason that mRect isn't taking the stroke into account is because in nsSVGPathGeometryFrame::UpdateCoveredRegion we call nsSVGGeometryFrame::HasStroke(), and HasStroke only returns true if |style->mStrokeOpacity > 0|.

The included test checks 1458 permutations of 'pointer-events', 'fill/stroke', 'fill/stroke-opacity', 'opacity' and 'visibility', so I'm pretty darn confident we're now handling 'pointer-events' correctly for simple graphics primitives.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 506619 [details] [diff] [review]
part 2: fix the bug and add an extensive test

(jwatt asked me to review the test.)

In the SVG spec, pointer-events="painted" says that it will capture pointer events if "the ‘fill’ property has an actual value other than none".  "actual value" in the CSS sense likely means "none" if fill="url(#badReference) none", but the SVG spec doesn't really say.  It would be good to test that too in the future if the spec is be clarified.

The test looks good to me, though.  Feel free to address any or none of the following comments:

+ * character then the requirement for a hit is that it's value is NOT any

s/it's/its/

+function for_all_permutations(inputs, callback)
+{
+  var current_permutation =
+    arguments.length == 2 ? {} : arguments[2];

It might be slightly more idiomatic to do:

  function for_all_permutations(inputs, callback)
  {
    current_permutation = arguments[2] || {};

+      for_all_permutations(inputs.slice(1), callback, current_permutation);

Slicing the array to pass in to the recursive call will result in O(n^2) copying in the length of inputs.  You could avoid all of the copying by just passing in inputs plus an index, if that starts to be a problem.  (Although I gather that it isn't at the moment.)

Comment 6

[Cameron McCormack (:heycam)]

There are also various requirements on pointer-events applying to SVG text elements and raster images.  It would be good to test those too.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 2: fix the bug and add an extensive test

Address heycam's review comments on the test and add an additional cleanup function.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 506643 [details] [diff] [review]
part 2: fix the bug and add an extensive test

Since this is only a three line patch and roc needs to look at it for approval, ma as well get review from him.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to comment #6)
> There are also various requirements on pointer-events applying to SVG text
> elements and raster images.  It would be good to test those too.

Yeah, that's bug 619955 and others.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch 2: fix the bug by fixing nsSVGPathGeometryFrame::GetHittestMask

Actually I think the way to fix the current setup is to fix nsSVGPathGeometryFrame::GetHittestMask rather than to include stroke bounds in mRect when stroke-opacity="0".

Comment 11

[Jonathan Watt [:jwatt]]

longsonr: since the test is reviewed, you only need to review the code change.

Comment 12

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/mozilla-central/rev/049638523ae9
http://hg.mozilla.org/mozilla-central/rev/4d6554646378