Closed
Bug 1119074
Opened 10 years ago
Closed 10 years ago
[e10s] keypress while popup menu is active should not be delivered to the web page
Categories
(Firefox :: Untriaged, defect)
Tracking
()
RESOLVED
FIXED
Firefox 41
People
(Reporter: heycam, Assigned: gw280)
References
Details
Attachments
(1 file, 2 obsolete files)
4.16 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
STR:
1. Open an e10s window with two tabs, one containing a GitHub page such as https://github.com/heycam/webidl/pull/22 and the other can be anything.
2. Select the GitHub page tab.
3. Right click on the other tab's tab.
4. Press "C" to select the "Close Tab" menu item.
5. Notice that the GitHub page has navigated to the "Create a new issue" page, presumably because the page got a keypress event for the "C".
Updated•10 years ago
|
tracking-e10s:
--- → ?
Updated•10 years ago
|
Comment 1•10 years ago
|
||
I can't reproduce this. Has it been fixed?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gwright
Assignee | ||
Comment 2•10 years ago
|
||
I can reproduce this. github doesn't seem to respond to a 'c' keypress anymore (or at least, on my machine it doesn't), but if you press 's' it focuses the search box when it shouldn't.
Assignee | ||
Comment 3•10 years ago
|
||
I'm not sure if this is the correct fix here, but basically what's happening is that we're dispatching the event to the DOM here: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8156, then receiving it at http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2150. consume is set to true, and we mark the event to stop propagation, but it still gets sent to EventStateManager::PostHandleEvent, and eventually still ends up being sent across the process boundary here: http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#3363
Setting mNoCrossProcessBoundaryForwarding (obviously) ensures it doesn't get sent across, but I'm unsure as to the wider implications of this. Does it make sense to stop cross process forwarding if propagation is stopped (I think it does?).
Attachment #8605481 -
Flags: review?(felipc)
Assignee | ||
Comment 4•10 years ago
|
||
Let's see how much this breaks...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45cebc870132
Comment 5•10 years ago
|
||
Comment on attachment 8605481 [details] [diff] [review]
0001-Bug-1119074-Mark-events-which-have-been-set-to-no-lo.patch
Review of attachment 8605481 [details] [diff] [review]:
-----------------------------------------------------------------
Looked to me like a reasonable thing to do, but there were tests failures from that change, right? Perhaps it's just those tests that are broken..
You might wanna check what smaug thinks about this too.
Attachment #8605481 -
Flags: review?(felipc) → feedback+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bugs)
Comment 7•10 years ago
|
||
... in case the context menu handles the event.
Comment 8•10 years ago
|
||
But yeah, I guess that patch works too, and it is probably something we want.
Assignee | ||
Comment 9•10 years ago
|
||
nsXULPopupManager already calls PreventDefault() at http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2223; it doesn't seem to have the desired effect? Is there another bug here at play?
Comment 10•10 years ago
|
||
I reported something (bug 1168058) which seems to be a duplicate of this bug report.
This is a security issue. Pages should not have access to my typing unless I intend to type on the page.
Assignee | ||
Comment 11•10 years ago
|
||
I think this is a safer approach. Rather than hit the event's StopPropagation method with the no-cross-process hammer, I've added a new method to disallow cross-process forwarding explicitly, and only called it from within nsXULPopupManager. I think this should pass the tests now.
Attachment #8605481 -
Attachment is obsolete: true
Attachment #8610223 -
Flags: review?(felipc)
Assignee | ||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
Comment on attachment 8610223 [details] [diff] [review]
0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch
Review of attachment 8610223 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/interfaces/events/nsIDOMEvent.idl
@@ +124,5 @@
> /**
> + * The StopCrossProcessForwarding method is used to disallow this event
> + * from crossing process boundaries
> + */
> + void stopCrossProcessForwarding();
this will expose this function to web content. You'll need something like [chromeonly] or [notxpcom] (not sure which is the right one to use here). But I think it would be fine to just set the flag manually from nsXULPopupManager. Maybe you tried and that didn't work because you need a concrete class to do that? Try using GetInternalNSEvent to get access to mFlags.
Attachment #8610223 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Updated as per smaug and felipe's comments on IRC.
- Added [noscript] attribute to stopCrossProcessForwarding
- Removed random comments from Event.h
Attachment #8610223 -
Attachment is obsolete: true
Attachment #8610241 -
Flags: review?(bugs)
Comment 15•10 years ago
|
||
Comment on attachment 8610241 [details] [diff] [review]
0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch
So do we actually need StopCrossProcessForwarding() calls in this case?
What if nsXULPopupManager::KeyDown called PreventDefault() ?
Comment 16•10 years ago
|
||
Comment on attachment 8610241 [details] [diff] [review]
0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch
StopCrossProcessForwarding() implementation looks ok to me, but
because of http://hg.mozilla.org/mozilla-central/rev/d5dc2d51f002
I'd prefer if masayuki looked at this.
gwright, could you test if just adding
aKeyEvent->PreventDefault(); to nsXULPopupManager::KeyDown where it used to
be before http://hg.mozilla.org/mozilla-central/rev/d5dc2d51f002 would fix this.
Attachment #8610241 -
Flags: review?(bugs) → review?(masayuki)
Assignee | ||
Comment 17•10 years ago
|
||
OK, so it looks like we can't just call PreventDefault() because that stops the popup from handling a keypress that's not a navigation event (ie - you should be able to hit 'P' when the popup from right-clicking a tab is open to pin it).
If PreventDefault() is called unconditionally in KeyDown(), then the event doesn't get sent to content as this bug wants, but then KeyPress() never gets the key event and so the popup doesn't get to handle it. This makes sense based on masayuki's comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=501496#c16
I think we can probably do away with the StopCrossProcessForwarding() calls everywhere except in KeyDown, but I think we should probably keep them in as they aren't expensive.
Comment 18•10 years ago
|
||
Comment on attachment 8610241 [details] [diff] [review]
0001-Bug-1119074-If-we-re-stopping-event-propagation-from.patch
I see. The StopCrossProcessForwarding calls are very cheap.
Attachment #8610241 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in
before you can comment on or make changes to this bug.
Description
•