Closed
Bug 1172310
Opened 9 years ago
Closed 9 years ago
don't add viewport frame bounds to the layer event regions because they are never the result of hit testing
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8616439 -
Flags: review?(roc)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8616439 [details] [diff] [review]
patch
Review of attachment 8616439 [details] [diff] [review]:
-----------------------------------------------------------------
I agree!
Attachment #8616439 -
Flags: review?(roc) → review+
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: needinfo?(npark)
Comment 6•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(npark)
Comment 7•9 years ago
|
||
Thanks for the quick turnaround!
Assignee | ||
Comment 8•9 years ago
|
||
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.
Blocks: 1168630
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•