508179 - Implement 'pointer-events:none;' for all elements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Breaking this out from bug 380573 to implement the none value at least.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

The only substantive change is the one to nsDisplayList.cpp (seems like I can put the check there rather than modifying all the HitTest implementations. The rest is just moving things around so that pointer-events will work with --disable-svg.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 392410 [details] [diff] [review]
patch

Need dbaron review on the style system changes.

+	"pointer-events": {
+    domProp: "pointerEvents",
+    inherited: true,
+    type: CSS_TYPE_LONGHAND,
+    initial_values: [ "visiblePainted" ],
+    other_values: [ "visiblePainted", "visibleFill", "visibleStroke", "visible",
+                    "painted", "fill", "stroke", "all", "none" ],
+    invalid_values: []

Looks like you need tabs here to be consistent with the rest of the file.

Can you add a test for the case where the root element is pointer-events:none? I'm not actually sure what should happpen in that case.

Also a test for the case where an element is pointer-events:none and the child is pointer-events:visiblePainted. Plus the same test where the parent element is opacity:0.5 (so it gets its own stacking context). I think the patch handles these cases correctly though. Yay for display lists :-).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Another interesting situation is when you have an IFRAME that's pointer-events:none. I think no events should be sent to the IFRAME's contents, but it seems to me that right now they will be. One way to fix this would be to modify nsSubDocumentFrame::BuildDisplayList to just bail out if aBuilder->IsForEventDelivery is true and the frame is pointer-events:none.

You'll definitely need a test; you're going to have to synthesize a mouse event, elementFromPoint won't work.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 392410 [details] [diff] [review]
patch

r- pending resolution of the IFRAME issue

Comment 6

[Jonathan Watt [:jwatt]]

> Can you add a test for the case where the root element is pointer-events:none?
> I'm not actually sure what should happpen in that case.

Probably the mouse event should be targeted at the document object, or else no event should be dispatched at all. I can't think of a good reason for users to prefer one over the other. However, currently we dispatch events at the documentElement if no element in the document is hit, which seems wrong to me. Any objections to me changing that? If not, any preferences as to which of the two choices to use?

> Also a test for the case where an element is pointer-events:none and the child
> is pointer-events:visiblePainted.

That's already in the test_pointer-events.html test in the patch ("eight").

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> Probably the mouse event should be targeted at the document object, or else no
> event should be dispatched at all. I can't think of a good reason for users to
> prefer one over the other. However, currently we dispatch events at the
> documentElement if no element in the document is hit, which seems wrong to me.
> Any objections to me changing that? If not, any preferences as to which of the
> two choices to use?

What does Webkit do?

Sending it directly to the document would be far too weird. I think perhaps the least-weird thing would be to dispatch it to the documentElement, as a special case.

> > Also a test for the case where an element is pointer-events:none and the child
> > is pointer-events:visiblePainted.
> 
> That's already in the test_pointer-events.html test in the patch ("eight").

Ah, great.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we want pointer-events:none on the root element of the document in an IFRAME to cause the event to be dispatched to the IFRAME element in the parent document. Then to be consistent, when a document is at the top level and its root element is pointer-events:none, it should not see the event at all. We probably should still fire chrome event handlers, though. Olli can probably tell us how to do that.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> Sending it directly to the document would be far too weird. 
That doesn't sound weird to me, not at all.
I think we should dispatch events to document, if document doesn't have
any elements, or if documentElement can receive any events.

(In reply to comment #8)
> I think we want pointer-events:none on the root element of the document in an
> IFRAME to cause the event to be dispatched to the IFRAME element in the parent
> document.
Or should it work so that if iframe has pointer-events:none, its contentDocument
doesn't get any events?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 392410 [details] [diff] [review]
patch

Seems fine to me other than the whitespace issue in property_database.js and roc's iframe issue, so marking review+ on my end so that you don't have to bug me about the revised patch.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #9)
> (In reply to comment #7)
> > Sending it directly to the document would be far too weird. 
> That doesn't sound weird to me, not at all.
> I think we should dispatch events to document, if document doesn't have
> any elements, or if documentElement can receive any events.

You mean "if documentElement cannot receive any events"?

I guess you're right. Giving the root element pointer-events:none should be consistent with there being no root element.

> (In reply to comment #8)
> > I think we want pointer-events:none on the root element of the document in
> > an IFRAME to cause the event to be dispatched to the IFRAME element in the
> > parent document.
> Or should it work so that if iframe has pointer-events:none, its
> contentDocument
> doesn't get any events?

Certainly <iframe style="pointer-events:none"> should prevent mouse events from reaching anything inside the IFRAME.

The question is how <iframe src="data:text/html,&lt;html style='pointer-events:none'&gt;"> should behave. I guess sending mouse events to the IFRAME's contentDocument makes sense (and similar for root documents). The implementation is a little tricker though. We'll need a display list item that can catch the event. Maybe in nsSubdocumentFrame::BuildDisplayList, we should remove the IsForEventDelivery check from here:
  if (!aBuilder->IsForEventDelivery()) {
    // Add the canvas background color.
    rv = presShell->AddCanvasBackgroundColorItem(
           *aBuilder, childItems, f ? f : this, &shellBounds);
  }

Timothy, do you forsee any problem with that?

Then we'd need to arrange for events targeted at the canvas background color item (associated with the ViewportFrame) to be delivered directly to the document instead of an element. I'm not sure where that goes.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> You mean "if documentElement cannot receive any events"?
Oops. Right, I mean "cannot"

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to comment #11)
> > (In reply to comment #8)
> > > I think we want pointer-events:none on the root element of the document in
> > > an IFRAME to cause the event to be dispatched to the IFRAME element in the
> > > parent document.
> > Or should it work so that if iframe has pointer-events:none, its
> > contentDocument
> > doesn't get any events?
> 
> Certainly <iframe style="pointer-events:none"> should prevent mouse events from
> reaching anything inside the IFRAME.
> 
> The question is how <iframe src="data:text/html,&lt;html
> style='pointer-events:none'&gt;"> should behave. I guess sending mouse events
> to the IFRAME's contentDocument makes sense (and similar for root documents).
> The implementation is a little tricker though. We'll need a display list item
> that can catch the event. Maybe in nsSubdocumentFrame::BuildDisplayList, we
> should remove the IsForEventDelivery check from here:
>   if (!aBuilder->IsForEventDelivery()) {
>     // Add the canvas background color.
>     rv = presShell->AddCanvasBackgroundColorItem(
>            *aBuilder, childItems, f ? f : this, &shellBounds);
>   }
> 
> Timothy, do you forsee any problem with that?
> 
> Then we'd need to arrange for events targeted at the canvas background color
> item (associated with the ViewportFrame) to be delivered directly to the
> document instead of an element. I'm not sure where that goes.

The IsForEventDelivery check is there because some mochitests would fail because events would die in the solid color item (what AddCanvasBackgroundColorItem adds) when the solid color item was based on a null frame. Solid color items always have a non-null frame now so the events wouldn't die. But for event delivery display lists, the solid color item would be behind the background item for the canvas frame (which is what currently get the events) so the solid color item wouldn't get any hits. If you got around that somehow, the events would goto the viewport frame instead of the canvas frame, I don't know if that would cause problems or not, but it would be a change.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's right, if the root element is pointer-events:none then the canvas background won't accept the event so we'll return the viewport frame (which has no associated element). That tells us we have to dispatch the event to the frame's prescontext's document.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I do have another issue with this patch:

I think it's really important that we also implement the 'auto' value.  The WebKit spec defines this as:

auto
    In SVG content, behave as visiblePainted. Otherwise, behave as visible. 

The reason is that for some uses of pointer-events, people will want to set the property to 'none' on an element and then "undo" that for some descendant of that element.  We want people to do that undoing with 'auto' rather than with one of the other values which we'll eventually change to mean something different.

I think implementing 'auto' should be equally simple; just add the value and then check for it in the code where SVG checks for 'pointer-events'.

Comment 16

[Jonathan Watt [:jwatt]]

Using the document object as an event target is actually not simple. The entire event handling infrastructure is set up to pass around and handle an nsIContent object, and documents don't implement that interface. We could change everything to pass around an nsINode, but that would be a fair bit of work and probably require a lot of testing of node type and converting/casting to nsIContent as necessary.

Putting pointer-events="none" on the root element seems like a bit of a weird edge case to me. Although it seems wrong to target an element with pointer-events="none" on it, I'm not sure that it's really useful to be able to put that on the root element. Is it worth going to a lot of trouble to support it? I guess there's also the issue of what should happen if the document has no elements, so maybe the code should be changed to handle nsINode anyway.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> The entire event handling infrastructure is set up to pass around and handle
> an nsIContent object
What case do you think here? What expects to have nsIContent? Sure presshell
does in some cases, but that shouldn't be difficult to change.

Or am I missing something here?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #16)
> pointer-events="none" on it, I'm not sure that it's really useful to be able to
> put that on the root element. Is it worth going to a lot of trouble to support
> it? I guess there's also the issue of what should happen if the document has no
> elements, so maybe the code should be changed to handle nsINode anyway.

I wouldn't worry about this edge case too much; it seems reasonable if we handle it the same way as we handle a document with no elements.  I certainly wouldn't let this block the feature from landing in 1.9.2, which I'd like to see happen.

(Any progress on the rest of the work here?)

Comment 19

[Jonathan Watt [:jwatt]]

To answer Olli's question, the presshell code doesn't seem to be too hard to modify, but nsEventStateManager also needs no be changed and that turns out to be fairly extensive surgery. It got a bit out of hand, so I'm going to leave that until later since I think what we do with pointer-events on the root is relatively unimportant.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 396873 [details] [diff] [review]
patch

Looks good! Let's get this in!

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> ..but nsEventStateManager also needs no be changed and that turns out to
> be fairly extensive surgery. It got a bit out of hand, so I'm going to leave
> that until later since I think what we do with pointer-events on the root is
> relatively unimportant.
Could you file a followup to change ESM.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Shouldn't the initial value be 'auto' rather than 'visiblePainted'?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #23)
> Shouldn't the initial value be 'auto' rather than 'visiblePainted'?

(this requires changing property_database.js and nsStyleStruct.cpp)

Comment 25

[Jonathan Watt [:jwatt]]

And nsRuleNode.cpp. ;-) It's a user visible change, but I think it's the right thing to do so that's what I've pushed:

http://hg.mozilla.org/mozilla-central/rev/390fb109b9fe

Comment 26

[j.j.]

I assume this bug implemented exactly that:
http://webkit.org/specs/PointerEventsProperty.html
Is this true?

Comment 27

[Jonathan Watt [:jwatt]]

All we do is add 'auto' as a more generic value name for the default behavior, and allow the value 'none' (to say the element cannot be the target of mouse events) to apply to all elements. It's as simple as that. Both obvious and good extensions.

The webkit document doesn't seem to propose anything more, although it does say about the value 'visible': "The given element receives pointer events". That seems next to useless to me though, and needs to be much better specified. In other words, I'm not really sure the webkit document actually does much.

As far as what we do goes, I've started an end user doc page here:

https://developer.mozilla.org/en/CSS/pointer-events

Comment 28

[Jonathan Watt [:jwatt]]

Olli, the followup bug I filed that would cover changing ESM is bug 513188.

Comment 29

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ec457a432c0b