Closed Bug 1126876 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 3 obsolete files)

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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Attached patch WIP (obsolete) — Splinter Review
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  +-----+---------------------+-----------------------+
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.
Yup, that makes sense. I don't know why I didn't think of that.. thanks!
This is much cleaner and seems to work just as well.
Assignee: nobody → bugmail.mozilla
Attachment #8556504 - Attachment is obsolete: true
Attachment #8557241 - Flags: review?(tnikkel)
Attachment #8557241 - Flags: review?(roc)
Whoops, had an extra hunk in there that I didn't need.
Attachment #8557241 - Attachment is obsolete: true
Attachment #8557241 - Flags: review?(tnikkel)
Attachment #8557241 - Flags: review?(roc)
Attachment #8557250 - Flags: review?(tnikkel)
Attachment #8557250 - Flags: review?(roc)
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.
Attachment #8557250 - Flags: review?(tnikkel) → review+
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.
Attachment #8557250 - Flags: review?(roc)
Here's the PresShellState version. I suppose it's a little simpler conceptually but the code is largely the same.
Attachment #8557912 - Flags: review?(tnikkel)
Attachment #8557912 - Flags: review?(roc)
Attachment #8557912 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/93c0418bd4b2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
Attachment #8557912 - Flags: approval-mozilla-b2g37?
Attachment #8557912 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.