1292053 - [e10s] ALT keys ignore prevent default

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[r.chrzanowski]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36 OPR/38.0.2220.41

Steps to reproduce:

1. Open a webpage with JS code
	addEventListener('keydown', function(e){
	console.log(e.keyCode);
      if(e.keyCode == 114 || e.keyCode == 118||  e.keyCode == 121|| e.keyCode == 122  || e.keyCode == 18){
       console.log('prevent');
        e.preventDefault();
      }
    }, true)

2. Press keys listed in if() parenthesis


Actual results:

Some keys ignore e.preventDefault. (F7 - 118; F10 - 121; alt - 18)
Other keys are prevented from running their default action (F3 - 114; F10 - 122)


Expected results:

All keys should have been prevented from running their default action.
Or is it an intended change in Firefox 48?

Comment 1

[r.chrzanowski]

Sorry, bad diagnosis again,

Comment 2

[r.chrzanowski]

Sorry, bad diagnosis again, it seems to just affect multiprocess windows in e10s...

Comment 3

[Neil Deakin]

Using F7 as an example (implemented as a system keypress <handler> in browser.xml), this handler is called before the keydown listener on the page. Masayuki should be able to investigate this.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I'm working on similar bug of <key> elements at bug 1257617.

However, I have no idea to do similar fix for normal key event handler of JS. I guess that we need to add an API to wait a reply from content process and listen "test keydown" event which will be added by bug 1257617.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I recommend that if it's possible keyboard event handlers in chrome should use XUL <key> elements as far as possible. Using normal keyboard event listeners will increase the implementation cost.

Comment 6

[Neil Deakin]

Both <key> elements and <handler event="key..."> go through nsXBLWindowKeyHandler so we should be able to have the same or similar fix for both no?

Also, I didn't investigate the other keys mentioned in this bug (F10 and Alt) which I suspect has to do with the way accesskeys and menus have a complex interaction.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #6)
> Both <key> elements and <handler event="key..."> go through
> nsXBLWindowKeyHandler so we should be able to have the same or similar fix
> for both no?

Oh, really? If so, it's very good news to me. Then, this must be fixed by bug 1257617 too.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

F10 key will be fixed by bug 1347351.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I can confirm the Alt key issue. But I cannot reproduce F7 key issue. Was the implementation was changed?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, we should treat only Alt key handling here. Please file a new bug if you still see this bug with F7.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1292053 - nsMenuBarListener::KeyDown() and nsMenuBarListener::KeyUp() should wait reply even from remote process if it's necessary

nsMenuBarListener::KeyDown() and nsMenuBarLitener::KeyUp() listen to Alt key events and some other key events to cancel handling following eKeyUp event.  In this time, if the menubar isn't active and a remote process has focus, eKeyDown and eKeyUp should be handled in the remote content first because nsMenuBarListener needs to check if the events are consumed by the others.

Review commit: https://reviewboard.mozilla.org/r/157422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157422/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8886634 [details]
Bug 1292053 - nsMenuBarListener::KeyDown() and nsMenuBarListener::KeyUp() should wait reply even from remote process if it's necessary

Canceling the review until fixing bug 1292053.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8886634 [details]
Bug 1292053 - nsMenuBarListener::KeyDown() and nsMenuBarListener::KeyUp() should wait reply even from remote process if it's necessary

This caused the new orange. Sorry for the bug spam.

Comment 16

[Brian Birtles (:birtles)]

Masayuki, this is the bug I mentioned yesterday about not being able to prevent Alt from triggering the menu, even on keyup.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, I forgot this bug...

Comment 18

[Cyrus Cuenca]

Are there any plans to get this fixed? My webapp utilizes the standard pan & zoom modifier keys (alt, ctrl, shift), but these modifiers are made inconsistent by the menu being triggered onkeyup.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Resetting assignee which I don't work on in this several months.

Comment 20

[semiaddict]

This might be related to #1574057.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Will be fixed by the patch for bug 1335347.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This was fixed by bug 1335347 and it contains a test for this case.