1126876 - Event regions are not properly built for contents of iframes with pointer-events:none

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

It seems like there is a difference between setting pointer-events:none on an overflow:scroll div versus setting it on an iframe. With the div, the contents of the div automatically inherit the pointer-events:none, and so the check at [1] ensures that all the contents of the div get excluded from the event-regions. However with iframes, setting pointer-events:none on the <iframe> element doesn't automatically trickle down to the contents (at least not via StyleVisibility()->GetEffectivePointerEvents) and so the contents of such iframes do end up included on the event-regions display items.

I believe that gecko hit-testing handles this case at least in part using the code at [2] where it skips over building certain items if pointer-events:none is set on the iframe. It's not clear to me yet how gecko skips over other frames inside the iframe (for example a div in the subdocument). There's probably a piece of code somewhere I'm missing.

Anyway, this behaviour is the root cause behind bug 1126427. In that bug, there is an iframe with opacity:0 and pointer-events:none sitting on top of a scrollable div, but the contents of the iframe end up generating hit regions that absorb the touch input and prevent them from getting to the scrollable div as expected.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=7c4059ebfcf7#3101
[2] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=07029b5f00e9#351

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> It's not clear to me yet how gecko
> skips over other frames inside the iframe (for example a div in the
> subdocument). There's probably a piece of code somewhere I'm missing.

Doh, this is just a few lines down, at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=07029b5f00e9#371

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

This is my ugly fix; it seems to fix all the problems I was seeing. It's based on this table of desired outcomes. There's probably room for optimization by storing flags on the presShell so that we don't have to keep walking up all the time.

                    pointer-events:none                 
                                                        
   +-----+---------------------+-----------------------+
   |     |      Yes            |        No             |
   |     |                     |                       |
   +---------------------------------------------------+
m  |     |                     |                       |
o  |     | Iframe border/back- | <iframe> border/back- |
z  |     | ground don't gener- | ground and descendant |
p  |     | ate hit regions but | frames all generate   |
a  |Yes  | descendant frames   | hit regions           |
s  |     | do.                 |                       |
s  |     |                     |                       |
p  |     |                     |                       |
o  |     |                     |                       |
i  |     |                     |                       |
n  +---------------------------------------------------+
t  |     |                     |                       |
e  |     | None of the iframe  | <iframe> border/back- |
r  |     | border/background   | ground and descendant |
e  |No   | or descendant frames| frames all generate   |
v  |     | generate hit regions| hit regions           |
e  |     |                     |                       |
n  |     |                     |                       |
t  |     |                     |                       |
s  +-----+---------------------+-----------------------+

Comment 3

[Timothy Nikkel (:tnikkel)]

Can we just flip a flag on the display list builder when we enter the subdoc of a subdocument frame that has pointer-events: none (without mozpasspointerevents) and restore it when we leave? And then AddFrame can just exit if that flag is set. We can turn that flag on if we are pointer-events: none (without mozpasspointerevents), but otherwise we leave it alone.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Yup, that makes sense. I don't know why I didn't think of that.. thanks!

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Prevent building event regions for things inside pointer-events:none iframes

This is much cleaner and seems to work just as well.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Prevent building event regions for things inside pointer-events:none iframes

Whoops, had an extra hunk in there that I didn't need.

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8557250 [details] [diff] [review]
Prevent building event regions for things inside pointer-events:none iframes

I think calling it mInsidePointerEventsNoneDoc would be clearer.

Comment 8

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

Comment on attachment 8557250 [details] [diff] [review]
Prevent building event regions for things inside pointer-events:none iframes

Review of attachment 8557250 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine basically...

::: layout/base/nsDisplayList.h
@@ +932,5 @@
>    bool                           mHaveScrollableDisplayPort;
> +  // True if we are inside an area of the frame tree that has the
> +  // pointer-events:none style in effect and should not build touch-sensitive
> +  // regions. Note that this is only set when we enter subdocuments, because
> +  // within a document itself the pointer-events:none is inherited.

I think it would be clearer to put this on CurrentPresShellState(), and have a comment that explains that this is a per-document flag turning off event handling for all content in the document, and that it's set when we enter a subdocument for a pointer-events:none frame without mozpasspointerevents. Right now the comment is confusing.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Prevent building event regions for things inside pointer-events:none iframes (PresShellState version)

Here's the PresShellState version. I suppose it's a little simpler conceptually but the code is largely the same.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93c0418bd4b2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93c0418bd4b2

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8557912 [details] [diff] [review]
Prevent building event regions for things inside pointer-events:none iframes (PresShellState version)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 928833
User impact if declined: when we encounter a visible <iframe> with pointer-events:none, the iframe will still receive some events via APZ. This can cause unexpected behaviour affecting user interaction like taps not doing what they're supposed to. Bug 1126427 was one example of this (but which we worked around in a different way)
Testing completed: locally
Risk to taking this patch (and alternatives if risky): low risk, code is pretty straightforward
String or UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/34f99e78f6f4