1172310 - don't add viewport frame bounds to the layer event regions because they are never the result of hit testing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

The problem with bug 1170764 and bug 1170611 is the same: the area of the viewport frame of the keyboard app gets added to the layer event regions even though the keyboard app has pointer-events: none on the root element.

nsStyleVisibility::GetEffectivePointerEvents has special code to handle the frames of the root element, but viewport frames don't have content so that code doesn't apply to viewport frames.

Bug 1121033 is the same bug, except for the in-process keyboard. The fix for that bug doesn't work because it relies on getting the parent of the root frame, which is in another process in the OOP keyboard case. Anyway based on

https://bugzilla.mozilla.org/show_bug.cgi?id=796452#c22

I think that patch is wrong. mozpasspointer events is meant to pass pointer events to the subdocument if an iframe has pointer-events: none. It is not intended to alter the pointer-events in the subdocument. (The fact that it doesn't really make sense to use mozpasspointerevents and not make the root of the subdocument pointer-events: none probably made it seem like the right fix.)

I think the proper fix is to never add viewport frame bounds to the layer event regions. Because, from my reading of the code, viewport frames never add any display items to the display list, not even a background item when doing hit testing. So they are never event targets. This makes sense since they don't have content, so the event handling would probably not work so great.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e29758cd5dcf

Comment 3

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

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

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

I agree!

Comment 4

[No-Jun Park [:njpark]]

I tested the patch with the layout.scroll.root-frame-containers to false (using edit-pref.sh), and tested the mentioned bugs in the description, and did not find any issues.  The keyboard response was tad slow when typing on certain fields (i.e. add contacts), but I don't think it's related to the patch, as I flashed the tip of the gaia/gecko plus the patch.

Comment 5

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

Awesome, thanks! Did you notice any other smoketest-breakage issues? The change to layout.scroll.root-frame-containers has been backed out a few times already and we'd like to make sure it doesn't get backed out again on the next landing, so getting some general smoketest coverage prior to landing would be great.

Comment 6

[No-Jun Park [:njpark]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Awesome, thanks! Did you notice any other smoketest-breakage issues? The
> change to layout.scroll.root-frame-containers has been backed out a few
> times already and we'd like to make sure it doesn't get backed out again on
> the next landing, so getting some general smoketest coverage prior to
> landing would be great.

I did also test gallery/camera/contacts, and didn't really see anything odd. What I did see was that after I entered the account detail for the email app, the app never actually was able to download and sync messages.  But I also suspect this is not related to the patch, probably some wip code on the tip of gaia.

Comment 7

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

Thanks for the quick turnaround!

Comment 8

[Timothy Nikkel (:tnikkel)]

Just adding some more info that I forgot to add in comment 0.

Part 2 of bug 1168630
https://hg.mozilla.org/mozilla-central/rev/bab9b38fcc1a
is the regressing changeset with containerless flipped on.

Before that changeset there would be no apzc on an ancestor layer to the keyboard app within the keyboard app, so it would walk up to the parent process root apzc and let it handle the event. There by hiding this bug. With part 2 of bug 1168630 with then returned the root apzc of the keyboard app (not of the parent process). The correct fix is to just not hit the keyboard app.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f39ed56f29a4

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f39ed56f29a4