[non-e10s] Keyboard events which are reserved shortcuts in chrome shouldn't be fired on web content like e10s mode

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: linuxhippy, Assigned: masayuki)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {regression})

43 Branch
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 wontfix, firefox48 fixed)

Details

User Story

As a user using key combinations, I should *always* be able to:

1. open a new window
2. close the current window
3. open a new tab
4. quit the browser
5. focus the location field
6. reload the page

We should never send these events to content.

FYI, the effectiveness of onkeypress/preventDefault for these keybindings is a bit of a mess across browsers:

OS X

Chrome & Safari

- N, W, Q, T
- F5 ( a11y )

Safari Only:

- R, L

Windows

Chrome & Edge
- Ctrl + F4 - closes window

Edge only
- Cltr+P triggers a double action

Chrome
- N, W, T

Attachments

(7 attachments, 8 obsolete attachments)

40.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.44 KB, patch
Details | Diff | Splinter Review
6.49 KB, patch
Details | Diff | Splinter Review
10.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.88 KB, patch
Details | Diff | Splinter Review
12.32 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150908030203

Steps to reproduce:

1. Opened Outlook Web Access
2. Pressed Ctrl+N


Actual results:

Outlook Web Access opened a new Email-Window
Firefox opened a new, empty browser window


Expected results:

The Ctrl+N default shortcut should be supressed (this is how outlook web access behaves on Firefox-stable and IE). So no new Firefox-Window should have been opened, just the Outlook Web Access email compose window.

multi-process mode was enabled
(Reporter)

Comment 1

4 years ago
Just double-checked, the issue is clearly caused by "multi-process nightly".
When I disable this option, FireFox behaves as expected.
Blocks: e10s
Component: Untriaged → Keyboard Navigation
Summary: Default shortcuts are not supressed when overridden by web-application → [e10s] Default shortcuts are not supressed when overridden by web-application
Flags: needinfo?(jmathies)
(Reporter)

Comment 2

3 years ago
Behaviour changed with FF43: Instead of just opening a new browser window, a new blank browser window as well as the new-email window is opened.
expected behaviour is that just the new-email window should pop up.
Assignee: nobody → jgriffiths
Safari, Chrome and MS Edge all prevent JS from overriding this keybinding, and it's potentially user hostile to override core system bindings like this from the web page.

My inclination is to WONTFIX. I'm needinfo'ing Mike Taylor so he's aware of this compatibility breakage. What I would actually prefer us to do here ( in an ideal world ) is pick a version of Gecko and change the behaviour for multi & single process modes to make it easier for web app developers to degrade gracefully.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(miket)
Resolution: --- → WONTFIX
I don't think we can leave this unfixed. We need to do something here. The safest option is to follow what non-e10s FF does here.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
This is a regression of bug 1074971. If <command> element which is associated with a <key> element has "reserved" attribute, a call of peventDefault() of keypress event cannot prevent the default action (i.e.,  nsXBLWindowKeyHandler ignores defaultPrevented attribute intentionally).

In non-e10s mode, this is handled after web contents receive keypress events. On the other hand, in e10s mode, the key events are never sent to web contents. So, in e10s mode, web contents never receive reserved keyboard events. This is too bad approach for backward compatibility. I don't understand why such approach is granted. However, indeed, if we send reserved keypress events to web pages, it may cause "double action". E.g., in this case, Outlook opens new E-mail window and Firefox also opens new window after that. The former keeps backward compatibility and the latter protects users from that users cannot move focus to another document from a document. Sigh...
Blocks: 1074971
Keywords: regression
Again, Safari, Chrome and MS Edge all prevent JS from overriding this keybinding. I am comfortable with breaking compat here as long as it is consistent between e10s and non-e10s modes.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #6)
> Again, Safari, Chrome and MS Edge all prevent JS from overriding this
> keybinding. I am comfortable with breaking compat here as long as it is
> consistent between e10s and non-e10s modes.

Sounds reasonable. Interop++.
Flags: needinfo?(miket)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #6)
> Again, Safari, Chrome and MS Edge all prevent JS from overriding this
> keybinding. I am comfortable with breaking compat here as long as it is
> consistent between e10s and non-e10s modes.

Chrome doesn't send 'n' keydown event if Ctrl key is pressed on Windows. But Edge is not so and a call of preventDefault() can prevent to open new window. Did you really test it? As far as I tested, Edge breaks the event spec only when Ctrl+Tab.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #8)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #6)
> > Again, Safari, Chrome and MS Edge all prevent JS from overriding this
> > keybinding. I am comfortable with breaking compat here as long as it is
> > consistent between e10s and non-e10s modes.
> 
> Chrome doesn't send 'n' keydown event if Ctrl key is pressed on Windows. But
> Edge is not so and a call of preventDefault() can prevent to open new
> window. Did you really test it? As far as I tested, Edge breaks the event
> spec only when Ctrl+Tab.

I tested using this (terrible) example:

http://codepen.io/mozhacks/pen/mVVgMX?editors=001

In Edge on a Win10 box nothing was logged to the console and a new window was always opened. In Canary on OS X, same results. In Safari, same results.

In Firefox release 43 and Dev Edition 45 ( a non-e10s window ) the above code was able to suppress the key combo, and 'Ctrl|Cmd + n hit on Mac' is logged to the console.

Am I doing something wrong?
I tested with this:

https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html

1. Click Show Options
2. Check "keydown" below "preventDefault"
3. Set focus to the <input> and type Ctrl+N.

> http://codepen.io/mozhacks/pen/mVVgMX?editors=001

Looks like that this test tries to catch keypress event. However, IIRC, Ctrl+foo nor Cmd+foo won't cause keypress event on other browsers because UI Events said that keypress event may be fired when the key causes inputting text.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #10)
> I tested with this:
> 
> https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html
> 
> 1. Click Show Options
> 2. Check "keydown" below "preventDefault"
> 3. Set focus to the <input> and type Ctrl+N.

Confirmed! Thanks for clarifying. I'm altering my opinion on this then: Safari and Chrome prevent JS from overriding this keybinding. I am comfortable with breaking compat here as long as it is consistent between e10s and non-e10s modes. MS Edge does not, as you pointed out, but I still believe that allowing a website to bind Ctrl+n is bad.

What I'd  like to hear from an engineer is: what is the relative effort of fixing this bug ( allowing content code to suppress ctrl+n ) in e10s vs preventing websites from suppressing ctrl+n in non-e10s windows?
Flags: needinfo?(masayuki)
We have two options:

1. Completely not dispatching keyboard events which are reserved by chrome to web content (current e10s behavior)
2. Dispatching keyboard events which are reserved by chrome too, but XUL should ignore .defaultPrevented value if it's reserved.

So, #2 may cause "double action", but I think that this doesn't kill any a11y to either content or chrome. Therefore, I was thinking about the #2.

Smaug, how about you?
Flags: needinfo?(masayuki) → needinfo?(bugs)
So which all shortcuts would be "reserved" by chrome? Double action feels rather bad to me.
(Eventually we should probably expose some navigator.isUAReservedKeyEvent("char", "modifiers"); API to web pages.)

Do we know which all key+modifier combinations other browsers block to be dispatched to the web page?
Is it totally random? Ctrl+n apparently doesn't reach the page in Chrome but ctrl+c does.
Flags: needinfo?(bugs)

Updated

3 years ago
Flags: needinfo?(jmathies)
(In reply to Olli Pettay [:smaug] from comment #13)
> So which all shortcuts would be "reserved" by chrome? Double action feels
> rather bad to me.
> (Eventually we should probably expose some
> navigator.isUAReservedKeyEvent("char", "modifiers"); API to web pages.)
> 
> Do we know which all key+modifier combinations other browsers block to be
> dispatched to the web page?
> Is it totally random? Ctrl+n apparently doesn't reach the page in Chrome but
> ctrl+c does.

The answer is it's a bit of a mess across browsers:

OS X

Chrome & Safari

- N, W, Q, T
- F5 ( a11y )

Safari Only:

- R, L

Windows

Chrome & Edge
- Ctrl + F4 - closes window

Edge only
- Cltr+P triggers a double action

Chrome
- N, W, T

Based on this, as a user using key combinations, I should always be able to:

1. open new windows
2. close the current window
3. open a new tab
4. quit the browser

(optional, from Safari):

4. focus the location field
5. reload the page

This is a very short list of things. Does this help scope / estimate the technical effort?

Aside, Masayuki - thank you very much for linking to the w3c tool, it made this much easier.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #12)
> We have two options:
> 
> 1. Completely not dispatching keyboard events which are reserved by chrome
> to web content (current e10s behavior)
> 2. Dispatching keyboard events which are reserved by chrome too, but XUL
> should ignore .defaultPrevented value if it's reserved.
> 
> So, #2 may cause "double action", but I think that this doesn't kill any
> a11y to either content or chrome. Therefore, I was thinking about the #2.

Can you provide more info on how #1 would 'kill a11y'? Otherwise I prefer #1 because I do not think users will want double action. What I think will happen in the wild if we ship #2 is that web apps detect Firefox and listen for these key combinations. When they catch one they will attempt to prevent the default action, fail silently and the user will see a confusing double action.

I would much rather break the content code for this short list of use cases. Web Developers have already come to grips with not being able to bind these key combos in Safari and Chrome.
Flags: needinfo?(masayuki)
If we perform both actions, one is defined by web contents and the other is defined by Firefox, of course, the behavior may look ugly (depending on what *are* performed by them).

However, this means that user can access both actions. Therefore, even if a web application provide a function which is only available with keyboard shortcut which conflicts with Firefox's reserved shortcut key, user can use the function. Even when users want to operate Firefox with a shortcut key, that is also performed. So, they can just ignore the web application's behavior.

Anyway, I have no idea how we reserve keyboard shortcuts which are implemented with keydown event handler in JS. E.g., Ctrl(Cmd)+Tab. See bug 1008772.
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #16)
> If we perform both actions, one is defined by web contents and the other is
> defined by Firefox, of course, the behavior may look ugly (depending on what
> *are* performed by them).

You're saying that we should provide a bad user experience because it is 'more correct'. I disagree, there is no 'correct' here, just a set of opinions by different browser vendors. So far Mozilla's opinion has been to allow preventDefault to hijack a bindings like ctrl+n, perhaps to ensure compatibility with IE. Chrome and Safari have broken from this and have taken measures to protect certain bindings from being overridden by content, and I agree with them that this is the reasonable thing to do. Given chrome's market share, I also think most web developers are now used to this, that they can no longer count on being able to override ctrl+n.

...

> Anyway, I have no idea how we reserve keyboard shortcuts which are
> implemented with keydown event handler in JS. E.g., Ctrl(Cmd)+Tab. See bug
> 1008772.

If you don't know how to do this, who does? I would like to get at least an estimate of the engineering effort involved.
Flags: needinfo?(masayuki)
Also, I logged bug 1235074 to capture what I think are the correct set of requirement for this.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #17)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #16)
> > If we perform both actions, one is defined by web contents and the other is
> > defined by Firefox, of course, the behavior may look ugly (depending on what
> > *are* performed by them).
> 
> You're saying that we should provide a bad user experience because it is
> 'more correct'. I disagree, there is no 'correct' here, just a set of
> opinions by different browser vendors.

According to the UI Events spec, the correct behavior is:

1. Dispatch whole keyboard events
2. Any default actions can be prevented by web contents.

However, I agree with that this is not the best behavior for users. If you believe it should be "correct" in the spec, you should file a spec issue.

> So far Mozilla's opinion has been to
> allow preventDefault to hijack a bindings like ctrl+n, perhaps to ensure
> compatibility with IE. Chrome and Safari have broken from this and have
> taken measures to protect certain bindings from being overridden by content,
> and I agree with them that this is the reasonable thing to do.

Note that I don't disagree with breaking the spec for security. Therefore, I fixed bug 1008772.

> Given
> chrome's market share, I also think most web developers are now used to
> this, that they can no longer count on being able to override ctrl+n.

Yeah, but in fact, this bug was filed.

> > Anyway, I have no idea how we reserve keyboard shortcuts which are
> > implemented with keydown event handler in JS. E.g., Ctrl(Cmd)+Tab. See bug
> > 1008772.
> 
> If you don't know how to do this, who does? I would like to get at least an
> estimate of the engineering effort involved.

I think that such keyboard event handlers in chrome need a way to register key combinations as reserved and EventDispatcher shouldn't dispatch such keyboard events into content if we will decide not to dispatch reserved keyboard events to content.

But I still think that allowing double action is better approach because it allows users to access both functions (by content and chrome).
Flags: needinfo?(masayuki)
Flags: needinfo?(jgriffiths)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline until 1/6) from comment #19)
...
> But I still think that allowing double action is better approach because it
> allows users to access both functions (by content and chrome).

I disagree, if the user hits ctrl+n, a new window will be opened obscuring the window the user was on taking the user away from the web app they were in. When they close the new window and go back to the web app, it would be strange to also have had some action triggered. This is the same for ctrl+t.

Going back to my requests for more info:

1. what is involved in changing Firefox so that a short list of key bindings cannot be overridden?
2. if you're not comfortable (roughly) estimating 1, who can? Please needinfo them.

Again, what I need to understand is, what are the relative engineering costs associated with fixing this bug ( restoring Firefox's old behaviour ) vs what I'm proposing.
Flags: needinfo?(jgriffiths) → needinfo?(masayuki)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #20)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline until 1/6)
> from comment #19)
> ...
> > But I still think that allowing double action is better approach because it
> > allows users to access both functions (by content and chrome).
> 
> I disagree, if the user hits ctrl+n, a new window will be opened obscuring
> the window the user was on taking the user away from the web app they were
> in. When they close the new window and go back to the web app, it would be
> strange to also have had some action triggered. This is the same for ctrl+t.

I still think that "double action" approach is the only way to allow users to access such functions if they can be preformed only with keyboard shortcut.

I'd like to ask Smaug again, what do you think?

If you don't like "double action" approach too, we should file an issue to UI Events.

> Again, what I need to understand is, what are the relative engineering costs associated with
> fixing this bug ( restoring Firefox's old behaviour ) vs what I'm proposing.

Not sure because nobody can answer to my question in comment 16. How can we prevent to dispatch keyboard events which are reserved by chrome script's keydown event handlers like <tabbrowser>?

If we take current e10s behavior, the fix must be simple, but we need to fix above issue for consistency (at least in another bug). BTW, I don't understand the reason why bug 1074971 changed the behavior only in e10s mode...
Flags: needinfo?(bugs)
I think some shortcut keys should be reserved for UA only. The same way some of them are reserved for operating system. Double action feels really odd and error prone.
And I don't consider it a bug in UI events. UI events talks about the events web page get, but
if web page doesn't get such events at all, it is all about UA behavior.
Flags: needinfo?(bugs)
User Story: (updated)
(In reply to Olli Pettay [:smaug] from comment #22)
> I think some shortcut keys should be reserved for UA only. The same way some
> of them are reserved for operating system. Double action feels really odd
> and error prone.
> And I don't consider it a bug in UI events. UI events talks about the events
> web page get, but
> if web page doesn't get such events at all, it is all about UA behavior.

I disagree with that. According to current UI Event spec, "keydown" event is defined as "A user agent MUST dispatch this event when a key is pressed down". So, not dispatching key events which are reserved by browser's UI is actually breaking the spec. However, not dispatching keydown event may make sense if browser vendor doesn't like it (like this discussion's decision). So, the spec should be changed. I filed spec bug: https://github.com/w3c/uievents/issues/65
Flags: needinfo?(masayuki)
Oops, I still don't answer to the ni? from Jeff. I'll comment it. Sorry for the delay due to my vacation.
Flags: needinfo?(masayuki)
Jeff, this has been sitting in Masayuki's need-info queue for a while, do you want to make a call on what we do here?
Flags: needinfo?(jgriffiths)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #25)
> Jeff, this has been sitting in Masayuki's need-info queue for a while, do
> you want to make a call on what we do here?

Sorry, hadn't checked on it for a while. I want to move ahead with blocking web apps from preventing certain keybindings from being run, as per the user story. Jim, do we have someone besides Masayuki that can look at this?
Flags: needinfo?(jgriffiths) → needinfo?(jmathies)
Unfortunately, I need to work on improving "reserved" attribute handling in this Q1. They shouldn't work with web pages which prevents default of "keydown" or without ASCII capable keyboard layout.

I'm still waiting answer from UI Events WG (comment 23). Then, we'll get the rights not to dispatch keyboard events for reserved shortcut keys.

> Going back to my requests for more info:
> 
> 1. what is involved in changing Firefox so that a short list of key bindings cannot be overridden?

I'm not sure what's you asking me because I'm not sure what we should do here. Should we stop dispatching keyboard events on non-e10s? Should we drop Ctrl+N from reserved shortcut key list?

For the former, I wonder, doesn't it work if we set mOnlyChromeDispatch to true at setting mNoCrossProcessBoundaryForwarding?

For the latter, it should be easy to fix.

> 2. if you're not comfortable (roughly) estimating 1, who can? Please needinfo them.

If one of two scenarios above is what you want to fix it, I think anybody can fix this.

But if not, i.e., you want to fix all problems around "reserved" attribute, my work is a part of that. I need to update all platform's widget code for that. Therefore, I don't have much time in this Q1 anymore.

And also I mentioned above, I still don't have a good idea to fix some keyboard shortcuts which are implemented by keydown event handler, e.g., switching active tab. It might be possible if each event handler owner also listens keydown events in default event group at capturing phase and stop its propagation. But I'm not 100% sure and I don't like such redundant script. I guess that we should add a new API which can reserve specific shortcut keys from JS and then, the key combination shouldn't be dispatched as default event group.

> Again, what I need to understand is, what are the relative engineering costs associated with fixing this bug ( restoring Firefox's old behaviour ) vs what I'm proposing.

So, above my though could be answer to you? If not, please ni? me again. Thanks.
Flags: needinfo?(masayuki)
back to jeff...
Flags: needinfo?(jmathies) → needinfo?(jgriffiths)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #27)
> Unfortunately, I need to work on improving "reserved" attribute handling in
> this Q1. They shouldn't work with web pages which prevents default of
> "keydown" or without ASCII capable keyboard layout.
> 
> I'm still waiting answer from UI Events WG (comment 23). Then, we'll get the
> rights not to dispatch keyboard events for reserved shortcut keys.
> 
> > Going back to my requests for more info:
> > 
> > 1. what is involved in changing Firefox so that a short list of key bindings cannot be overridden?
> 
> I'm not sure what's you asking me because I'm not sure what we should do
> here. Should we stop dispatching keyboard events on non-e10s? Should we drop
> Ctrl+N from reserved shortcut key list?

We should stop dispatching keyboard events for ctrl+n.

> For the former, I wonder, doesn't it work if we set mOnlyChromeDispatch to
> true at setting mNoCrossProcessBoundaryForwarding?

I don't know, I just know that the *behaviour* I want is that JS can never override ctrl+n ( and t, and r and l )

...
> But if not, i.e., you want to fix all problems around "reserved" attribute,
> my work is a part of that. I need to update all platform's widget code for
> that. Therefore, I don't have much time in this Q1 anymore.

The reason why we're here is a) we're trying to prevent a situation where non-e10s behaves differently than e10s, which is a bad message to send to web developers and, b) having looked at this issue I think the right thing for Firefox users is to always reserve a relatively short list of bindings, as outlined in the user story.

e10s will ship to some release channel users in 46, so there is some urgency if we want to prevent confusion for those users.

> And also I mentioned above, I still don't have a good idea to fix some
> keyboard shortcuts which are implemented by keydown event handler, e.g.,
> switching active tab. It might be possible if each event handler owner also
> listens keydown events in default event group at capturing phase and stop
> its propagation. But I'm not 100% sure and I don't like such redundant
> script. I guess that we should add a new API which can reserve specific
> shortcut keys from JS and then, the key combination shouldn't be dispatched
> as default event group.

The new api seems like a good approach to me?

> > Again, what I need to understand is, what are the relative engineering costs associated with fixing this bug ( restoring Firefox's old behaviour ) vs what I'm proposing.
> 
> So, above my though could be answer to you? If not, please ni? me again.
> Thanks.

Jim - when I last talked to you about this I think you indicated we could get someone from e10s looking into a more expedient fix?
Flags: needinfo?(jgriffiths) → needinfo?(jmathies)
Somebody from the e10s team might be able to pick this up. Removing ni so this falls back into e10s triage.
Flags: needinfo?(jmathies)
Assignee: jgriffiths → mrbkap
> a) we're trying to prevent a situation where non-e10s behaves differently than e10s, which is a bad
> message to send to web developers

Absolutely, therefore, I don't understand why bug 1074971 touched only e10s case...

> b) having looked at this issue I think the right thing for Firefox users is to always reserve a
> relatively short list of bindings, as outlined in the user story.

I agree if UI Events WG won't disagree that. But they must agree because other browsers do that.


I think that the new API issue should be filed as a new bug. I think that it's not more important than this bug.

This bug should fix the fix of bug 1074971 non-e10s aware. So, I change the summary of this bug.

I wonder, if we will stop dispatching reserved key events to web contents, we can fix a lot of part of bug 78414. That's my work of this Q1. I'll check this today.
Summary: [e10s] Default shortcuts are not supressed when overridden by web-application → [non-e10s] Keyboard events which are reserved shortcuts in chrome shouldn't be fired on web content like e10s mode
Okay, I investigated something which I wondered about.

1. Cannot fix this bug with setting mNoContentDispatch/mOnlyChromeDispatch because nsXBLWindowKeyHandler::HandleEventOnCapture() is in system group. So, already dispatched to content. So, somebody needs to redesign to handle keyboard event. I think that it should be registered to nsIPresShell directly and before dispatching keyboard events, nsXBLWindowKeyHandler needs to initialize keyboard event for marking the event as reserved.

2. Even if this is fixed, the shortcut key event handler will be prevented by a call of preventDefault() of preceding keydown event. This is what currently I'm working on. If reserved a keypress, it should be performed at keydown. However, current keydown event doesn't have alternative char code values. So, keydown event isn't keyhell-aware. (i.e., having a lot of problem for i18n.)

3. keydown events are not fired even on <body> while plugin has focus. So, this is not related to bug 78414. I.e., my previous comment is wrong.
FYI: currently, event handlers of nsXBLWindowKeyHandler are registered by nsXBLService::AttachGlobalKeyHandler().
Duplicate of this bug: 1101975
Assignee: mrbkap → mconley
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1173061#c5

I don't think this needs to block, FWIW. Reserved shortcuts are a new e10s feature, so this is not something regressing from non-e10s.  And the shortcut works, it's just a matter that the page will still get them.
(In reply to :Felipe Gomes (needinfo me!) from comment #35)
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1173061#c5
> 
> I don't think this needs to block, FWIW. Reserved shortcuts are a new e10s
> feature, so this is not something regressing from non-e10s.  And the
> shortcut works, it's just a matter that the page will still get them.

Such different behavior requires to check the user's setting if a user files a bug. And I believe the fix of bug 1074971 is just incomplete. Bug 1074971 should've check non-e10s cases and keydown.preventDefault() cases. If it supported non-e10s case, keydown.preventDefault() issue must have been found before requesting review since it's possible to write automated tests.

I'm currently rewriting widget code for making keydown events usable in nsXBLWindowKeyHandler...
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #36)
> (In reply to :Felipe Gomes (needinfo me!) from comment #35)
> > Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1173061#c5
> > 
> > I don't think this needs to block, FWIW. Reserved shortcuts are a new e10s
> > feature, so this is not something regressing from non-e10s.  And the
> > shortcut works, it's just a matter that the page will still get them.
> 
> Such different behavior requires to check the user's setting if a user files
> a bug. And I believe the fix of bug 1074971 is just incomplete. Bug 1074971
> should've check non-e10s cases and keydown.preventDefault() cases. If it
> supported non-e10s case, keydown.preventDefault() issue must have been found
> before requesting review since it's possible to write automated tests.
> 
> I'm currently rewriting widget code for making keydown events usable in
> nsXBLWindowKeyHandler...

Hey :masayuki, in which bug are you doing this work? I'd like to make sure we're not either duplicating, or working at cross-purposes.

I've been assigned this bug in order to try to make non-e10s behave similarly to e10s to reduce confusion in how keyboard shortcuts are handled (or not handled) by web content.

My initial plan is to use the infrastructure already laid out with the mNoCrossProcessBoundaryForwarding marker on the event, but try to make it so that for events with that marker, we also don't allow them to cross the "content boundary" (I'll probably rename the marker mNoCrossContentBoundary).

Anybody see any problems with that approach?

Comment 38

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #37)
> My initial plan is to use the infrastructure already laid out with the
> mNoCrossProcessBoundaryForwarding marker on the event, but try to make it so
> that for events with that marker, we also don't allow them to cross the
> "content boundary" (I'll probably rename the marker mNoCrossContentBoundary).
> 
> Anybody see any problems with that approach?

See bug 1052569 comment 27 and further.

Essentially, you need to make a decision at keydown time if you don't want content to have the event, because otherwise content will call preventDefault() at keydown time, and you won't have a keypress event. But keydown events don't currently have the requisite information to know whether or not they match one of the reserved shortcuts. I believe this is what Masayuki meant when they wrote:

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #36)
> I'm currently rewriting widget code for making keydown events usable in
> nsXBLWindowKeyHandler...
No longer blocks: 1074971
Depends on: 1154183, 1074971
> Hey :masayuki, in which bug are you doing this work? I'd like to make sure we're not either
> duplicating, or working at cross-purposes.

I'm currently working on the files under widget/.
Bug 1137572 (TextEventDispatcher), bug 1137561 (Window), bug 1137563 (Mac), bug 1137565 (Linux/GTK). Perhaps, I can request reviews at late next week. (Note that these bugs won't change the behavior, they are just (big) preparation.)
Having analyzed the work that masayuki is doing, I'm concerned about this bug being an M8, since I don't think the work that masayuki is doing is going to be safe for uplift given how late in the cycle it is.

I'm looking at alternatives, but going to re-nom for now to discuss this in triage today.
Priority: -- → P1
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> Having analyzed the work that masayuki is doing, I'm concerned about this
> bug being an M8, since I don't think the work that masayuki is doing is
> going to be safe for uplift given how late in the cycle it is.

Yes, they become very big change. So, not possible to uplift.

> I'm looking at alternatives, but going to re-nom for now to discuss this in
> triage today.

I think that you (this bug) should focus to fix the difference of the behavior in e10s vs. non-e10s. The other issues cannot be fixed with small patches which can uplift.
Now, this bug blocks my work on m-c and I found a _hacky_ way to fix this bug (it's safe to uplift). Can I take this? Or do you have patches already? If the latter, please post patches ASAP.
Flags: needinfo?(mconley)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #43)
> Now, this bug blocks my work on m-c and I found a _hacky_ way to fix this
> bug (it's safe to uplift). Can I take this? Or do you have patches already?
> If the latter, please post patches ASAP.

You can definitely take this.
Assignee: mconley → masayuki
Flags: needinfo?(mconley)
Thank you.

I'll request to review to smaug after he will be back (next week).
Status: REOPENED → ASSIGNED
I found a bug on OS X. Cmd+Q isn't reserved:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc?rev=4247dcdafb2f&mark=445-445#443

Firefox UI team should fix this.
Flags: needinfo?(jgriffiths)
browser_tabCtrl.js's intermittent failures in e10s mode (bug 1211273, bug 1233956, bug 1233956) could be fixed by this bug because the test isn't async-aware but this bug will fix it for fixing new orange.
Blocks: 1211273, 1233956
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #52)
> I found a bug on OS X. Cmd+Q isn't reserved:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> sets.inc?rev=4247dcdafb2f&mark=445-445#443
> 
> Firefox UI team should fix this.

I logged bug 1253284 to address this and set it to block this bug.
Depends on: 1253284
Flags: needinfo?(jgriffiths)
UL <key> elements listen to keyboard events on its XUL document node. Therefore, calling stopImmediatePropagation() at capture phase in the default event group can stop dispatching reserved keyboard events on web contents.

This is a little bit hacky but it's the safest approach to fix this bug for now.
# Actually, chrome window can listen the reserved keyboard events even in the default event group.
Attachment #8727280 - Flags: review?(bugs)
I think that adding a lot of keyboard event listeners for nsXBLWindowKeyHandler should be done by nsXBLWindowKeyHandler itself.
Attachment #8727284 - Flags: review?(bugs)
Fixing a lot of oranges which are caused by stopping dispatching reserved keypress events in the default event group.
Attachment #8727285 - Flags: review?(bugs)
Now, browser_ctrlTab.js becomes always orange in e10s mode but I'm not sure the reason...

Anyway, the tests are not aware of e10s and there are some random oranges. So, I should rewrite this test with generator.
Attachment #8727286 - Flags: review?(bugs)
Moving the test changes into part.2 because this patch doesn't change actual behavior.
Attachment #8727280 - Attachment is obsolete: true
Attachment #8727280 - Flags: review?(bugs)
Attachment #8727296 - Flags: review?(bugs)
Smaug: I'd like to land the patches for this bug *before* bug 1137572 and related bugs since it's safer way to uplift the patches for this bug.
Masayuki, I tried these patches, and they do not fix the duplicate bug 1101975. Should they?
Flags: needinfo?(masayuki)
(In reply to Botond Ballo [:botond] from comment #65)
> Masayuki, I tried these patches, and they do not fix the duplicate bug
> 1101975. Should they?

Yeah, this is a behavior difference between e10s and non-e10s. This is a remaining fix of bug 1074971 in non-e10s mode. On the other hand, bug 1101975 is a bug of access key handling in content process in e10s mode. So, it's really different bug.
Flags: needinfo?(masayuki)
Smaug: Could you give higher priority to review this bug? For consistency behavior between e10s and non-e10s on 46, we need to uplift them to Beta. So, we need to fix this bug ASAP.
Flags: needinfo?(bugs)
uh, beta. feels super scary. But looking. Sorry, todo list has been rather long.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #67)
> Smaug: Could you give higher priority to review this bug? For consistency
> behavior between e10s and non-e10s on 46, we need to uplift them to Beta.
> So, we need to fix this bug ASAP.

Just needinfo'ing canuckistani to confirm this. I'd hate to uplift something dangerous if we feel we can let the difference between the two states exist for one release.
Flags: needinfo?(jgriffiths)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #69)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #67)
> > Smaug: Could you give higher priority to review this bug? For consistency
> > behavior between e10s and non-e10s on 46, we need to uplift them to Beta.
> > So, we need to fix this bug ASAP.
> 
> Just needinfo'ing canuckistani to confirm this. I'd hate to uplift something
> dangerous if we feel we can let the difference between the two states exist
> for one release.

We're not releasing 46 with e10s enabled, so this can slip to 47.
Flags: needinfo?(jgriffiths)
Comment on attachment 8727296 [details] [diff] [review]
part.1 nsXBLWincowKeyHandler mark WidgetKeyboardEvent if it's reserved by chrome before the event is dispatched into the content

> nsXBLWindowKeyHandler::HandleEvent(nsIDOMEvent* aEvent)
> {
>   nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aEvent));
>   NS_ENSURE_TRUE(keyEvent, NS_ERROR_INVALID_ARG);
> 
>   uint16_t eventPhase;
>   aEvent->GetEventPhase(&eventPhase);
>   if (eventPhase == nsIDOMEvent::CAPTURING_PHASE) {
>-    HandleEventOnCapture(keyEvent);
>+    if (aEvent->WidgetEventPtr()->mFlags.mInSystemGroup) {
>+      HandleEventOnCaptureInSystemEventGroup(keyEvent);
>+    } else {
>+      HandleEventOnCaptureInDefaultEventGroup(keyEvent);
>+    }
oh, took a bit time to understand this, but ok, we add key event listener to different phases and different groups.
Flags: needinfo?(bugs)
Attachment #8727296 - Flags: review?(bugs) → review+
Comment on attachment 8727297 [details] [diff] [review]
part.2 When a keyboard event is reserved by chrome, it shouldn't be dispatched into web contents

But this may break key event handling also in chrome, no? I mean the case when there are other listeners in chrome.

WidgetEvent::mNoContentDispatch is somewhat related to this, but that is unfortunately an ancient and broken feature which shouldn't be change in this bug.

But we need something else here. Couldn't this change break some addons for example.
I don't really have a good suggestion for this though :/
Perhaps in event target chain mark "chrome event handler" (the first event handler above content) with some flag and then in the event have a flag which allows dispatching only above that level
Attachment #8727297 - Flags: review?(bugs) → review-
Comment on attachment 8727286 [details] [diff] [review]
part.5 Rewrite browser_ctrlTab.js with generators

I truly hate generators since they are an easy way to make spaghetti code.
(but that is very personal view and I know many other people really like generators and promises and stuff)

But fine. rs+
Attachment #8727286 - Flags: review?(bugs) → review+
Comment on attachment 8727285 [details] [diff] [review]
part.4 Use system event listener in some modules and tests which need to listen reserved key events

This is very worrisome. Could break addons.
I think we need to keep the normal event dispatch in chrome, which is why I r-'ed part 2.
I assume if part2 was fixed differently, we would need a lot fewer changes here, right?


(If needed, we could even go as far as have even system event group handling for content - so disable only default group there. But perhaps that isn't needed.)
Attachment #8727285 - Flags: review?(bugs) → review-
Comment on attachment 8727284 [details] [diff] [review]
part.3 Installing and removing keyboard event listeners of nsXBLWindowKeyHandler should be done by the class itself

>+nsXBLWindowKeyHandler::InstallKeyboardEventListenersTo(
>+                         EventListenerManager* aEventListenerManager)
>+{
>+  // Handle keyboard events in bubbling phase of the system event group.
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keydown"),
>+                           TrustedEventsAtSystemGroupBubble());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keyup"),
>+                           TrustedEventsAtSystemGroupBubble());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keypress"),
>+                           TrustedEventsAtSystemGroupBubble());
>+
>+  // For marking each keyboard event as if it's reserved by chrome,
>+  // nsXBLWindowKeyHandlers need to listen each keyboard events before
>+  // web contents.
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keydown"),
>+                           TrustedEventsAtCapture());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keyup"),
>+                           TrustedEventsAtCapture());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keypress"),
>+                           TrustedEventsAtCapture());
>+
>+  // For reducing the IPC cost, preventing to dispatch reserved keyboard
>+  // events into the content process.
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keydown"),
>+                           TrustedEventsAtSystemGroupCapture());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keyup"),
>+                           TrustedEventsAtSystemGroupCapture());
>+  aEventListenerManager->AddEventListenerByType(
>+                           this, NS_LITERAL_STRING("keypress"),
>+                           TrustedEventsAtSystemGroupCapture());
>+}
Might even make sense to move the system group bubble listener as last, to ease readability. Those listeners are after all called last.
Attachment #8727284 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8727297 [details] [diff] [review]
> part.2 When a keyboard event is reserved by chrome, it shouldn't be
> dispatched into web contents
> 
> But this may break key event handling also in chrome, no? I mean the case
> when there are other listeners in chrome.

Hmm, yes.

> WidgetEvent::mNoContentDispatch is somewhat related to this, but that is
> unfortunately an ancient and broken feature which shouldn't be change in
> this bug.

Okay.

> But we need something else here. Couldn't this change break some addons for
> example.
> I don't really have a good suggestion for this though :/

Yeah, if add-ons listen keypress events for web contents, reserved key combinations won't be fired in current e10s mode anyway. But it's really our agreement of our discussion in this bug.

So, breaking some addons (but perhaps, the number of them is not so many) should be accepted.

> Perhaps in event target chain mark "chrome event handler" (the first event
> handler above content) with some flag and then in the event have a flag
> which allows dispatching only above that level

Okay, I think that I should add mOnlyChromeDispatchInDefaultEventGroup and EventDispatcher should check the target in the loop.
I'm not sure if EventTargetChainItem::IsCurrentTargetChrome() is correct for all cases. However, mOnlyChromeDispatchInDefaultEventGroup may be true only when the event is a keyboard event. So, it must be run in the main thread and EventTarget must be one of DOM Window, DOM Document node or DOM Element node, it must be safe and enough for now.
Attachment #8727297 - Attachment is obsolete: true
Attachment #8729966 - Flags: review?(bugs)
Indeed, as you guessed, the new part.2 doesn't require additional changes of existing event handlers (as far as tested by the automated tests).

Let's improve test_keycodes.xul to check keydown and keypress propagation.
Attachment #8727285 - Attachment is obsolete: true
Attachment #8729967 - Flags: review?(bugs)
Comment on attachment 8729966 [details] [diff] [review]
part.2 When a keyboard event is reserved by chrome, it should be fired only on chrome


>+  bool IsCurrentTargetChrome()
Could we have a helper method  IsChromeEventTarget(EventTarget* aTarget)
and then reuse that in ::Dispatch where we have "if (aEvent->mFlags.mOnlyChromeDispatch) {" 
and in this IsCurrentTargetChrome()



>+  // If the key combination is reserved by chrome, we shouldn't expose the
>+  // keyboard event to web contents because such keyboard events shouldn't be
>+  // cancelable.  So, it's not good behavior to fire keyboard events but
>+  // to ignore the defaultPrevented attribute value in chrome.
>+  if (widgetKeyboardEvent->mIsReservedByChrome) {
>+    widgetKeyboardEvent->mFlags.mOnlyChromeDispatchInDefaultEventGroup = true;
>+  }
So why we need mIsReservedByChrome if we have also mOnlyChromeDispatchInDefaultEventGroup (which I propose will be renamed to mOnlySystemGroupDispatchInContent)


>   // If mOnlyChromeDispatch is true, the event is dispatched to only chrome.
>+  // XXX This is an ancient and broken feature, don't use this for new bug
>+  //     as far as possible.
>   bool    mOnlyChromeDispatch : 1;
mOnlyChromeDispatch is not broken. mNoContentDispatch is.


>+  // If mOnlyChromeDispatchInDefaultEventGroup is true, the event is fired on
>+  // only chrome in the default event group and both chrome and content in
>+  // the system event group.
This is now backwards.
And the variable name too.
Call the variable.
mOnlySystemGroupDispatchInContent and then the comment could be:
...is true, event listeners added to the default group for non-chrome EventTarget won't be called.

And the comment should also say that the flag should be set only on relatively rarely dispatched events, since it slows down event dispatch.
(so, not to be used with mouse or touch events for example)



>+  function testReservedCommand(aEvent, aKeyElementId)
>+  {
>+    var testName = "testReservedCommand: " + eventToString(aEvent) + " ";
>+    var keyElement = document.getElementById(aKeyElementId);
>+    var commandElement = document.getElementById(keyElement.getAttribute("command"));
>+    commandElement.activeCount = 0;
>+
>+    function onKeypressInDefaultEventGroup(aDOMEvent)
>+    {
>+      ok(false, testName + "keypress event shouldn't be fired in the default event group");
>+    }
>+    function onKeypressInSystemEventGroup(aDOMEvent)
>+    {
>+      ok(true, testName + "keypress event should be fired in the system event group");
>+    }
>+    // XXX Cannot test with listeners of window since this is a chrome window
>+    //     and nsXBLWindowKeyHandlers listens chrome document's events.
>+    //     Therefore, window.addEventListener() wins against
>+    //     nsXBLWindowKeyHandler's event listener...  Of course, content window
>+    //     never wins, though.
Ok, a bit hackish test then. Would be nice to test this all in non-chrome window.
But fine. And I see there is another patch dealing with tests too.


I think I'd like to see a new patch with those small issues fixed.
Attachment #8729966 - Flags: review?(bugs) → review+
Attachment #8729967 - Flags: review?(bugs) → review+
Okay, replacing WidgetKeyboardEvent::mIsReservedByChrome with WidgetEvent.mFlags::mOnlySystemGroupDispatchInContent.

It seems that we can remove mNoCrossProcessBoundaryForwarding now. But I'm not 100% sure. Anyway, it's not in the scope of this bug.
Attachment #8727296 - Attachment is obsolete: true
Attachment #8730580 - Flags: review?(bugs)
Attachment #8730580 - Flags: review?(bugs) → review+
Comment on attachment 8730581 [details] [diff] [review]
part.2 When an event is reserved by chrome, it should be fired only on chrome

/me likes our setup for DOM event dispatch. So flexible, yet fast.
Attachment #8730581 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72453a034c39e94ed561851334c122bc26a3ffa
Bug 1203059 part.1 nsXBLWincowKeyHandler mark WidgetEvent::mFlags if it's reserved by chrome before the event is dispatched into the content r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/4838a51a99b7717e0aa6464e83e100f1dc7311a9
Bug 1203059 part.2 When an event is reserved by chrome, it should be fired only on chrome r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/37fd93e4e4dbbf0cf53eb18b77ae72d9586229c0
Bug 1203059 part.3 Installing and removing keyboard event listeners of nsXBLWindowKeyHandler should be done by the class itself r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/0469e84deb514394e533cc96e2ddd8dff1bd7fc9
Bug 1203059 part.4 Update test_keycodes.xul for the new behavior r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/facab7643e3f6b2fd5cc20524f2dbbe35c3a6071
Bug 1203059 part.5 Rewrite browser_ctrlTab.js with generators r=smaug
Comment on attachment 8730580 [details] [diff] [review]
part.1 nsXBLWincowKeyHandler mark WidgetEvent::mFlags if it's reserved by chrome before the event is dispatched into the content

Approval Request Comment
[Feature/regressing bug #]: Bug 1074971
[User impact if declined]: Bug 1074971 stopped dispatching keyboard events which are reserved by chrome as important shortcut key *only* in e10s mode. These patches makes non-e10s mode same behavior. So, starting 46, e10s mode will be shipped, we should fix the difference of both mode on web pages as far as possible.
[Describe test coverage new/current, TreeHerder]: Landed on m-c and has automated tests for the new behavior.
[Risks and why]: Mid. This may cause breaking some addons if they listen to keyboard events at node in web contents with the default event group. However, such addon should be rare because the key combinations which will be not able to listen in web contents are only Ctrl+W, Ctrl+N, Ctrl+T. Even if there are such addons, they can listen the keyboard events of them with the event listeners which are added into the system event group (and they should do so, in strictly speaking, even without this change).
[String/UUID change made/needed]: Nothing.
Attachment #8730580 - Flags: approval-mozilla-aurora?
Comment on attachment 8730581 [details] [diff] [review]
part.2 When an event is reserved by chrome, it should be fired only on chrome

Approval Request Comment: See comment 93.
Attachment #8730581 - Flags: approval-mozilla-aurora?
Comment on attachment 8729968 [details] [diff] [review]
part.3 Installing and removing keyboard event listeners of nsXBLWindowKeyHandler should be done by the class itself (r=smaug)

Approval Request Comment: See comment 93.
Attachment #8729968 - Flags: approval-mozilla-beta?
Attachment #8729968 - Flags: approval-mozilla-aurora?
Comment on attachment 8729967 [details] [diff] [review]
part.4 Update test_keycodes.xul for the new behavior

Approval Request Comment: See comment 93.
Attachment #8729967 - Flags: approval-mozilla-beta?
Attachment #8729967 - Flags: approval-mozilla-aurora?
Comment on attachment 8729969 [details] [diff] [review]
part.5 Rewrite browser_ctrlTab.js with generators (r=smaug)

Approval Request Comment: See comment 93.
Attachment #8729969 - Flags: approval-mozilla-beta?
Attachment #8729969 - Flags: approval-mozilla-aurora?
FYI: The new behavior (not dispatching reserved keyboard events into web contents) is similar to the other browsers.
It is unclear to me why we'd take this kind of some what risky change to beta.
(In reply to Olli Pettay [:smaug] from comment #101)
> It is unclear to me why we'd take this kind of some what risky change to
> beta.

I concur. If the concern is shipping with e10s, please see comment 70. Requesting to Aurora is probably okay, but I think we should avoid uplifting this to Beta.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #102)
> (In reply to Olli Pettay [:smaug] from comment #101)
> > It is unclear to me why we'd take this kind of some what risky change to
> > beta.
> 
> I concur. If the concern is shipping with e10s, please see comment 70.
> Requesting to Aurora is probably okay, but I think we should avoid uplifting
> this to Beta.

If e10s won't be shipped for *any* users at 46, I agree with not uplifting it to Beta. But as far as I know, e10s will be enabled at 46 for some users who don't use any addons. My understanding is wrong?
I don't know the current plan. mconley?
Flags: needinfo?(mconley)
I believe that if these patches won't be landed onto beta even if 46 will ship e10s even for a few users, we should back out the patches for bug 1074971. Then, we can keep consistency between non-e10s mode and e10s mode.
(In reply to Olli Pettay [:smaug] from comment #104)
> I don't know the current plan. mconley?

We're running beta experiments in 46 now. No release users will get e10s in 46. 47 is a possible candidate for exposing release user to e10s. Depends on the numbers we get back from beta 46 and 47.
Flags: needinfo?(mconley)
Comment on attachment 8732026 [details] [diff] [review]
part.1 nsXBLWincowKeyHandler mark WidgetEvent::mFlags if it's reserved by chrome before the event is dispatched into the content (r=smaug, for Beta)

e10s not going to release, so I don't think this is worth the risk to uplift to beta.
Attachment #8732026 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8732029 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8729967 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8729968 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8729969 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Jim, based on the latest e10s release roadmap, we may not be enabling e10s on 47 either. Do you think I should be uplifting this change to Aurora 47 or let it ride the trains? Appreciate your help here.
Flags: needinfo?(jmathies)
Duplicate of this bug: 1173061
(In reply to Ritu Kothari (:ritu) from comment #108)
> Jim, based on the latest e10s release roadmap, we may not be enabling e10s
> on 47 either. Do you think I should be uplifting this change to Aurora 47 or
> let it ride the trains? Appreciate your help here.

we'll be running another experiment at least in 47, so it would be good to get testing coverage. I'm comfortable letting major changes like this ride the trains though. this looks pretty invasive. Jeff, any opinion here?
Flags: needinfo?(jmathies) → needinfo?(jgriffiths)
Correction, this is a fix for a non-e10s regression caused by e10s code that landed. So we need to make make a call on whether we can like with that for non-e10s users.
s/like/live
(In reply to Jim Mathies [:jimm] from comment #110)
> (In reply to Ritu Kothari (:ritu) from comment #108)
> > Jim, based on the latest e10s release roadmap, we may not be enabling e10s
> > on 47 either. Do you think I should be uplifting this change to Aurora 47 or
> > let it ride the trains? Appreciate your help here.
> 
> we'll be running another experiment at least in 47, so it would be good to
> get testing coverage. I'm comfortable letting major changes like this ride
> the trains though. this looks pretty invasive. Jeff, any opinion here?

Sounds reasonable to let it ride the trains.
Flags: needinfo?(jgriffiths)
Comment on attachment 8729967 [details] [diff] [review]
part.4 Update test_keycodes.xul for the new behavior

This is a pretty big code change and the risk was assessed by Jim and Jeff. They suggested letting it ride the trains.
Attachment #8729967 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8729968 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8729969 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8730580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8730581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

3 years ago
Blocks: 1101975

Comment 115

3 years ago
Is this bug a WONTFIX on Linux? If so it should be marked as such. (platform OSX, Windows)

Is there really nothing that can be done to fix this on Linux?
I feel like at the very least a Linux specific bug should be made so it can be fixed later.

It really sucks if Linux users have to endure the 15-year-old Bug 78414 even longer, especially now that it's fixed everywhere else.
(In reply to Alan from comment #115)
> Is this bug a WONTFIX on Linux?

No, this is fixed on all platforms.
We cannot fix only bug 1257761 since we cannot interrupt any input events before plugin handles them only on Linux. That's the platform's event model issue.
See Also: → 1291706
isn't this supposed to work also for F5/ctrl-F5?
(In reply to eyal gruss (eyaler) from comment #118)
> isn't this supposed to work also for F5/ctrl-F5?

It depends on chrome. This bug is for implementing the new KeyboardEvent behavior. So, you're wrong place to ask that. If you have some trouble with a specific key combination, please file bugs for each key combination and don't put comments here.

Updated

3 years ago
Depends on: 1301476

Updated

2 years ago
Depends on: 1319643
See Also: → 1518962
You need to log in before you can comment on or make changes to this bug.