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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: heycam, Assigned: jwatt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(status2.0 wanted)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
If a shape has

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

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

Comment 1

8 years ago
Created attachment 498405 [details]
Test
(Assignee)

Updated

8 years ago
Assignee: nobody → jwatt
status2.0: --- → wanted
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

8 years ago
Created attachment 506427 [details] [diff] [review]
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.
Attachment #506427 - Flags: review?(longsonr)
(Assignee)

Comment 3

8 years ago
Created attachment 506428 [details] [diff] [review]
rename current test_pointer-events.html + Makefile.in change
Attachment #506427 - Attachment is obsolete: true
Attachment #506428 - Flags: review?(longsonr)
Attachment #506427 - Flags: review?(longsonr)
Attachment #506428 - Flags: review?(longsonr) → review+
(Assignee)

Comment 4

8 years ago
Created attachment 506619 [details] [diff] [review]
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.
Attachment #506619 - Flags: review?(longsonr)
(Assignee)

Updated

8 years ago
Attachment #506619 - Flags: review?(cam)
(Reporter)

Comment 5

8 years ago
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.)
Attachment #506619 - Flags: review?(cam) → review+
(Reporter)

Comment 6

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

Comment 7

8 years ago
Created attachment 506643 [details] [diff] [review]
part 2: fix the bug and add an extensive test

Address heycam's review comments on the test and add an additional cleanup function.
Attachment #506619 - Attachment is obsolete: true
Attachment #506643 - Flags: review+
Attachment #506619 - Flags: review?(longsonr)
(Assignee)

Comment 8

8 years ago
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.
Attachment #506643 - Flags: review?(roc)
Attachment #506643 - Flags: approval2.0?
(Assignee)

Comment 9

8 years ago
(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.
Attachment #506643 - Flags: review?(roc)
Attachment #506643 - Flags: review+
Attachment #506643 - Flags: approval2.0?
Attachment #506643 - Flags: approval2.0+
(Assignee)

Comment 10

8 years ago
Created attachment 506708 [details] [diff] [review]
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".
Attachment #506643 - Attachment is obsolete: true
Attachment #506708 - Flags: review?(longsonr)
(Assignee)

Updated

8 years ago
Attachment #506708 - Attachment is patch: true
Attachment #506708 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 11

8 years ago
longsonr: since the test is reviewed, you only need to review the code change.
Attachment #506708 - Flags: review?(longsonr) → review+
(Assignee)

Comment 12

8 years ago
http://hg.mozilla.org/mozilla-central/rev/049638523ae9
http://hg.mozilla.org/mozilla-central/rev/4d6554646378
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.