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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: heycam, Assigned: jwatt)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 wanted)

Details

()

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

9 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

9 years ago
Posted image Test
Assignee

Updated

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

Comment 2

9 years ago
(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

9 years ago
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

9 years ago
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

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

Comment 5

9 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

9 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

9 years ago
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

9 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

9 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

9 years ago
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

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

Comment 11

9 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

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