Closed
Bug 1203059
Opened 9 years ago
Closed 8 years ago
[non-e10s] Keyboard events which are reserved shortcuts in chrome shouldn't be fired on web content like e10s mode
Categories
(Firefox :: Keyboard Navigation, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: linuxhippy, Assigned: masayuki)
References
(Blocks 2 open bugs)
Details
(Keywords: regression)
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 files, 8 obsolete files)
40.00 KB,
patch
|
smaug
:
review+
ritu
:
approval-mozilla-aurora-
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
10.44 KB,
patch
|
ritu
:
approval-mozilla-aurora-
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
6.49 KB,
patch
|
ritu
:
approval-mozilla-aurora-
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
10.76 KB,
patch
|
smaug
:
review+
ritu
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
12.22 KB,
patch
|
smaug
:
review+
ritu
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
10.88 KB,
patch
|
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
12.32 KB,
patch
|
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Just double-checked, the issue is clearly caused by "multi-process nightly". When I disable this option, FireFox behaves as expected.
Updated•8 years ago
|
Summary: Default shortcuts are not supressed when overridden by web-application → [e10s] Default shortcuts are not supressed when overridden by web-application
Updated•8 years ago
|
Flags: needinfo?(jmathies)
Reporter | ||
Comment 2•8 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.
Updated•8 years ago
|
Assignee: nobody → jgriffiths
Comment 3•8 years ago
|
||
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
Closed: 8 years ago
Flags: needinfo?(miket)
Resolution: --- → WONTFIX
Comment 4•8 years ago
|
||
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 → ---
Assignee | ||
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Comment 9•8 years ago
|
||
(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?
Assignee | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(jmathies)
Updated•8 years ago
|
tracking-e10s:
--- → m8+
Comment 14•8 years ago
|
||
(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.
Comment 15•8 years ago
|
||
(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.
Updated•8 years ago
|
Flags: needinfo?(masayuki)
Assignee | ||
Comment 16•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
(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)
Comment 18•8 years ago
|
||
Also, I logged bug 1235074 to capture what I think are the correct set of requirement for this.
Assignee | ||
Comment 19•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(jgriffiths)
Comment 20•8 years ago
|
||
(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)
Assignee | ||
Comment 21•8 years ago
|
||
(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)
Comment 22•8 years ago
|
||
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)
Updated•8 years ago
|
Updated•8 years ago
|
User Story: (updated)
Assignee | ||
Comment 23•8 years ago
|
||
(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)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Comment 25•8 years ago
|
||
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)
Comment 26•8 years ago
|
||
(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)
Assignee | ||
Comment 27•8 years ago
|
||
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)
Comment 29•8 years ago
|
||
(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)
Comment 30•8 years ago
|
||
Somebody from the e10s team might be able to pick this up. Removing ni so this falls back into e10s triage.
Flags: needinfo?(jmathies)
Updated•8 years ago
|
Assignee: jgriffiths → mrbkap
Assignee | ||
Comment 31•8 years ago
|
||
> 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
Assignee | ||
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
FYI: currently, event handlers of nsXBLWindowKeyHandler are registered by nsXBLService::AttachGlobalKeyHandler().
Updated•8 years ago
|
Assignee: mrbkap → mconley
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
(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...
Comment 37•8 years ago
|
||
(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•8 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...
Updated•8 years ago
|
Assignee | ||
Comment 39•8 years ago
|
||
> 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.)
Comment 40•8 years ago
|
||
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.
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 41•8 years ago
|
||
(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.
Assignee | ||
Comment 42•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39779e3c929
Assignee | ||
Comment 43•8 years ago
|
||
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)
Assignee | ||
Comment 44•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0744191ba8b8
Comment 45•8 years ago
|
||
(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)
Assignee | ||
Comment 46•8 years ago
|
||
Thank you. I'll request to review to smaug after he will be back (next week).
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 47•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb83123f8347
Assignee | ||
Comment 48•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70fa9e558fd2
Assignee | ||
Comment 49•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea403629fd70
Assignee | ||
Comment 50•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a2cc61a78cb
Assignee | ||
Comment 51•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bbc45313918
Assignee | ||
Comment 52•8 years ago
|
||
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)
Assignee | ||
Comment 53•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef93630c6ce
Assignee | ||
Comment 54•8 years ago
|
||
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.
Comment 55•8 years ago
|
||
(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)
Assignee | ||
Comment 56•8 years ago
|
||
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)
Assignee | ||
Comment 57•8 years ago
|
||
This patch actually changes the behavior.
Attachment #8727282 -
Flags: review?(bugs)
Assignee | ||
Comment 58•8 years ago
|
||
I think that adding a lot of keyboard event listeners for nsXBLWindowKeyHandler should be done by nsXBLWindowKeyHandler itself.
Attachment #8727284 -
Flags: review?(bugs)
Assignee | ||
Comment 59•8 years ago
|
||
Fixing a lot of oranges which are caused by stopping dispatching reserved keypress events in the default event group.
Attachment #8727285 -
Flags: review?(bugs)
Assignee | ||
Comment 60•8 years ago
|
||
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)
Assignee | ||
Comment 61•8 years ago
|
||
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)
Assignee | ||
Comment 62•8 years ago
|
||
Attachment #8727282 -
Attachment is obsolete: true
Attachment #8727282 -
Flags: review?(bugs)
Attachment #8727297 -
Flags: review?(bugs)
Assignee | ||
Comment 63•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9eb603233ed
Assignee | ||
Comment 64•8 years ago
|
||
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.
Comment 65•8 years ago
|
||
Masayuki, I tried these patches, and they do not fix the duplicate bug 1101975. Should they?
Flags: needinfo?(masayuki)
Assignee | ||
Comment 66•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 67•8 years ago
|
||
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)
Comment 68•8 years ago
|
||
uh, beta. feels super scary. But looking. Sorry, todo list has been rather long.
Comment 69•8 years ago
|
||
(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)
Comment 70•8 years ago
|
||
(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 71•8 years ago
|
||
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 72•8 years ago
|
||
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 73•8 years ago
|
||
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 74•8 years ago
|
||
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 75•8 years ago
|
||
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+
Assignee | ||
Comment 76•8 years ago
|
||
(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.
Assignee | ||
Comment 77•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=482435b15e01
Assignee | ||
Comment 78•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f9c0fd0acc
Assignee | ||
Comment 79•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e651a8052b
Assignee | ||
Comment 80•8 years ago
|
||
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)
Assignee | ||
Comment 81•8 years ago
|
||
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)
Assignee | ||
Comment 82•8 years ago
|
||
Attachment #8727284 -
Attachment is obsolete: true
Assignee | ||
Comment 83•8 years ago
|
||
Attachment #8727286 -
Attachment is obsolete: true
Comment 84•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8729967 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 85•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=921355a698c6
Assignee | ||
Comment 86•8 years ago
|
||
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)
Assignee | ||
Comment 87•8 years ago
|
||
Attachment #8729966 -
Attachment is obsolete: true
Attachment #8730581 -
Flags: review?(bugs)
Updated•8 years ago
|
Attachment #8730580 -
Flags: review?(bugs) → review+
Comment 88•8 years ago
|
||
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+
Assignee | ||
Comment 89•8 years ago
|
||
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
Assignee | ||
Comment 90•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=980216610c4c
Assignee | ||
Comment 91•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e169a3eec70e
Comment 92•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f72453a034c3 https://hg.mozilla.org/mozilla-central/rev/4838a51a99b7 https://hg.mozilla.org/mozilla-central/rev/37fd93e4e4db https://hg.mozilla.org/mozilla-central/rev/0469e84deb51 https://hg.mozilla.org/mozilla-central/rev/facab7643e3f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 93•8 years ago
|
||
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?
Assignee | ||
Comment 94•8 years ago
|
||
Approval Request Comment: See comment 93.
Attachment #8732026 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 95•8 years ago
|
||
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?
Assignee | ||
Comment 96•8 years ago
|
||
Approval Request Comment: See comment 93.
Attachment #8732029 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 97•8 years ago
|
||
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?
Assignee | ||
Comment 98•8 years ago
|
||
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?
Assignee | ||
Comment 99•8 years ago
|
||
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?
Assignee | ||
Comment 100•8 years ago
|
||
FYI: The new behavior (not dispatching reserved keyboard events into web contents) is similar to the other browsers.
Comment 101•8 years ago
|
||
It is unclear to me why we'd take this kind of some what risky change to beta.
Comment 102•8 years ago
|
||
(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.
Assignee | ||
Comment 103•8 years ago
|
||
(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?
Assignee | ||
Comment 105•8 years ago
|
||
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.
Comment 106•8 years ago
|
||
(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)
Updated•8 years ago
|
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Comment 107•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8732029 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•8 years ago
|
Attachment #8729967 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•8 years ago
|
Attachment #8729968 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•8 years ago
|
Attachment #8729969 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 108•8 years ago
|
||
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)
Comment 110•8 years ago
|
||
(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)
Comment 111•8 years ago
|
||
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.
Comment 112•8 years ago
|
||
s/like/live
Comment 113•8 years ago
|
||
(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 114•8 years ago
|
||
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-
Assignee | ||
Updated•8 years ago
|
Blocks: PluginShortcuts
Comment 115•8 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.
Assignee | ||
Comment 116•8 years ago
|
||
(In reply to Alan from comment #115) > Is this bug a WONTFIX on Linux? No, this is fixed on all platforms.
Assignee | ||
Comment 117•8 years ago
|
||
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.
Comment 118•8 years ago
|
||
isn't this supposed to work also for F5/ctrl-F5?
Assignee | ||
Comment 119•8 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•