User-defined (custom, System Preferences) shortcuts don't work

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: dev-null, Assigned: spohl)

Tracking

(Blocks 2 bugs, {regression})

Trunk
mozilla55
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +

Firefox Tracking Flags

(blocking2.0 -, relnote-firefox 55+, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [No more "I want this FIXED" comments please] [Advo] [tpi:+])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; ja-JP-mac; rv:1.9pre) Gecko/2008041908 Minefield/3.0pre Firefox/3.0b
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041904 Minefield/3.0pre

User-defined shortcuts added in System Preferences don't work.

Reproducible: Always

Steps to Reproduce:
1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
2. Click [+] and add shortcut as
   Menu Title: Add-ons
   Keyboard Shortcut: ⇧⌘A
3. Start Firefox.
4. Press Cmd+Shift+A

Actual Results:  
Add-ons window does not open.

Expected Results:  
Add-ons window opens.

20080407_2058 works
20080407_2140 fails

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1207627080&maxdate=1207629599
I suspect regression from Bug 398514.

Updated

11 years ago
Blocks: 398514
Keywords: regression
I don't see any dupes and I can confirm this in a Minefield nightly. This is a pretty bad regression for Mac users. Requesting blocking.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
Hardware: Macintosh → All
Version: unspecified → Trunk

Comment 2

11 years ago
Unfortunately I don't think this feature is compatible with the way Gecko works now and we're going to have to live with the regression for now. The reason it worked before is similar to the reason we didn't suffer from bug 78414 for a long time (see bug 428047). We might be able to work out a solution to this, but any solution is going to involve another very risky change to the way our key event handling works on Mac OS X and we simply cannot do that for Gecko 1.9 any more.
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-

Comment 3

11 years ago
Note that if user-defined shortcuts are a big enough deal for Mozilla Mac OS X users it wouldn't be very hard to write an extension that allowed it.
(In reply to comment #2)
> Unfortunately I don't think this feature is compatible with the way Gecko works
> now and we're going to have to live with the regression for now. 

Just to make sure I understand... this feature worked fine in Carbon builds (Firefox 2), worked fine in Cocoa builds (Firefox 3b5) prior to the landing of bug 398514, and won't work fine in Firefox 3 final because the fix in bug 398514 made Gecko incompatible with a fairly standard Mac OS X feature. Is that all correct?

(In reply to comment #2)
> We might be able to work out a solution to this,
> but any solution is going to involve another very risky change to the way our
> key event handling works on Mac OS X and we simply cannot do that for Gecko 1.9
> any more.

I consider bug 398514 a very risky change to the way our key event handling works, very late in the game, and it caused several regressions (this one included), but that didn't stop us from landing it.

Comment 5

11 years ago
> (In reply to comment #2)
> Just to make sure I understand... this feature worked fine in Carbon builds
> (Firefox 2), worked fine in Cocoa builds (Firefox 3b5) prior to the landing of
> bug 398514, and won't work fine in Firefox 3 final because the fix in bug
> 398514 made Gecko incompatible with a fairly standard Mac OS X feature. Is that
> all correct?

Yes. We had fundamental problems with keyboard event handling from Firefox 1.0 up until the patch for bug 398514 landed. We finally had to deal with those issues and it was worth it even with consequences like this bug. Eventually we'll be able to make even more progress and hopefully fix this bug, but we've run out of time for this release.

"Fairly standard" is a sneaky phrase - clearly the vast majority of Mac OS X users don't use that feature of Mac OS X, even fewer with Gecko apps in particular, so what is "fairly standard" supposed to mean?

> I consider bug 398514 a very risky change to the way our key event handling
> works, very late in the game, and it caused several regressions (this one
> included), but that didn't stop us from landing it.

If you're trying to advocate the go-ahead for another risky patch, your own statement here is a solid argument against doing so. The consequences of not fixing that bug were far more severe than this, and it is now even later.

I think we can find a way to make this feature with with XUL apps, I'll keep it in mind when I'm working on further key handling improvements in the next release of Gecko.
Duplicate of this bug: 435863

Updated

11 years ago
Duplicate of this bug: 438267

Comment 8

11 years ago
> "Fairly standard" is a sneaky phrase - clearly the vast majority of Mac OS X
> users don't use that feature of Mac OS X, even fewer with Gecko apps in
> particular, so what is "fairly standard" supposed to mean?

With all due respect Josh, please note that "clearly the vast majority of Mac OS X users don't use that feature..." is an even sneakier phrase than "Fairly standard".

There's nothing "clear" about whether the vast majority of users do or do not use this feature.  Do you know how many 3rd party websites advertise this feature as a workaround for the shortcomings in Firefox and other apps?  There are quite a few, but I'm going to avoid saying anything sneaky like "clearly there are more users than you know about, and they're all going to be very annoyed when FF3 launches."  Ever seen how fanatical and illogical Mac users can be over something like this?

This is a big feature for OSX users, especially power users like the early adopters of Firefox.  I don't really care if it's fairly standard or not, but it shouldn't be dismissed too casually.

I avoided upgrading to RC1 and RC2 because of this, I'm sure other users will avoid upgrading to FF3 when the final version is released.

If you can't fix this, then consider adding a utility (or enabling an add-on) that will allow users to change the keyboard mappings within Firefox.  It can be as simple as some entries in about:config for the menu items that do not currently have keyboard shortcuts on OSX.

Thanks.

Comment 9

11 years ago
(In reply to comment #5)
> "Fairly standard" is a sneaky phrase - clearly the vast majority of Mac OS X
> users don't use that feature of Mac OS X, even fewer with Gecko apps in
> particular, so what is "fairly standard" supposed to mean?
> 
I am not a programmer and don't understand all of what is written is this thread, however, I think I have found the reason that my custom Mac OS keyboard commands no longer work.

I am one of the presumed "minority" of Mac OS X users that depend on this feature.  The fact that early versions weren't compatible with it kept me from using Firefox. I was very disappointed to realize that The change from vs2 to vs3 broke this compatibility, but I am not complaining about the price - I just want you to know that there are users that will very much appreciate when this feature returns.

Nancy


Duplicate of this bug: 441977

Updated

11 years ago
Duplicate of this bug: 442716

Comment 12

11 years ago
Please could we please have an update on this? I can't still use version 3.0 because of this bug.
Duplicate of this bug: 445656

Comment 14

11 years ago
Another user here that misses the Universal keyboard shortcuts in firefox 3.0.

I like to maximize(Windows->Zoom in os-x terms I guess) the firefox window using a keyboard shortcut. 

This used to work (kind of) in firefox 2.x but it's completely broken in firefox 3.0.

Comment 15

11 years ago
I'd like to chime in and request that Firefox PLEASE support the OS-based keyboard shortcuts. It is frankly one of the fundamental rights of the user to use the software the way he wants, and in my own case I've got a dozen or so keys that I use in Safari a certain way, which I'd like to fix. There are some workarounds for the keyboard shortcuts being broken (Menu Master is one, but it has a success rate of only 90% every time you go to use a keyboard shortcut), and Quickeys is another. For FF to be considered a stable release by Mac users, this functionality needs to be restored. Basically, if something is broken and the release doesn't have a 'b' in its version number, it's basically lying to the users that the program is stable and okay for them to use.

The lack of this functionality in FF3 is similar to when Vuescan, a (now) poplar scanning software for Mac and Windows that supports many scanners, first came out. The guy didn't bother to make an icon and when I told him that Mac users often won't even click on an app where the programmer didn't care enough to crate an icon, he pooh-poohed me. Thankfully he saw the light and his software is now very polished and successful. 
(In reply to comment #5)
> I think we can find a way to make this feature with with XUL apps, I'll keep it
> in mind when I'm working on further key handling improvements in the next
> release of Gecko.

Josh, before we have the same timing problems with Gecko 1.9.1 again, here a friendly reminder. No idea if this bug qualifies itself for blocking but at least it should be wanted1.9.1.
Flags: blocking1.9.1?

Comment 17

11 years ago
I do want to fix it, and in 1.9.1 would certainly be nice, but I doubt we're going to get it fixed for 1.9.1. Most likely 1.9.2.

Please no more comments about how people really want this fixed. Post only if you have new information or something to say about a fix.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-

Updated

11 years ago
Flags: wanted1.9.1+

Updated

11 years ago
Duplicate of this bug: 457973

Updated

11 years ago
Duplicate of this bug: 469597

Updated

10 years ago
Duplicate of this bug: 471761
(Reporter)

Updated

10 years ago
Summary: User-defined shortcuts don't work → User-defined (custom, System Preferences) shortcuts don't work

Updated

10 years ago
Duplicate of this bug: 472271
Flags: blocking1.9.2?

Updated

10 years ago
Assignee: joshmoz → nobody

Comment 22

10 years ago
I wish it were fixed, i'm highly reliant on shortcuts to keep my productivity up, so i'm not always stuck in the office so i can spend some time with family or loved ones. I'm sure other pple are the same. I don't know any programming, but i've donated money to open source pple keep up the good work. 
It's been racking my mind why the keyboard preference custom shortcuts don't work, because they show up visibly on the menu, and are blue highlighted when u click'em but they just don't work. 
I've maxed out, all these imacros, autopages,mouseless browsing, one-click URL addons, but this one seems like too hard to be fixed. 

Good luck fellas.

Comment 23

10 years ago
3.5 is out and new bug report on this has been reported.  Any chance this is ever going to get fixed?
Flags: wanted-fennec1.0?

Comment 24

10 years ago
Did it make it into 1.9.2?
This doesn't even apply to Fennec in any way, so please don't nominate it for a Fennec release.

1.9.2 has not shipped yet, it's still in development, but this bug is currently not fixed, so it hasn't been fixed anywhere.
Flags: wanted-fennec1.0?

Comment 26

10 years ago
I've been tracking this for ages.

We seem to have a practical problem.  The people that CAN repair this, on a core level, aren't seeing it as a high-enough priority to risk the complex regressions that it might introduce.

The people who want it (for the most part) don't know how to fix it.

On really dark nights, I've begun to wade into the development docs for FF, but I tend to wave off because (as most people who have tried to help and actually DO code can attest) there's a nasty entryway to this particular dev-tree... there's a rich "if you don't already know, don't ask" quality to this space that blocks developers from joining in.

But aside from nominal political conversations -- it sounds like the code requirement can be addressed in other ways as well (this is an OR list):

1 - allow ALL menu commands to be custom programmed to a user-defined keystroke
2 - point us noobs at a decent place to outline how we can create an overlay that will intercept key event
3 - examine the fact that, universally, Mozilla is placing a second click-requirement on event focus between the OS and the app.

Every bug related to this has been described as "once I click on <insert menu name here>" it works until I navigate away and come back."

Like I said, I don't now your code space, but that says to me that this SEEMS to be more about placing the effective functionality higher up in [MAIN]

This may be an intrinsic and powerful mondo super core code problem -- but I just get this Spidey sense that it's about managing subordinate focus -- that somewhere in there, at the moment that the FF window has focus, but the menu layer (and its related subfunctions) has NOT been touched, the FF window just fails to echo the keystroke back to the system.  Once the menu has been touched, and whatever magic brings it into active space, the keystrokes DO go back to the System -- which is the scope that actually processes these custom keys, btw.

In other words - I don't think the FF community needs to build a "custom system shortcut processor" at all -- I think you need to recognize the difference in state between when you first bring focus to FF and you subsequently click on the menus.  Whatever state change occurs at that moment is what makes the keystroke properly get back to the OS for processing.

At the risk of rambling, take my example:

I have SHIFT+CMD+D set to trigger "Run Filters on Folders..." under the Tools menu of TB (same problem, different app).

I open TB (focus comes to the TB window).  I hit my keystroke, nothing happens.

I click on the Tools menu, select "Run Filters on Folders..." manually (it forks and runs, presumably).

STOP.  What is the STATE CHANGE at this moment?  What is different between opening the window vs. opening the window and clicking a menu.  Because at this moment in the code -- CONSISTENTLY - the shortcut now works.

In other words -- once the sub-procedure is run or called by way of mousing, the key-events are echoed to the OS successfully.

Take the focus to another app (like Word or some such thing) and come back -- I've lost the magic, and the key event is not echoed again.

Can't we just find that moment BEFORE the menu is clicked and insert a call to echo the key event back to the OS?
Malcolm, this bug is + on wanted:next, and ? on blocking 1.9.2, so the odds are that is will get fixed in Firefox.next. You have priorities too.
Whiteboard: [No more "I want this FIXED" comments please]

Updated

10 years ago
Flags: blocking1.9.2? → blocking1.9.2-
Duplicate of this bug: 487401

Comment 29

10 years ago
This bug is devastating when using a danish keyboard;

Typing cmd+alt+8 or cmd+alt+9 which are the [ and ] gives the same result - the history menu flashes and nothing happens...

As i also need shortcuts for programming my logitech mouse, this is almost ruining firefox!

Please at least make some logical shortcuts (like alt+arrows)

Programming shortcuts and using the mouse works perfectly in safari...

Comment 30

10 years ago
(In reply to comment #29)
> Please at least make some logical shortcuts (like alt+arrows)

You can use cmd-arrows and ctrl-tab/ctrl-shift-tab.

Using a modifier and arrowkeys for navigating back and forward is generally a bad idea as it will not work if the cursor is located in a text-field. Here it will default to standard end/home of the current line.

Comment 31

10 years ago
Ah, cmd-arrows are used for default spaces switching, so with spaces activated you need to select alt-arrows as the space-shifting for firefox to be recieve the keypress!

Now it works, would be nice with this information easily available and for the danish version for mac to have the cmd-arrows listed in the menu by default

Updated

10 years ago
Duplicate of this bug: 515395

Updated

10 years ago
Duplicate of this bug: 533318

Comment 34

9 years ago
I'm a Dvorak & OSX user and I like to have my <copy><paste> shortcuts mapped as
<apple>+J
<apple>+K

Unfortunately, <apple>+J is the shortcut for the downloads window.  I tried fixing this by applying custom keyboard shortcuts via the System Preferences>Keyboard&Mouse panel, and they show up corrected in Firefox's menu, but downloads shows up every time I hit <apple>+J.  This pretty much destroys my comfort in using Firefox.
blocking2.0: --- → ?

Comment 35

9 years ago
Thunderbird 3.01 - this bug has escalated to a worse condition for MacOSX users.

Now, once the System Keyboard shortcut has been defined by the user to map to a specific menu command (in my case "Run Filters on Folder"), the menu "flashes" when the keystroke is pressed, but the actual command does not trigger.

So, on the plus side, we have "recognition" inside the Thunderbird interface scope -- but we do not have it following through with an actual trigger to the menu command.

This behavior is repeatable, and survives across removal of system configuration (i.e. I went to the System Prefs, removed my keyboard shortcut, then put it back in and still get the same behavior)
Duplicate of this bug: 546970

Updated

9 years ago
Duplicate of this bug: 552691
Duplicate of this bug: 554657
Won't block the release on this, but we'd definitely take a patch.
blocking2.0: ? → -

Updated

9 years ago
Duplicate of this bug: 560760
Duplicate of this bug: 316459
Duplicate of this bug: 328746
Duplicate of this bug: 566809

Comment 45

9 years ago
Add me to the list of people who want this fixed.
It prevents me from switching to Firefox 100%. :(
Duplicate of this bug: 546899

Updated

8 years ago
Duplicate of this bug: 624881

Comment 48

8 years ago
How many ”duplicate bugs” will we have before someone takes the bug seriously? :)
(In reply to comment #3)
> Note that if user-defined shortcuts are a big enough deal for Mozilla Mac OS X
> users it wouldn't be very hard to write an extension that allowed it.

Josh, could you give some details which specific things an extension would have to include to make system-wide shortcuts available in Firefox? If we can't fix it in core we could probably find someone who can work on such an extension.

Updated

8 years ago
Duplicate of this bug: 646362

Comment 51

8 years ago
Just wanted to say I too have been frustrated by this bug.

Comment 52

8 years ago
There seems to be a solution!
At least it worked for me.

I installed the add-on keyconfig: http://forums.mozillazine.org/viewtopic.php?t=72994

Then I wen to its preferences and set the keys I want to use for ”Back” and ”Forward” and it worked!

Tested in Firefox 5 and all seems fine. Finally. :)

Comment 53

8 years ago
(In reply to comment #52)
I don't have time to test this add-on, but still feel the bug should be fixed. It's not so much that keyboard shortcuts defined in sysprefs "don't work" as "they are not respected". What I mean is that if I specify that the "Quit Firefox" menu command shortcut should be replaced by option-command-Q, the option-command-Q combo *will* work to quit FF, but **so will command-Q**. In other words the custom shortcut works but so does the original. FF is the only app I use that has this problem. It needs to be fixed. I don't know how many times I lost a session by hitting Command-Q instead of Command-W (keys right next to each other)

Comment 54

8 years ago
I can't think of any disadvantage if cmd+Q accidentally pressed other than a momentary loss of one's time, since one can always immediately relaunch.  If one happens to be in the middle of creating a new message and accidentally presses Quit, the following safety message appears:

"Message has not been sent. Do you want to save the message in the Drafts folder?"

That's the only time I can see it as an issue, and having this safety prompt appears to cover it already.
(In reply to comment #54)
> I can't think of any disadvantage if cmd+Q accidentally pressed other than a
> momentary loss of one's time, since one can always immediately relaunch.

This bug covers Core code and impacts much more than just Thunderbird. Quitting and re-launching Firefox or Seamonkey with several dozen tabs open is not a "momentary loss" of time.
(In reply to comment #54)
> I can't think of any disadvantage if cmd+Q accidentally pressed other than a
> momentary loss of one's time

The issue of it being a "minor inconvenience", or not, is irrelevant. It's a bug.

Comment 57

8 years ago
(In reply to comment #55)
> (In reply to comment #54)
> > I can't think of any disadvantage if cmd+Q accidentally pressed other than a
> > momentary loss of one's time, since one can always immediately relaunch.
> 
> This bug covers Core code and impacts much more than just Thunderbird.
> Quitting and re-launching Firefox or Seamonkey with several dozen tabs open
> is not a "momentary loss" of time.

... particularly if one has spent time and effort typing into a text box or is in the middle of some sort of interactive editing session.

Comment 58

8 years ago
just voted this up again … and it seems even more pertinent now that "Cmd-Shift-T" no longer maps to "Get All Messages" in Thunderbird 5 (Bug 655989).

for ages, i've had this problem and hated the fact that mozilla couldn't get this right.

and for ages, this has been "close".

by this, i mean, if one re-maps a key via "System Preferences", one can see the menu containing the re-mapped item "flash" once as though it knows where to find it, and one can also force the re-mapping to work if first opening the menu!


reproducible steps for preceding paragraph:
1. go into "System Preferences -> Keyboard -> Keyboard Shortcuts"
2. choose "Application Shortcuts" in the left pane
3. click "+", and then choose Thunderbird
4. enter "All Accounts" in the "Menu Title:" field
5. click in the "Keyboard Shortcut:" field and perform "Cmd-Shift-T"
6. click "Add"

7. now, get back to Thunderbird
7a. watch (but do not open) the "File" menu and perform "Cmd-Shift-T"
    note that the "File" menu "flashes" briefly.
7b1. click on the "File" menu, but don't scroll into it or do anything else
7b2. perform "Cmd-Shift-T"
     note that it goes and gets all of your mail now!!!

this works the same way for a wide variety of other key-bindings that i've had in my System Preferences from a time when they were actually working.

Comment 59

8 years ago
Keyconfig doesn't seem to work anymore in Firefox 7 so now we (I'm at least) are back at square one again. :(

Back to Safari and Chrome for me...
Duplicate of this bug: 700961
Duplicate of this bug: 705642
Whiteboard: [No more "I want this FIXED" comments please] → [No more "I want this FIXED" comments please] [Advo]
You're not alone. My employer has had Firefox removed from all business computers because he was so irked by this bug. Can't blame him, I've complained of it myself. Is this even still being tracked in any legit sense? It's been what, 5 years since a bug fix wrecked user space for anyone with a medical interface device?

(In reply to staraffinity from comment #59)
> Keyconfig doesn't seem to work anymore in Firefox 7 so now we (I'm at least)
> are back at square one again. :(
> 
> Back to Safari and Chrome for me...
Flags: blocking1.9.2-
Flags: blocking1.9.1-
Flags: blocking1.9-

Updated

6 years ago
Duplicate of this bug: 827652

Comment 64

6 years ago
(In reply to Atsushi Sakai from comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; ja-JP-mac;
> rv:1.9pre) Gecko/2008041908 Minefield/3.0pre Firefox/3.0b
> Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
> rv:1.9pre) Gecko/2008041904 Minefield/3.0pre
> 
> User-defined shortcuts added in System Preferences don't work.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> 2. Click [+] and add shortcut as
>    Menu Title: Add-ons
>    Keyboard Shortcut: ⇧⌘A
> 3. Start Firefox.
> 4. Press Cmd+Shift+A
> 
> Actual Results:  
> Add-ons window does not open.
> 
> Expected Results:  
> Add-ons window opens.
> 
> 20080407_2058 works
> 20080407_2140 fails
> 
> http://bonsai.mozilla.org/cvsquery.
> cgi?module=PhoenixTinderbox&date=explicit&mindate=1207627080&maxdate=12076295
> 99
> I suspect regression from Bug 398514.


My symptoms are:  
Using a Dell keyboard

Open System Preferences Keyboard
Set CTRL to Control
Set Control to CTRL
Try to copy with CTRL-C CTRL-V.

Does not work.  

This is HIGHLY essential.  Is there ANY way to do this in Firefox itself?
Duplicate of this bug: 938303
Duplicate of this bug: 995074
To all problem reporters of bug which was closed as dup of this bug.

If your problem is relevant to "Command+Q", and if your problem is "Command-Q still invokes Quit even though different short cut assignmeet to Quit Firefox works well as expected", read bug 938303 and bugs in dependency tree for that bug, please.
As clearly stated in original report of this bug, comment #0,
> Steps to Reproduce:
> 1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> 2. Click [+] and add shortcut as
>    Menu Title: Add-ons
>    Keyboard Shortcut: ⇧⌘A
> 3. Start Firefox.
> 4. Press Cmd+Shift+A
> Actual Results:  
> Add-ons window does not open.
> 20080407_2058 works
> 20080407_2140 fails
this bug is obviously for symptom of ;
    initial :        Menu Title: XXX, Keyboard Shortcut: Original_Shorcut
    change to : Menu Title: XXX, Keyboard Shortcut: New_Shorcut
   -> New_Shortcut doesn't work

To peoples who closed many many bugs as dup of this bug :

Please never close bugs for following pheomeon as dup of this bug any more.
    initial :        Menu Title: XXX, Keyboard Shortcut: Original_Shorcut
    change to : Menu Title: XXX, Keyboard Shortcut: New_Shorcut
   -> New_Shortcut works pretty well as expected always(or works well in some situations)
   -> Original_Shorcut still invokes XXX always,(or invokes in some situations)
   Example of this : Command-Q still invokes Quit, even after shorcut change to option-Command-Q which works pretty well.
To all problem reporters :

There are two kinds of "Shortcut key" in Firefox on Mac OS X.
(1) Key binding defined by <key>
(2) Keyboard Shortcut assigned to a menuitem, which is defined by <menuitem>.
     This can be changed via System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
     This is shown at right most position of relevant menu item.
Associated key combination to a <key> and a <menuitem> is easily known by "DOM Inspector." Addon.

When you did following for a "Menu Title: XXX",
    initial :        Menu Title: XXX, Keyboard Shortcut: Original_Shorcut
    change to : Menu Title: XXX, Keyboard Shortcut: New_Shorcut
which case did you do?
                    Original_Shorcut                New_Shorcut
   case-1          Not defined by <key>       Not  defined by <key>
   case-2          Defined by <key>             Not defined by <key>
   case-3          Not defined by <key>       Defined by <key>
   case-4          Defined by <key>             Defined by <key>

   Note: Command-Q case is usually case-2, but some peoples try case-4.
             In any Command-Q case, New_Shorcut changed by "Keyboard & Mouse -> Keyboard Shortcuts" works.
Bug 457973, which was once closed as dup of this bug by someone, has following sysmtom.
> Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
> Reproducible: Always
> Steps to Reproduce:
> 1. Using the Keyboard & Mouse pane in System Preferences, add a keyboard shortcut for the "Quit Firefox" menu item
> that's something other than Command-Q.
> 2. Start Firefox (or quit and restart if it was already running).
> 3. Press Command-Q.
> Actual Results:  
> Firefox quits.
> Expected Results:  
> Nothing should happen (or the error tone should sound),
> because command-q is no longer the keyboard shortcut for the "Quit Firefox" menu item.

Obviously,  "keyboard shortcut other than Command-Q" worked well as expected in Firefox 3.0.3 on Mac OS X 10.4.
Problem of Bug 457973 is never that newly assigned "other than Command-Q" doesn't work.
Problem of Bug 457973  is "Command-Q, which is defined as key binding by <key>" still invokes Quit, even after shortcut change works.

To peoples who closed many many bugs, which was opened after the Bug 457973, as dup of this bug :.
Any bug opened after the Bug 457973 is actually dup of this bug, comment #0?

Note:  I already removed  bug 457973, bug 624881, bug 646362, and bug 938303, from dup chain of this bug.
FYI. Bug 515395 was also Command-Q case.
It looks that any bug was closed as dup of this bug, if a bug report is relevant to "Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts"...
(In reply to Atsushi Sakai from comment #0)
> Reproducible: Always
> Steps to Reproduce:
> 1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> 2. Click [+] and add shortcut as
>    Menu Title: Add-ons
>    Keyboard Shortcut: ⇧⌘A
> 3. Start Firefox.
> 4. Press Cmd+Shift+A
> Actual Results:  
> Add-ons window does not open.

Atsushi Sakai san(bug opener), do you still see you problem in recent Firefox(29.0.1 as of today) on recent Mac OS X 10?
(In reply to pythologist from comment #64)
> Open System Preferences Keyboard
> Set CTRL to Control
> Set Control to CTRL
> Try to copy with CTRL-C CTRL-V.
> Does not work. 
> Is there ANY way to do this in Firefox itself?

A way to customize key binding in Mozilla family is;
   Define your key binding by XUL <key> element, using Addon for exection of JavaScript code on XUL element.
For example,
  Install Addon such as Custom Buttons, PrefBar, and create your own button.
      Ctrl+C on Win is defined as : <key id="key_copy" key="c" modifiers="accel"/> ("accel" = Ctrl on Win, Command on Mac)
      Delete is defined as : <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
      So , adding <key id="Your_ID_for_key_name" key="c" modifiers="alt meta control" command="cmd_delete" /> like one
      can be used as alternative of key binding for "Delete something" .
  This is done by pretty simple JavaScript code like next which is defined for the button :
      var keysetID  = "baseMenuKeyset" ; // currently, nothing is defined under this <keyset>, so use it.
      var keyID  = "Your_ID_for_key_name" ;
      var keyset = document.getElementById(keysetID) ;
      var     key = document.getElementById(keyID) ;
      if(key){ keyset.removeChild(key); return; } // Toggle user defined key binding
      var elem = document.createElement("key") ;
      elem.setAttribute("id", keyID) ;
      elem.setAttribute("key", "c") ;
      elem.setAttribute("modifiers", "alt meta control") ;
      elem.setAttribute("command", "cmd_delete") ;
      keyset.appendChild(elem) ;          
Because actual action by Ctrl+C/Ctrl+V like one depends on context(text is selected or not, etc.), customizing of key binding is perhaps harder than key binding for simple, single-context/single-action..
Other possible ways are :
>  Add a key binding by an user defined button : 
>  Shortcut to Toggle Bookmarks Sidebar : Invoke function which is used by existent button or menuitem.
>  <key  ... any key you want oncommand="toggleSidebar('viewBookmarksSidebar');"/>
> Shortcut for Reload :  Invoke DOM event on existent button or menuitem.
>  <key  ... oncommand="var elem=document.getElementById('urlbar-reload-button');elem.dispatchEvent( new Event('command') );" />
Above are essentially similar to creating "small Addon(extension) which adds <key>, <toolbarbutton> etc.".
Flags: needinfo?(dev-null)
(In reply to pythologist from comment #64)

If your own shortcut for edit command is needed, invoking DOM event at menuitem is useful.
  <key  key="c" modifiers="control"  (assume Contrl+C is not used anywhere)
        oncommand="var elem=document.getElementById('menu_copy');elem.dispatchEvent( new Event('command') );"
   />
(In reply to Atsushi Sakai from comment #0)
> Reproducible: Always
> Steps to Reproduce:
> 1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> 2. Click [+] and add shortcut as
>    Menu Title: Add-ons
>    Keyboard Shortcut: ⇧⌘A
> 3. Start Firefox.
> 4. Press Cmd+Shift+A
> Actual Results:  
> Add-ons window does not open.
> Expected Results:  
> Add-ons window opens.

Key binding for "Open addon" is already defined at;
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#277
> <key id="key_openAddons" key="&addons.commandkey;" command="Tools:Addons" modifiers="accel,shift"/>
Entity &addons.commandkey;
> http://mxr.mozilla.org/mozilla-central/search?string=addons.commandkey&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#207
> <!ENTITY addons.commandkey            "A">

These definitions is common among Mac OS X build, Linux build, and Win build.
modifiers="accel" == Command in Mac OS X build(Ctrl in Win/Linux build).
So, Command+Shift+A is already defined for "Open addon" in Mac OS X build, and ."Command+Shift+A" should open Addons window or tab.

I believe following part of this bug is already WORKSFORME.
> 3. Start Firefox.
> 4. Press Cmd+Shift+A
> Actual Results:  
> Add-ons window does not open.

Why this bug is still kept open even though original problem of this bug doesn't occur?
"Cmd+Shift+A" still doesn't work in recent Firefox on Mac OS X?
Or following still interferes Firefox's key binding for "Cmd+Shift+A"?
> System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> 2. Click [+] and add shortcut as
>    Menu Title: Add-ons
>    Keyboard Shortcut: ⇧⌘A
Bug 827652, which was closed as dup of this bug on 2013-04-12, was for following symptom in Firefox 17.
> Assign Cmd+Shift+Ctrl+M for Menu Title, type "Zoom" (Window > Zoom menu.)
> Actual Results:
> When the shortcut is pressed, the Window menu is highlighted, but nothing happens.
Reporter says;
> Expected Results:
> When the shortcut is pressed the first time, the Firefox window should maximize.
> When the shortcut is pressed a second time, the Firefox window should return to its normal size.
> Observations:
> 1. Firefox behaves as expected when clicking directly on Window > Zoom menu.
> 2. The shortcut for Window > Minimize works correctly.
So, this is perhaps for "Full Screen (toggle)", instead of Zoom In/Out(Full Page==not Text Zoom),  "Cmd +" / "Cmd -".
> See http://www.accessfirefox.org/Firefox_Keyboard_and_Mouse_Shortcuts.html

Phenomenon of that bug is following :
> Actual Results:
> When the shortcut is pressed, the Window menu is highlighted, but nothing happens.
> When already in Full Screen mode(via menu), assigned shortcut works as expected.

How can this phenomenon of Bug 827652 be dup of original problem of this bug, comment #0?
If "System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts" is relevant, all is same problem?
(In reply to WADA from comment #76)

> If "System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts" is
> relevant, all is same problem?

Yes. The user _should_ be able to override / modify / add keyboard shortcuts to Firefox through that System Preference pane – for any menu item. As of the current nightly build (and all release builds since this bug was filed) this does _not_ work. 

At best, the new keyboard shortcut will be added to the menu item (on OS X 10.8 and 10.9 there is no need to restart the app for that), but pressing the keyboard shortcut will only highlight the menu, not perform the required action (as it should).

And this what this bug is about, and the multiple dupes that this bug has/had.
(In reply to philippe (part-time) from comment #77)
> (In reply to WADA from comment #76)
> > If "System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts" is relevant, all is same problem?
> Yes. 

I see. I will never touch this bug any more.
Please never close Command-Q relevant report as dup of this bug any more. If you'll dup such bug, please dup to bug 938303.
(In reply to philippe (part-time) from comment #77)
> (In reply to WADA from comment #76)
> 
> > If "System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts" is
> > relevant, all is same problem?
> 
> Yes. The user _should_ be able to override / modify / add keyboard shortcuts
> to Firefox through that System Preference pane – for any menu item. As of
> the current nightly build (and all release builds since this bug was filed)
> this does _not_ work. 
> 
> At best, the new keyboard shortcut will be added to the menu item (on OS X
> 10.8 and 10.9 there is no need to restart the app for that), but pressing
> the keyboard shortcut will only highlight the menu, not perform the required
> action (as it should).
> 
> And this what this bug is about, and the multiple dupes that this bug has/had.

If so, I recommend you to open bug for "Firefox has bug. Firefox's bug should be resolved. Firefox shouldn't have bug."
Any bug report by Firefox user at bugzilla.mozilla.org, which is relevant to Firefox, can be closed as dup of the bug.
It can pretty efficiently prohibit, or disturb, or interfere, problem analysis/bug fixing at bugzilla.mozilla.org.
(In reply to philippe (part-time) from comment #77)
> (In reply to WADA from comment #76)
> 
> > If "System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts" is
> > relevant, all is same problem?
> 
> Yes. The user _should_ be able to override / modify / add keyboard shortcuts
> to Firefox through that System Preference pane – for any menu item. As of
> the current nightly build (and all release builds since this bug was filed)
> this does _not_ work. 
> 
> At best, the new keyboard shortcut will be added to the menu item (on OS X
> 10.8 and 10.9 there is no need to restart the app for that), but pressing
> the keyboard shortcut will only highlight the menu, not perform the required
> action (as it should).
> 
> And this what this bug is about, and the multiple dupes that this bug has/had.

FYI.
Bug 457973, Bug 515395, Bug 624881, Bug 646362, and Bug 938303, which were once closed as dup of this bug by someone like you, will be resolved by Bug 938303.
This is pretty good example  that bad duping efficiently prohibits, disables, interferes, or disturbs,  proper problem analysis/bug fixing at bugzilla.mozilla.org.
(In reply to philippe (part-time) from comment #77)
> but pressing the keyboard shortcut will only highlight the menu, not perform the required action (as it should).

Who can know that essential symptom/phenomenon of this bug is it by reading this bug?
There is another important phenomenon, isn't there?
   If relevant menu is opened, newly assigned shortcut key works as expected.
Such "only highlight the menu, not perform the required action" actually occurred in Frefox menu/Quit Firefox(=Command-Q) case?
See bug 827652 comment #7, please.

By the way, please never use "duping" for tracking purpose.
"Dup" is "duplicate bug" == Basically, "absolutely same phenomenon, absolutely same cause, absolutely same condiion, absolutely same solution, ...".

For tracking purpose of bugs relevant to "User-defined (custom, System Preferences) shortcuts don't work",
> If relevant menu is not opened, pressing the keyboard shortcut will only highlight the menu, not perform the required action (as it should)
> If relevant menu is opened, newly assigned shortcut key works as expected.
utilize "Meta Bug", please.
  - meta in keyword: field. Many peoples puts [Meta Bug], [Tracking Bug] in bug summary for eye catcher.
  - Put relevant bugs in "Depends on:" field of the "Meta Bug" for tracking.
    == Put the Meta Bug in "Blocks:" field of each bug.
All relevant bugs are listed by "Show Dependency Tree".
I've reopened following bugs which were once closed as dup of this bug(Bug 429824), in order to check similarities/differences among these bug reports. (i.e. I've removed these bugs from dup chain of this bug for proper problem analysis).
    (a) Zoom(no assigned shortcut) case :
          Bug 316459  for Firefox 1.5 : Oldest bug which was once closed as dup of this bug.
             Reported phenomenon in Firefox 1.5 was : New shortcut doesn't work until menu is clicked.
             Symptom-a-1 : Before first menu click after shortcut assignment, new shortcut doesn't invoke action of menu.
             Symptom-a-2 : After first menu click after shortcut assignment, new shortcut does invoke action of menu.
          Bug 827652  for Firefox 17  : Newest bug which was once closed as dup of this bug.
             Symptom-a-3 : 
                  Symptom-a-3-1 : If menu is closed, new shortcut doesn't work(==command is not invoked).
                  Symptom-a-3-2 : Menu blinks instead.
             Symptom-a-4 : If menu is opened, new shortcut does work(==command associated to menuitem is invoked).
    (b) Menu which has assigned shortcut other than  "Quit Firefox" . Minimize(Cmd-M) and Download(Cmd-J)
          Bug 316459  for Firefox 1.5 : Minimize(Cmd-M) : Bug 316459 reported both Zoom(no shortcut) and Minimize(Cmd-M) 
          Bug 328746  for Firefox 1.5 : Download(Cmd-J) : Oldest bug which was once closed as dup of this bug.
             Newly assigned shortcut :
                Symptom-a-1  always occurs on newly assigned shortcut
             Originally assigned shortcut :
                Symptom-a-5 : Original shortcut always invokes command associated to <key>
    (c)  Quit Firefox(Command-Q) case
          Bug 938303 and bugs which are now placed in dup chain of  Bug 938303. 
          Report after Firefox 3.0 only..
             Newly assigned shortcut :
                 Symptom-a-3 doesn't occur in Quit Firefox(new shortcut) case. Newly assigned shortcut always works.
             Originally assigned shortcut (Command-Q) :
                 Firefox 1, Firefox 2      :  Symptom-a-5 doesn't occur. Original shortcut does do nothing.
                                                        i.e. user could kill Command+Q by new shortcut assignment.
                 Firefox 3 to Firefox 29 :  Symptom-a-5 always occur, then Firefox terminates by Command-Q.
                                                        i.e. user can't kill Command+Q by new shortcut assignment.
                 After fix of Bug 938303 : Command-Q does do nothing, so user can press Command-Q freely, safely.
Because of Symptom-a-4  and Symptom-a-5 exists, following occurs when "Swap two shortcuts" is executed.
    (d)  "Swap two shortcuts" case
           If menu is opened, Symptom-a-4 always occurs.
           If menu is closed,   Symptom-a-5 always occurs.

Any bug report which was once closed as dup of this bug has one or more following symptoms.
    Symptom-a-1 in Firefox 1.5, Symptom-a-2 in Firefox 1.5,
    Symptom-a-3-1, Symptom-a-3-2, Symptom-a-4 after Firefox 3 (excluding Quit Firefox)
    Symptom-a-5 in any Firefox releases (excluding Command-Q for Quit Firefox in Firefox 1 and Firefox 2)

What kind of symptom does following bug summary of this bug mean?
    User-defined (custom, System Preferences) shortcuts don't work
It's usually Symptom-a-3-1 only.
If so, "duping Command-Q case to this bug" was apparently wrong, bad, stupid action, because Symptom-a-3-1 never occurs.
"don't work as user expects"?
If so, what is purpose of "duping any bug relevant to User-defined shortcuts to this bug"?
It's merely hiding important/valuable observations done in each bug by bug reporter, isn't it?
It's merely prohibiting proper problem analysis/bug fixing at bugzilla.mozilla.org, isn't it?

Original report of this bug, comment #0, has following special characteristics.
    Refers to Symptom-a-3-1 only.
    No description about important/common Symptom-a-3-2.
    Keyboard Shortcut: ⇧⌘A is already assigned to Add-ons menu by default.
    i.e. Phenomenon stated in comment #0 is already WORKSFORME.
Is "duping any bug relevant to User-defined shortcuts to this bug" good behavior at bugzilla.mozilla.org?
Is "duping bug reports for Firefox 1.5 to this bug for Firefox 3.0"  good behavior at bugzilla.mozilla.org?
Flags: needinfo?(dev-null)
(In reply to philippe (part-time) from comment #77)
> (a) As of the current nightly build (and all release builds since this bug was filed) this does _not_ work. 
> (b) pressing the keyboard shortcut will only highlight the menu, not perform the required action (as it should).
> And this what this bug is about, and the multiple dupes that this bug has/had.

After removal of Quit Firefox(Command-Q) from dup chain of this bug and removal of two bug reports on Firefox 1.5, I read Bug 435863(oldest bug in current dup chain) and Bug 827652(which was newest bug in dup chain of this bug after removal of Command-Q case).
Both are for same ZOOM(no predefined shortcut) case, and reported phenomenon was absolutely same - (a) & (b) in your comment, and (c) "If menu is opened, newly assigned shortcut invokes command specified in <menuitem>".
    Bug 435863 : report on Firefox 3 RC1
    Bug 827652 : report on Firefox 17
We also saw same symptom of your (a) & (b) & (c) in ZOOM case on latest Firefox 29.0.1.

No proble analysis about "(b) even though (c)" since bug open of this bug till today?

Anyway, I opened meta bug 1015670 for ease of tracking and analysis.
Duplicate of this bug: 436712
Duplicate of this bug: 434477
Duplicate of this bug: 995074
(Reporter)

Comment 87

5 years ago
(In reply to WADA from comment #72)
> > Steps to Reproduce:
> > 1. Open System Preferences -> Keyboard & Mouse -> Keyboard Shortcuts.
> > 2. Click [+] and add shortcut as
> >    Menu Title: Add-ons
> >    Keyboard Shortcut: ⇧⌘A
> > 3. Start Firefox.
> > 4. Press Cmd+Shift+A
> > Actual Results:  
> > Add-ons window does not open.
> 
> Atsushi Sakai san(bug opener), do you still see you problem in recent
> Firefox(29.0.1 as of today) on recent Mac OS X 10?

I'm sorry to be late.
With recent Firefox, actually add-ons open.
Because recent Firefox itself has Cmd+Shift+A shortcut, it works independently with System Preferences.
(In reply to Atsushi Sakai from comment #87)
> Because recent Firefox itself has Cmd+Shift+A shortcut, it works independently with System Preferences.

Thanks for your response.
If so, "duping all bug reports to this bug", which is bug for "Cmd+Shift+A" case, which is already WORKSFORME case, is pretty confusing, rather misleading, even if your report on "Cmd+Shift+A"  case was absolutely correct in the past and your report was first proper report about the problem after Firefox RC1?
(Reporter)

Comment 89

5 years ago
I think duping all bugs is confusing too. Utilizing meta bug makes sense.

Updated

5 years ago
Duplicate of this bug: 1028561

Updated

5 years ago
Duplicate of this bug: 316459

Comment 92

4 years ago
I'm adding further detail: if there are no open or visible windows in Firefox, i.e. if there are either no windows or all existing are minimized to dock, then shortcuts set via System Preferences -> Keyboard -> Shortcut pane will work.

I tested this with both History -> Show All History menu item and with Tools -> Apps on OS X 10.10.5 using Firefox 40.0.3.
In both cases with the previous condition being verified Firefox showed the full history windows or un-mimized the last window and created a new tab pointing to the marketplace.

If the previous condition isn't true then, as already mentioned, the relative menu item highlights but no action is performed by Firefox.

Comment 93

4 years ago
For all who arrived here and were trying to figure out if there was an easy workaround:

1.) Install "Customizable Shortcuts". The latest release here: https://addons.mozilla.org/en-US/firefox/addon/customizable-shortcuts/
2.) Once installed, you will see a small keyboard key icon in the upper right corner of your browser, click it.
3.) Scroll down to the "Edit" section.
4.) Select "Copy" and hit the edit button. Hold down the 'Ctrl' and 'c' keys. It will remap and work as expected. 
5.) Repeat the process for select all, cut, and paste if you wish. 

My $.02:
  I agree, it is annoying when keys are re-mapped at an iOS wide level, the Edit menu in Firefox shows the correct re-mapped keys, but it still does not work as expected. Based on all the constraints and requirements for security, Customizable Shortcuts and like solutions are probably a better option than beating a dead horse or the Development team. 

;-)

Updated

3 years ago
Duplicate of this bug: 1024061

Comment 95

3 years ago
Unfortunately, the Customizable Shortcuts add-on has been removed by its author.

This horrid bug really needs to be fixed.

Comment 96

3 years ago
Tried Keybinder, it works for me. This bug still needs to be fixed. How an application can be so non-compliant with its host O/S is beyond me.

https://addons.mozilla.org/en-US/firefox/addon/keybinder/
Whiteboard: [No more "I want this FIXED" comments please] [Advo] → [No more "I want this FIXED" comments please] [Advo] [tpi:?]

Updated

3 years ago
Priority: -- → P1
Whiteboard: [No more "I want this FIXED" comments please] [Advo] [tpi:?] → [No more "I want this FIXED" comments please] [Advo] [tpi:+]
As originally reported in comment 1 - this is now working for me. "ni" me if there are additional concerns or confirmations
Version 	47.0.1
Build ID 	20160623154057
 User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
1) Apple System Preferences
2) Keyboard, Shortcuts, App Shortcuts, Add cmd shift A named AddOn
3) Open or return to Firefox and apply short cut key combo
Observe: AddOn page is opened in new tab

2)

Comment 98

3 years ago
Michelle, the reason it works for you is that since the original report, Shift-Cmd-A has been added to Firefox as a built-in default keyboard shortcut. This shortcut was not present in Firefox 3; the reporter was attempting to add it as a user-defined shortcut. So that test is no longer valid for this bug, however the bug itself (user-defined shortcuts don't work) is still valid.

The problem appears to be that Firefox handles keyboard shortcuts itself, according to its built-in default shortcuts, unless a menu is open / has focus. It doesn't have to be the menu containing the command - as long as any menu is visible, the system-defined user shortcuts are respected.

It can be reproduced by:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0

A) Add a custom user-defined shortcut for a menu item that has no built-in default shortcut, eg. "Show All History (open History (Library) window)":
1. Go to Apple System Preferences
2. In Keyboard, Shortcuts, App Shortcuts, add: Ctrl-Cmd H, with menu name: Show All History
3. Open or return to Firefox, and press Ctrl-Cmd H.
Result: The History menu title flashes, but the History (Library) window does not open.
Expected result: the History (Library) window should open, just as though you had selected "Show All History" from the History menu with the mouse.
4. Click on any menu title, the menu drops down, do not move the mouse.
5. Press Ctrl-Cmd H.
Result: the History (Library) window opens correctly.
Expected result: the History (Library) window should open, regardless of whether the menu is visible / has focus or not.

B) Change a built-in default shortcut to a custom user-defined shortcut, eg. "History" (show History sidebar, default Shift-Cmd H):
1. Go to Apple System Preferences
2. In Keyboard, Shortcuts, App Shortcuts, add: Ctrl-Cmd H, with menu name: History
3. Open or return to Firefox and press Ctrl-Cmd H.
Result: The View menu title flashes, but the History sidebar does not open.
Expected result: the History sidebar should open, just as though you had selected "History" from the View->Sidebar menu with the mouse.
4. Click on any menu title, the menu drops down, do not move the mouse.
5. Press Ctrl-Cmd H.
Result: the History sidebar opens correctly.
Expected result: the History sidebar should open regardless of whether the menu is visible / has focus or not.
6. Close the History sidebar, then press the built-in default shortcut, Shift-Cmd H.
Result: The View menu title does not blink, but the History sidebar opens.
Expected result: Nothing. The original default shortcut should be disabled.
7. Close the History sidebar, then click on any menu title, the menu drops down, do not move the mouse.
8. Press the default built-in shortcut, Shift-Cmd H.
Result: the History sidebar does not open.
Expected result: The key combination should be the same, regardless of whether the menu is visible / has focus or not.

As a workaround for case B), an addon such as Keybinder or the updated "Dorando Keyconfig" can be used to change the shortcut key combination. However, it may not work with all shortcuts.

For case A), it doesn't seem possible for an addon to add a shortcut for a menu item that doesn't already have one. In Dorando Keyconfig, a new key can be added with appropriate code, eg.:
PlacesCommandHook.showPlacesOrganizer('History');
...but this doesn't work as a toggle, the way the actual menu command does.

PS, I want this fixed! :-)
Flags: needinfo?(mfunches)
QA Confirmation (Michelle-QA): Thanks for the updates and explanations Elhem. The issues has a P1 priority set which indicates the issues in not forgotten and it is being addressed.
Flags: needinfo?(mfunches)
(In reply to Michelle Funches - QA from comment #99)
> QA Confirmation (Michelle-QA): Thanks for the updates and explanations
> Elhem. The issues has a P1 priority set which indicates the issues in not
> forgotten and it is being addressed.

Thank you for the status update. One of the effects of this bug is that it interferes with accessibility devices which present as USB keyboard input; glad to know we haven't been forgotten.

Updated

3 years ago
Duplicate of this bug: 1315498
(Assignee)

Updated

2 years ago
Blocks: 1328604
(Assignee)

Comment 104

2 years ago
Posted patch Patch (obsolete) — Splinter Review
This isn't a perfect solution yet, but it might be a step in the right direction. This patch basically adds the fourth step in this sequence. Step 1-3 are existing behavior:

1. Let native menu system handle shortcut event. This is basically a no-op and just makes sure that the menu bar flashes to give the user feedback when a shortcut is pressed. [1]
2. Let keyDown: handle the event and execute any associated command, if available. [2]
3. If the shortcut has not been handled, see if the application menu wants to handle it. [3]
4. If the event has still not been handled yet, give the native menu system another chance to handle the event by calling NSMenu's performKeyEquivalent directly to see if a custom shortcut has been set for a menu item.

There is one problem that remains: If a shortcut is a built-in shortcut, it cannot be reused as custom shortcut. If this shortcut is pressed, it will continue to trigger the built-in command associated with the shortcut. We could handle this in a followup bug as it may require quite a bit of additional work. We may have to sync our XUL representation of shortcuts with what's actually displayed to the user, or we have to remove our internal shortcut handling via XBL prototypes and let the native menu system handle all shortcuts. Both of these approaches are scary and far exceed the amount of time I currently have available.

Try run of this patch is green (see comment 103).

[1] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuBarX.mm#842
[2] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#5444
[3] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#5450
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8829562 - Flags: review?(mstange)
(In reply to Stephen A Pohl [:spohl] from comment #104)
> 3. If the shortcut has not been handled, see if the application menu wants
> to handle it. [3]
> 4. If the event has still not been handled yet, give the native menu system
> another chance to handle the event by calling NSMenu's performKeyEquivalent
> directly to see if a custom shortcut has been set for a menu item.

Would it make sense to swap the order of 3 and 4? Possibly slightly riskier, but I think it would make a little more sense, because it would allow users to re-assign existing application menu shortcuts to the main menu, effectively avoiding the problem you mention below for the application menu.

> There is one problem that remains: If a shortcut is a built-in shortcut, it
> cannot be reused as custom shortcut. If this shortcut is pressed, it will
> continue to trigger the built-in command associated with the shortcut.

So this patch basically allows users to add their own shortcuts to menu items, but it does not allow them to re-assign existing ones. We should call that out in the commit message and in a comment in -[ChildView keyDown:].
(Assignee)

Comment 107

2 years ago
Posted patch 1. PatchSplinter Review
Thanks, Markus!

(In reply to Markus Stange [:mstange] from comment #105)
> (In reply to Stephen A Pohl [:spohl] from comment #104)
> > 3. If the shortcut has not been handled, see if the application menu wants
> > to handle it. [3]
> > 4. If the event has still not been handled yet, give the native menu system
> > another chance to handle the event by calling NSMenu's performKeyEquivalent
> > directly to see if a custom shortcut has been set for a menu item.
> 
> Would it make sense to swap the order of 3 and 4? Possibly slightly riskier,
> but I think it would make a little more sense, because it would allow users
> to re-assign existing application menu shortcuts to the main menu,
> effectively avoiding the problem you mention below for the application menu.

Good point! I swapped the two and kicked off another try build.

> > There is one problem that remains: If a shortcut is a built-in shortcut, it
> > cannot be reused as custom shortcut. If this shortcut is pressed, it will
> > continue to trigger the built-in command associated with the shortcut.
> 
> So this patch basically allows users to add their own shortcuts to menu
> items, but it does not allow them to re-assign existing ones. We should call
> that out in the commit message and in a comment in -[ChildView keyDown:].

Added comment in both places.

Once this lands I'll open a new bug to support reassignment of existing shortcuts.
Attachment #8829562 - Attachment is obsolete: true
Attachment #8829951 - Flags: review?(mstange)
Attachment #8829562 - Flags: review?(mstange)
Comment on attachment 8829951 [details] [diff] [review]
1. Patch

Review of attachment 8829951 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Attachment #8829951 - Flags: review?(mstange) → review+
(Assignee)

Comment 109

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2823fca4f54792fec980a81e6fdf31171a1f9462
Bug 429824: Let OSX's native menu system handle keyboard shortcuts that we did not handle ourselves via XBL prototype handlers. This allows for handling of custom shortcuts, but does not allow for reassignment of existing shortcuts. r=mstange
We should watch out for reports of new keyboard shortcuts suddenly working when they did nothing in the past.
Have you done any testing while looking out for these cases, or have you run into old bug reports of such cases while looking through the history of this code?
I could imagine keys like +, -, /, ?, = suddenly doing new things when combined with Cmd/Shift/etc on some keyboard layouts, especially on weird mixtures like Dvorak/QuertyCmd. And especially for keyboard shortcuts where the displayed shortcut doesn't match what's displayed on the keyboard, e.g. the shortcut Cmd+?, which on a German QWERTZ keyboard layout needs to be executed as Cmd+Shift+ß. (That particular shortcut looks like it's completely unavailable with the US QWERTY layout - if I switch to that layout, the annotation Cmd+? vanishes from the "Nightly Help" menuitem, and Cmd+Shift+/ just opens the help menu itself.)
(Assignee)

Updated

2 years ago
Blocks: 1333781
(Assignee)

Comment 111

2 years ago
(In reply to Markus Stange [:mstange] from comment #110)
> We should watch out for reports of new keyboard shortcuts suddenly working
> when they did nothing in the past.
> Have you done any testing while looking out for these cases, or have you run
> into old bug reports of such cases while looking through the history of this
> code?

I have done quite a bit of testing but I can't possibly claim that I tested every shortcut or keyboard layout combination. I will keep an eye out for anything that comes in. As discussed on IRC the other day, this approach seemed like the most conservative approach that we could take at the moment. I just filed bug 1333781 where I outline two significantly riskier and more aggressive approaches that would support reassignment of existing shortcuts as well.

Comment 112

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2823fca4f547
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Carsten Book [:Tomcat] from comment #114)
> https://hg.mozilla.org/mozilla-central/rev/8b37b56c428a

backout also landed on m-c
Status: RESOLVED → REOPENED
Flags: needinfo?(spohl.mozilla.bugs)
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
(Assignee)

Comment 116

2 years ago
I was finally able to narrow bug 1334140 down to the fact that in e10s, we dispatch the keyboard event to the content process asynchronously and don't wait for the status of the event to change to nsEventStatus_eConsumeNoDefault, which means that at https://hg.mozilla.org/mozilla-central/annotate/2823fca4f547/widget/cocoa/nsChildView.mm#l5474, |handled| remains false and we end up sending the paste event a second time at https://hg.mozilla.org/mozilla-central/annotate/2823fca4f547/widget/cocoa/nsChildView.mm#l5487.

Mike or Markus, what is the proper way for the chrome process to wait for the content process to handle the event in e10s before proceeding?

As fyi, here is the callstack of the chrome process in e10s when we dispatch the event to the content process:

(lldb) bt
* thread #1: tid = 0xb06661, 0x00000001149bd312 XUL`mozilla::dom::TabParent::SendRealKeyEvent(this=0x000000018f38a800, event=0x00007fff4fec3430) + 722 at TabParent.cpp:1445, queue = 'com.apple.main-thread', stop reason = step over
  * frame #0: 0x00000001149bd312 XUL`mozilla::dom::TabParent::SendRealKeyEvent(this=0x000000018f38a800, event=0x00007fff4fec3430) + 722 at TabParent.cpp:1445
    frame #1: 0x0000000113c8c290 XUL`mozilla::EventStateManager::DispatchCrossProcessEvent(this=0x0000000137463980, aEvent=0x00007fff4fec3430, aFrameLoader=0x000000018c93d280, aStatus=0x00007fff4fec2ee8) + 272 at EventStateManager.cpp:1214
    frame #2: 0x0000000113c8caf4 XUL`mozilla::EventStateManager::HandleCrossProcessEvent(this=0x0000000137463980, aEvent=0x00007fff4fec3430, aStatus=0x00007fff4fec2ee8) + 1140 at EventStateManager.cpp:1402
    frame #3: 0x0000000113c917d2 XUL`mozilla::EventStateManager::PostHandleEvent(this=0x0000000137463980, aPresContext=0x000000013788c000, aEvent=0x00007fff4fec3430, aTargetFrame=0x000000014091cde8, aStatus=0x00007fff4fec2ee8) + 274 at EventStateManager.cpp:2927
    frame #4: 0x00000001152fb504 XUL`mozilla::PresShell::HandleEventInternal(this=0x0000000137a75800, aEvent=0x00007fff4fec3430, aStatus=0x00007fff4fec2ee8, aIsHandlingNativeEvent=true) + 2548 at PresShell.cpp:8070
    frame #5: 0x00000001152fa388 XUL`mozilla::PresShell::HandleEvent(this=0x0000000137a75800, aFrame=0x0000000137a05060, aEvent=0x00007fff4fec3430, aDontRetargetEvents=false, aEventStatus=0x00007fff4fec2ee8, aTargetContent=0x0000000000000000) + 11336 at PresShell.cpp:7748
    frame #6: 0x0000000114de114d XUL`nsViewManager::DispatchEvent(this=0x000000012d8831c0, aEvent=0x00007fff4fec3430, aView=0x0000000137a84c00, aStatus=0x00007fff4fec2ee8) + 637 at nsViewManager.cpp:814
    frame #7: 0x0000000114ddd45c XUL`nsView::HandleEvent(this=0x0000000137a84c00, aEvent=0x00007fff4fec3430, aUseAttachedEvents=false) + 300 at nsView.cpp:1125
    frame #8: 0x0000000114e69252 XUL`nsChildView::DispatchEvent(this=0x0000000137a75400, event=0x00007fff4fec3430, aStatus=0x00007fff4fec3124) + 1202 at nsChildView.mm:1443
    frame #9: 0x0000000114dec73b XUL`nsBaseWidget::ProcessUntransformedAPZEvent(this=0x0000000137a75400, aEvent=0x00007fff4fec3430, aGuid=0x00007fff4fec31f0, aInputBlockId=0, aApzResponse=nsEventStatus_eIgnore) + 363 at nsBaseWidget.cpp:1077
    frame #10: 0x0000000114decf9b XUL`nsBaseWidget::DispatchInputEvent(this=0x0000000137a75400, aEvent=0x00007fff4fec3430) + 267 at nsBaseWidget.cpp:1217
    frame #11: 0x0000000114e1d53f XUL`mozilla::widget::TextEventDispatcher::DispatchInputEvent(this=0x0000000137aa7dc0, aWidget=0x0000000137a75400, aEvent=0x00007fff4fec3430, aStatus=0x00007fff4fec379c) + 143 at TextEventDispatcher.cpp:206
    frame #12: 0x0000000114e1ea68 XUL`mozilla::widget::TextEventDispatcher::DispatchKeyboardEventInternal(this=0x0000000137aa7dc0, aMessage=eKeyPress, aKeyboardEvent=0x00007fff4fec37a0, aStatus=0x00007fff4fec379c, aData=0x00000001378be640, aIndexOfKeypress=0, aNeedsCallback=false) + 2952 at TextEventDispatcher.cpp:536
    frame #13: 0x0000000114e1ece0 XUL`mozilla::widget::TextEventDispatcher::MaybeDispatchKeypressEvents(this=0x0000000137aa7dc0, aKeyboardEvent=0x00007fff4fec37a0, aStatus=0x00007fff4fec379c, aData=0x00000001378be640, aNeedsCallback=false) + 448 at TextEventDispatcher.cpp:569
    frame #14: 0x0000000114ec5834 XUL`mozilla::widget::TextInputHandler::DoCommandBySelector(this=0x00000001378be600, aSelector="noop:") + 1252 at TextInputHandler.mm:2318
    frame #15: 0x0000000114e7d754 XUL`-[ChildView doCommandBySelector:](self=0x00000001375b6030, _cmd="doCommandBySelector:", aSelector="noop:") + 100 at nsChildView.mm:5296
    frame #16: 0x00007fff8f048616 AppKit`-[NSTextInputContext(NSInputContext_WithCompletion) doCommandBySelector:completionHandler:] + 118
    frame #17: 0x00007fff8f0242b5 AppKit`-[NSKeyBindingManager(NSKeyBindingManager_MultiClients) interpretEventAsCommand:forClient:] + 1867
    frame #18: 0x00007fff8f02d93b AppKit`__61-[NSTextInputContext _handleEvent:options:completionHandler:]_block_invoke978 + 335
    frame #19: 0x00007fff8f0484d6 AppKit`__61-[NSTextInputContext _handleEvent:options:completionHandler:]_block_invoke_3 + 95
    frame #20: 0x00007fff8f02d7ad AppKit`-[NSTextInputContext tryHandleEvent_HasMarkedText_withDispatchCondition:dispatchWork:continuation:] + 101
    frame #21: 0x00007fff8f02d737 AppKit`__61-[NSTextInputContext _handleEvent:options:completionHandler:]_block_invoke966 + 321
    frame #22: 0x00007fff933f2bb8 HIToolbox`__TSMProcessRawKeyEventWithOptionsAndCompletionHandler_block_invoke_5 + 70
    frame #23: 0x00007fff933f1b41 HIToolbox`___ZL23DispatchEventToHandlersP14EventTargetRecP14OpaqueEventRefP14HandlerCallRec_block_invoke + 108
    frame #24: 0x00007fff8f026ae8 AppKit`__55-[NSTextInputContext handleTSMEvent:completionHandler:]_block_invoke176 + 2866
    frame #25: 0x00007fff8f025ec8 AppKit`__55-[NSTextInputContext handleTSMEvent:completionHandler:]_block_invoke_2 + 95
    frame #26: 0x00007fff8f025e3b AppKit`-[NSTextInputContext tryHandleTSMEvent_HasMarkedText_withDispatchCondition:dispatchWork:continuation:] + 101
    frame #27: 0x00007fff8f025b2b AppKit`-[NSTextInputContext handleTSMEvent:completionHandler:] + 3173
    frame #28: 0x00007fff8f024e5a AppKit`_NSTSMEventHandler + 324
    frame #29: 0x00007fff9339a7be HIToolbox`DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*) + 1231
    frame #30: 0x00007fff93399c48 HIToolbox`SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*) + 404
    frame #31: 0x00007fff93399aab HIToolbox`SendEventToEventTargetWithOptions + 43
    frame #32: 0x00007fff933eef7d HIToolbox`SendTSMEvent_WithCompletionHandler + 417
    frame #33: 0x00007fff933ef46c HIToolbox`__SendUnicodeTextAEToUnicodeDoc_WithCompletionHandler_block_invoke + 400
    frame #34: 0x00007fff933ef2bf HIToolbox`__SendFilterTextEvent_WithCompletionHandler_block_invoke + 189
    frame #35: 0x00007fff933eefcd HIToolbox`SendTSMEvent_WithCompletionHandler + 497
    frame #36: 0x00007fff933eedaf HIToolbox`SendFilterTextEvent_WithCompletionHandler + 236
    frame #37: 0x00007fff933eea88 HIToolbox`SendUnicodeTextAEToUnicodeDoc_WithCompletionHandler + 284
    frame #38: 0x00007fff933ee838 HIToolbox`__utDeliverTSMEvent_WithCompletionHandler_block_invoke_2 + 296
    frame #39: 0x00007fff933ee6dc HIToolbox`__utDeliverTSMEvent_WithCompletionHandler_block_invoke + 437
    frame #40: 0x00007fff933ee4cf HIToolbox`TSMKeyEvent_WithCompletionHandler + 721
    frame #41: 0x00007fff933ee1c5 HIToolbox`__TSMProcessRawKeyEventWithOptionsAndCompletionHandler_block_invoke_4 + 251
    frame #42: 0x00007fff933ee033 HIToolbox`__TSMProcessRawKeyEventWithOptionsAndCompletionHandler_block_invoke_3 + 325
    frame #43: 0x00007fff933edd71 HIToolbox`__TSMProcessRawKeyEventWithOptionsAndCompletionHandler_block_invoke_2 + 261
    frame #44: 0x00007fff933edb3a HIToolbox`__TSMProcessRawKeyEventWithOptionsAndCompletionHandler_block_invoke + 253
    frame #45: 0x00007fff933ed1f0 HIToolbox`TSMProcessRawKeyEventWithOptionsAndCompletionHandler + 3199
    frame #46: 0x00007fff8f024d0b AppKit`__61-[NSTextInputContext _handleEvent:options:completionHandler:]_block_invoke955 + 131
    frame #47: 0x00007fff8f0239ed AppKit`-[NSTextInputContext tryTSMProcessRawKeyEvent:dispatchCondition:setupForDispatch:furtherCondition:dispatchWork:continuation:] + 131
    frame #48: 0x00007fff8f0236e7 AppKit`-[NSTextInputContext _handleEvent:options:completionHandler:] + 1266
    frame #49: 0x00007fff8f0231ba AppKit`-[NSTextInputContext handleEvent:] + 109
    frame #50: 0x00007fff8f0230c3 AppKit`-[NSView interpretKeyEvents:] + 204
    frame #51: 0x0000000114ebf4e6 XUL`mozilla::widget::TextInputHandler::HandleKeyDownEvent(this=0x00000001378be600, aNativeEvent=0x000000012d11fe20) + 3846 at TextInputHandler.mm:1625
    frame #52: 0x0000000114e7e8b6 XUL`-[ChildView keyDown:](self=0x00000001375b6030, _cmd="keyDown:", theEvent=0x000000012d11fe20) + 2166 at nsChildView.mm:5444
    frame #53: 0x00007fff8f64cb15 AppKit`-[NSWindow _reallySendEvent:isDelayedEvent:] + 2108
    frame #54: 0x00007fff8f08b539 AppKit`-[NSWindow sendEvent:] + 517
    frame #55: 0x0000000114ee7ffd XUL`-[ToolbarWindow sendEvent:](self=0x000000012ed97780, _cmd="sendEvent:", anEvent=0x000000012d11fe20) + 381 at nsCocoaWindow.mm:3644
    frame #56: 0x00007fff8f00c16a AppKit`-[NSApplication sendEvent:] + 4382
    frame #57: 0x0000000114ed21fd XUL`-[GeckoNSApplication sendEvent:](self=0x00000001102bfaa0, _cmd="sendEvent:", anEvent=0x000000012d11fe20) + 141 at nsAppShell.mm:110
    frame #58: 0x00007fff8ee72df2 AppKit`-[NSApplication run] + 796
    frame #59: 0x0000000114ed409c XUL`nsAppShell::Run(this=0x000000012c96f0d0) + 172 at nsAppShell.mm:666
    frame #60: 0x00000001163b7a1b XUL`nsAppStartup::Run(this=0x000000012c473470) + 155 at nsAppStartup.cpp:283
    frame #61: 0x00000001164f6764 XUL`XREMain::XRE_mainRun(this=0x00007fff4fec7058) + 6804 at nsAppRunner.cpp:4461
    frame #62: 0x00000001164f7864 XUL`XREMain::XRE_main(this=0x00007fff4fec7058, argc=1, argv=0x00007fff4fec76f8, aConfig=0x00007fff4fec7238) + 3012 at nsAppRunner.cpp:4638
    frame #63: 0x00000001164f7cac XUL`::XRE_main(argc=1, argv=0x00007fff4fec76f8, aConfig=0x00007fff4fec7238) + 92 at nsAppRunner.cpp:4729
    frame #64: 0x000000011650ee67 XUL`mozilla::BootstrapImpl::XRE_main(this=0x000000011025a0d8, argc=1, argv=0x00007fff4fec76f8, aConfig=0x00007fff4fec7238) + 39 at Bootstrap.cpp:45
    frame #65: 0x000000010fd39d46 firefox`do_main(argc=1, argv=0x00007fff4fec76f8, envp=0x00007fff4fec7708) + 758 at nsBrowserApp.cpp:241
    frame #66: 0x000000010fd397f1 firefox`main(argc=1, argv=0x00007fff4fec76f8, envp=0x00007fff4fec7708) + 161 at nsBrowserApp.cpp:347
    frame #67: 0x000000010fd39744 firefox`start + 52
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange)
Flags: needinfo?(mconley)
I _think_ there's a way of doing what you want here, but I don't know the specifics.

This flag in mozilla::BaseEventFlags looks relevant: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/widget/BasicEvents.h#117-121

You might want to search around for uses of mWantReplyFromContentProcess. Bug 862519 looks relevant.
Flags: needinfo?(mconley)

Comment 118

2 years ago
Yes, that's the right flag but it is a messy way, and you would need to do something different when there is no content process (which in this case you might not be able to determine).

I think that masayuki is trying to fix this up as part of bug 1257617.
(Assignee)

Comment 119

2 years ago
(In reply to Neil Deakin from comment #118)
> Yes, that's the right flag but it is a messy way, and you would need to do
> something different when there is no content process (which in this case you
> might not be able to determine).
> 
> I think that masayuki is trying to fix this up as part of bug 1257617.

Masayuki, should I wait for a patch in bug 1257617? Or do you know of a way to fix this isolated case beforehand?
Flags: needinfo?(masayuki)
Flags: needinfo?(mstange)
Looks like that this doesn't need to wait bug 1257617 since you can set mWantReplyFromContentProcess to true before dispatching keydown/keypress event if it will be executed by native menu (if it's possible).

# Anyway, bug 1257617 won't fix this case, because XP code doesn't check native menu on macOS if it should wait a reply from content process.
Flags: needinfo?(masayuki)
(Assignee)

Comment 124

2 years ago
I'm just requesting feedback (instead of a full review) on the current approach because the try run in comment 123 shows quite a number of failures. I'm still working through them. So far I've been able to either categorize them as existing intermittent failures, or the tests pass for me locally. This patch builds on top of the previous one.
Attachment #8853503 - Flags: feedback?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8829951 - Attachment description: Patch → 1. Patch
(Assignee)

Comment 128

2 years ago
This fixes the test failures on try and we're now green (see comment 127).
Attachment #8853503 - Attachment is obsolete: true
Attachment #8853503 - Flags: feedback?(mstange)
Attachment #8858985 - Flags: review?(mstange)
Comment on attachment 8858985 [details] [diff] [review]
2. Handle async event dispatch in e10s

Review of attachment 8858985 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good except for the nsXBLWindowKeyHandler::HandleEventOnCaptureInSystemEventGroup part.

Masayuki should review this patch (or the next version of it) as well.

::: dom/events/EventStateManager.cpp
@@ +2839,5 @@
>      return;
>    }
>  
> +  if (!dispatchedToContentProcess) {
> +    // We expect a reply from the content process. If the event wasn't

The first sentence seems out of place inside this if branch.

Maybe start with "The widget expects a reply for every keyboard event.", and then add an "Otherwise, we need to wait for the content process to handle the event." at the end?

::: dom/xbl/nsXBLWindowKeyHandler.cpp
@@ +510,3 @@
>  
>  void
>  nsXBLWindowKeyHandler::HandleEventOnCaptureInSystemEventGroup(

I don't really like how this function takes it on itself to find the widget and let it know about the event. Is there a way to use widgetEvent->mFlags to report a piece of information back to the EventStateManager, and then have the ESM call widget->PostHandleEvent?

::: widget/cocoa/nsChildView.mm
@@ +5519,5 @@
> +    sUniqueKeyEventId++;
> +    [sNativeKeyEventsMap setObject:theEvent forKey:@(sUniqueKeyEventId)];
> +    // Purge old native events, in case we're still holding on to them. We keep
> +    // at most 10 references to 10 different native events.
> +    [sNativeKeyEventsMap removeObjectForKey:@(sUniqueKeyEventId - 10)];

I like this idea.
Attachment #8858985 - Flags: review?(mstange)
(Assignee)

Comment 130

2 years ago
Updated for current trunk and addressed review feedback.

> ::: dom/xbl/nsXBLWindowKeyHandler.cpp
> @@ +510,3 @@
> >  
> >  void
> >  nsXBLWindowKeyHandler::HandleEventOnCaptureInSystemEventGroup(
> 
> I don't really like how this function takes it on itself to find the widget
> and let it know about the event. Is there a way to use widgetEvent->mFlags
> to report a piece of information back to the EventStateManager, and then
> have the ESM call widget->PostHandleEvent?

I was pretty sure you would raise this in the review, and I wasn't a fan of it either. I meant to get initial feedback on the patch and point this out as something that we could move elsewhere, but I forgot to do so when I ran into try failures and started looking into those.

I've moved this up to EventDispatcher, which is earlier in the call stack (see call stack below, using the old patch). But we don't actually go through EventStateManager here. Is this sufficient, or is there an even better place?

I'll be asking masayuki for a review once I've addressed all of your feedback. Thank you!

Call stack:
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000010e2bb497 XUL`nsXBLWindowKeyHandler::HandleEventOnCaptureInSystemEventGroup(this=0x000000013189efc0, aEvent=0x0000000122f0a110) at nsXBLWindowKeyHandler.cpp:556
    frame #1: 0x000000010e2bb145 XUL`nsXBLWindowKeyHandler::HandleEvent(this=0x000000013189efc0, aEvent=0x0000000122f0a110) at nsXBLWindowKeyHandler.cpp:444
    frame #2: 0x000000010d312491 XUL`mozilla::EventListenerManager::HandleEventSubType(this=0x000000012f1931f0, aListener=0x000000012f1e2920, aDOMEvent=0x0000000122f0a110, aCurrentTarget=0x000000012f192f20) at EventListenerManager.cpp:1137
    frame #3: 0x000000010d313180 XUL`mozilla::EventListenerManager::HandleEventInternal(this=0x000000012f1931f0, aPresContext=0x0000000131ab2000, aEvent=0x00007fff5accd4d8, aDOMEvent=0x00007fff5accd280, aCurrentTarget=0x000000012f192f20, aEventStatus=0x00007fff5accd288) at EventListenerManager.cpp:1311
    frame #4: 0x000000010d305d2d XUL`mozilla::EventListenerManager::HandleEvent(this=0x000000012f1931f0, aPresContext=0x0000000131ab2000, aEvent=0x00007fff5accd4d8, aDOMEvent=0x00007fff5accd280, aCurrentTarget=0x000000012f192f20, aEventStatus=0x00007fff5accd288) at EventListenerManager.h:374
    frame #5: 0x000000010d2fac78 XUL`mozilla::EventTargetChainItem::HandleEvent(this=0x0000000123fd3288, aVisitor=0x00007fff5accd270, aCd=0x00007fff5accd368) at EventDispatcher.cpp:315
    frame #6: 0x000000010d2fa548 XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5accd360, aVisitor=0x00007fff5accd270, aCallback=0x0000000000000000, aCd=0x00007fff5accd368) at EventDispatcher.cpp:442
    frame #7: 0x000000010d2fa8de XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5accd360, aVisitor=0x00007fff5accd270, aCallback=0x0000000000000000, aCd=0x00007fff5accd368) at EventDispatcher.cpp:517
    frame #8: 0x000000010d2fc1f2 XUL`mozilla::EventDispatcher::Dispatch(aTarget=0x0000000157ecd5e0, aPresContext=0x0000000131ab2000, aEvent=0x00007fff5accd4d8, aDOMEvent=0x0000000000000000, aEventStatus=0x0000000000000000, aCallback=0x0000000000000000, aTargets=0x0000000000000000) at EventDispatcher.cpp:825
    frame #9: 0x000000010e0e03e8 XUL`mozilla::dom::TabParent::RecvReplyKeyEvent(this=0x00000001582e7000, aEvent=0x00007fff5acce3c0) at TabParent.cpp:1999
    frame #10: 0x000000010adb00cc XUL`mozilla::dom::PBrowserParent::OnMessageReceived(this=0x00000001582e7000, msg__=0x000000010999f708) at PBrowserParent.cpp:3099
    frame #11: 0x000000010af1675f XUL`mozilla::dom::PContentParent::OnMessageReceived(this=0x00000001583ab000, msg__=0x000000010999f708) at PContentParent.cpp:3214
    frame #12: 0x000000010a7d1e03 XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage(this=0x00000001583ab138, aMsg=0x000000010999f708) at MessageChannel.cpp:2019
    frame #13: 0x000000010a7d047b XUL`mozilla::ipc::MessageChannel::DispatchMessage(this=0x00000001583ab138, aMsg=0x000000010999f708) at MessageChannel.cpp:1954
    frame #14: 0x000000010a7d1016 XUL`mozilla::ipc::MessageChannel::RunMessage(this=0x00000001583ab138, aTask=0x000000010999f6b0) at MessageChannel.cpp:1823
    frame #15: 0x000000010a7d1758 XUL`mozilla::ipc::MessageChannel::MessageTask::Run(this=0x000000010999f6b0) at MessageChannel.cpp:1856
    frame #16: 0x0000000109dc08ff XUL`nsThread::ProcessNextEvent(this=0x00000001081d4210, aMayWait=false, aResult=0x00007fff5acd3133) at nsThread.cpp:1270
    frame #17: 0x0000000109dbc99c XUL`NS_ProcessPendingEvents(aThread=0x00000001081d4210, aTimeout=10) at nsThreadUtils.cpp:335
    frame #18: 0x000000010e5752ee XUL`nsBaseAppShell::NativeEventCallback(this=0x00000001081a8af0) at nsBaseAppShell.cpp:97
    frame #19: 0x000000010e610cb2 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x00000001081a8af0) at nsAppShell.mm:399
    frame #20: 0x00007fff8445f3c1 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #21: 0x00007fff844402cd CoreFoundation`__CFRunLoopDoSources0 + 557
    frame #22: 0x00007fff8443f7c6 CoreFoundation`__CFRunLoopRun + 934
    frame #23: 0x00007fff8443f1c4 CoreFoundation`CFRunLoopRunSpecific + 420
    frame #24: 0x00007fff839a0ebc HIToolbox`RunCurrentEventLoopInMode + 240
    frame #25: 0x00007fff839a0cf1 HIToolbox`ReceiveNextEventCommon + 432
    frame #26: 0x00007fff839a0b26 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #27: 0x00007fff81f3be24 AppKit`_DPSNextEvent + 1120
    frame #28: 0x00007fff826b785e AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 2796
    frame #29: 0x000000010e60f7b4 XUL`::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x00000001052e76b0, _cmd="nextEventMatchingMask:untilDate:inMode:dequeue:", mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode="kCFRunLoopDefaultMode", flag=YES) at nsAppShell.mm:130
    frame #30: 0x00007fff81f307ab AppKit`-[NSApplication run] + 926
    frame #31: 0x000000010e61172c XUL`nsAppShell::Run(this=0x00000001081a8af0) at nsAppShell.mm:673
    frame #32: 0x0000000110ca996b XUL`nsAppStartup::Run(this=0x00000001226cc740) at nsAppStartup.cpp:283
    frame #33: 0x0000000110df8c4d XUL`XREMain::XRE_mainRun(this=0x00007fff5acd5398) at nsAppRunner.cpp:4540
    frame #34: 0x0000000110dfa1b3 XUL`XREMain::XRE_main(this=0x00007fff5acd5398, argc=5, argv=0x00007fff5acd5a30, aConfig=0x00007fff5acd5578) at nsAppRunner.cpp:4720
    frame #35: 0x0000000110dfa92c XUL`XRE_main(argc=5, argv=0x00007fff5acd5a30, aConfig=0x00007fff5acd5578) at nsAppRunner.cpp:4813
    frame #36: 0x0000000110e0ea57 XUL`mozilla::BootstrapImpl::XRE_main(this=0x0000000105213138, argc=5, argv=0x00007fff5acd5a30, aConfig=0x00007fff5acd5578) at Bootstrap.cpp:45
    frame #37: 0x0000000104f2b116 firefox`do_main(argc=5, argv=0x00007fff5acd5a30, envp=0x00007fff5acd5a60) at nsBrowserApp.cpp:236
    frame #38: 0x0000000104f2abf1 firefox`main(argc=5, argv=0x00007fff5acd5a30, envp=0x00007fff5acd5a60) at nsBrowserApp.cpp:307
    frame #39: 0x0000000104f2ab04 firefox`start + 52
Attachment #8858985 - Attachment is obsolete: true
Attachment #8862955 - Flags: review?(mstange)
(Assignee)

Comment 131

2 years ago
(In reply to Stephen A Pohl [:spohl] from comment #130)
> [...] 
> I've moved this up to EventDispatcher, which is earlier in the call stack

I meant to say EventListenerManager. EventDispatcher is another option even earlier in the call stack, but I'll wait for your feedback based on the call stack in comment 130 before I move it anywhere else. Thanks!
(Assignee)

Updated

2 years ago
Duplicate of this bug: 435863
(Assignee)

Updated

2 years ago
Duplicate of this bug: 827652
(Assignee)

Updated

2 years ago
Duplicate of this bug: 328746
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1015670
Comment on attachment 8862955 [details] [diff] [review]
2. Handle async event dispatch in e10s

Review of attachment 8862955 [details] [diff] [review]:
-----------------------------------------------------------------

I think TabParent::RecvReplyKeyEvent would be an even better place to handle this. You even have a GetWidget() method there.
(Assignee)

Comment 138

2 years ago
(In reply to Markus Stange [:mstange] from comment #136)
> I think TabParent::RecvReplyKeyEvent would be an even better place to handle
> this. You even have a GetWidget() method there.

Turns out this is much simpler and still passes all of my tests. Thank you!
Attachment #8862955 - Attachment is obsolete: true
Attachment #8862955 - Flags: review?(mstange)
Attachment #8864970 - Flags: review?(mstange)
Comment on attachment 8864970 [details] [diff] [review]
2. Handle async event dispatch in e10s

Review of attachment 8864970 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I think this is ready for masayuki to take a look.

::: dom/events/EventStateManager.cpp
@@ +2844,5 @@
> +    // The widget expects a reply for every keyboard event. If the event wasn't
> +    // dispatched to a content process (non-e10s or no content process
> +    // running), we need to short-circuit here. Otherwise, we need to wait for
> +    // the content process to handle the event.
> +    aKeyboardEvent->mWidget->PostHandleKeyEvent(aKeyboardEvent);

What happens if this code runs in the content process?
Attachment #8864970 - Flags: review?(mstange) → review+
(Assignee)

Updated

2 years ago
Attachment #8864970 - Flags: review?(masayuki)
(Assignee)

Comment 140

2 years ago
(In reply to Markus Stange [:mstange] from comment #139)
> Comment on attachment 8864970 [details] [diff] [review]
> ::: dom/events/EventStateManager.cpp
> @@ +2844,5 @@
> > +    // The widget expects a reply for every keyboard event. If the event wasn't
> > +    // dispatched to a content process (non-e10s or no content process
> > +    // running), we need to short-circuit here. Otherwise, we need to wait for
> > +    // the content process to handle the event.
> > +    aKeyboardEvent->mWidget->PostHandleKeyEvent(aKeyboardEvent);
> 
> What happens if this code runs in the content process?

I just confirmed that in the content process, this calls nsBaseWidget::PostHandleKeyEvent, which is a no-op.
Comment on attachment 8864970 [details] [diff] [review]
2. Handle async event dispatch in e10s

>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
> EventStateManager::PostHandleKeyboardEvent(WidgetKeyboardEvent* aKeyboardEvent,
>                                            nsEventStatus& aStatus,
>                                            bool dispatchedToContentProcess)
> {
>   if (aStatus == nsEventStatus_eConsumeNoDefault) {
>     return;
>   }
> 
>+  if (!dispatchedToContentProcess) {
>+    // The widget expects a reply for every keyboard event. If the event wasn't
>+    // dispatched to a content process (non-e10s or no content process
>+    // running), we need to short-circuit here. Otherwise, we need to wait for
>+    // the content process to handle the event.
>+    aKeyboardEvent->mWidget->PostHandleKeyEvent(aKeyboardEvent);
>+  }

If aKeyboardEvent is handled by nsIWidget::PostHandleKeyEvent(), aStatus should be changed to nsEventStatus_eConsumeNoDefault and won't be handled in following code.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>   if (localEvent.mFlags.mIsSuppressedOrDelayed) {
>     localEvent.PreventDefault();
>   }
> 
>   // If a response is desired from the content process, resend the key event.
>   // If mAccessKeyForwardedToChild is set, then don't resend the key event yet
>   // as RecvHandleAccessKey will do this.
>   if (localEvent.mFlags.mWantReplyFromContentProcess) {
>+    localEvent.mUniqueId = aEvent.mUniqueId;

mUniqueId may be useful if logging keyboard event in content process. So, please set it before dispatching the localEvent into the DOM tree.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>   NS_ENSURE_TRUE(presShell, IPC_OK());
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ENSURE_TRUE(presContext, IPC_OK());
> 
>   AutoHandlingUserInputStatePusher userInpStatePusher(localEvent.IsTrusted(),
>                                                       &localEvent, doc);
>-
>+#ifdef XP_MACOSX
>+  if (!localEvent.mFlags.mDefaultPrevented &&
>+      !localEvent.mFlags.mIsSynthesizedForTests) {
>+    nsCOMPtr<nsIWidget> widget = GetWidget();
>+    if (widget) {
>+      widget->PostHandleKeyEvent(&localEvent);
>+      localEvent.StopPropagation();
>+    }
>+  }
>+#endif

I don't think that we should use #ifdef here. Let's avoid to create platform specific behavior as far as possible.

And don't refer mDefaultPrevented directly, you should refer WidgetEvent::DefaultPrevented().

Additionally, the event will be dispatched into chrome DOM tree after here. That must be wrong. If it might be handled by UI or Add-ons, it should have higher priority than system settings. So, this block should be performed after EventDispatcher::Dispatch().

>diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
>--- a/widget/cocoa/TextInputHandler.h
>+++ b/widget/cocoa/TextInputHandler.h
>@@ -536,47 +536,54 @@ protected:
>     // Whether keypress event was consumed by web contents or chrome contents.
>     bool mKeyPressHandled;
>     // Whether the key event causes other key events via IME or something.
>     bool mCausedOtherKeyEvents;
>     // Whether the key event causes composition change or committing
>     // composition.  So, even if InsertText() is called, this may be false
>     // if it dispatches keypress event.
>     bool mCompositionDispatched;
>+    // Unique id associated with a keydown / keypress event. It's ok if this
>+    // wraps over long periods.
>+    uint32_t mUniqueId;

Please declare this between |nsString mInsertedString;| and |bool mKeyDownHandled;| for reducing the size of this struct.

> 
>-    KeyEventState() : mKeyEvent(nullptr)
>+    KeyEventState() : mKeyEvent(nullptr), mUniqueId(0)

nit: Could you rewrite this as:

KeEventState()
  : mKeyEvent(nullptr)
  , mUniqueId(0)
{

for the future reviews?

>-    explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
>+    explicit KeyEventState(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0) :
>+                             mKeyEvent(nullptr),
>+                             mUniqueId(0)
>     {

Same here. And I don't understand why aUniqueId should be optional argument here. If it's not, we can remove explicit keyword too.

>   /**
>    * PushKeyEvent() adds the current key event to mCurrentKeyEvents.
>    */
>-  KeyEventState* PushKeyEvent(NSEvent* aNativeKeyEvent)
>+  KeyEventState* PushKeyEvent(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0)

I don't understand why aUniqueId should be optional argument of this.

>@@ -1723,16 +1724,19 @@ TextInputHandler::HandleKeyDownEvent(NSE
>     // 1. If key events were nested during calling interpretKeyEvents, it means
>     //    that IME did something.  Then, we should do nothing.
>     // 2. If one or more commands are called like "deleteBackward", we should
>     //    dispatch keypress event at that time.  Note that the command may have
>     //    been a converted or generated action by IME.  Then, we shouldn't do
>     //    our default action for this key.
>     if (!(interpretKeyEventsCalled &&
>           IsNormalCharInputtingEvent(keypressEvent))) {
>+      // Inform the child process that this is an event that we want a reply
>+      // from.
>+      keypressEvent.mFlags.mWantReplyFromContentProcess = true;
>       currentKeyEvent->mKeyPressDispatched =
>         mDispatcher->MaybeDispatchKeypressEvents(keypressEvent, status,
>                                                  currentKeyEvent);
>       currentKeyEvent->mKeyPressHandled =
>         (status == nsEventStatus_eConsumeNoDefault);
>       currentKeyEvent->mKeyPressDispatched = true;
>       MOZ_LOG(gLog, LogLevel::Info,
>         ("%p TextInputHandler::HandleKeyDownEvent, keypress event dispatched",
>@@ -2527,16 +2531,20 @@ TextInputHandler::DoCommandBySelector(co
>            "FAILED, due to BeginNativeInputTransaction() failure "
>            "at dispatching keypress", this));
>       return Destroyed();
>     }
> 
>     WidgetKeyboardEvent keypressEvent(true, eKeyPress, widget);
>     currentKeyEvent->InitKeyEvent(this, keypressEvent);
> 
>+    // Inform the child process that this is an event that we want a reply
>+    // from.
>+    keypressEvent.mFlags.mWantReplyFromContentProcess = true;

Hmm, I don't like setting the flag specifically. Please do it in IMEInputHandler::WillDispatchKeyboardEvent(). I guess that you want to allow to fallback it to system shortcut keys only when:
* it doesn't cause text input
* it's not consumed by IME
* it's an eKeyPress event and first eKeyPress event of a key sequence (an eKeyDown may be followed by multiple eKeyPress events if it causes two or more characters)

So, I think that you should set it to true only when:
* aKeyboardEvent.mMessage is eKeyPress
* aIndexOfKeypress is 0
* insertString isn't empty string

Then, even if new eKeyPress event dispatcher is created, the implementer doesn't need to take care this bug.

>+void
>+nsChildView::PostHandleKeyEvent(const mozilla::WidgetKeyboardEvent* anEvent)
>+{

>+  bool handled = false;
>+  nsCocoaWindow* widget = GetXULWindowWidget();
>+  if (widget) {
>+    nsMenuBarX* mb = widget->GetMenuBar();
>+    if (mb) {
>+      // Check if main menu wants to handle the event.
>+      handled = mb->PerformKeyEquivalent(cocoaEvent);
>+    }
>+  }
>+
>+  if (!handled && sApplicationMenu) {
>+    // Check if application menu wants to handle the event.
>+    handled = [sApplicationMenu performKeyEquivalent:cocoaEvent];
>+  }

Why don't you share these blocks with keyDown:?

And if the event is handled, WidgetEvent::PeventDefault() should be called.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>     /**
>+     * Allows for handling of key events if they have not been handled by
>+     * content or XBL handlers.
>+     */
>+    virtual void
>+    PostHandleKeyEvent(const mozilla::WidgetKeyboardEvent* anEvent) = 0;

nit: s/anEvent/aEvent

I don't like the comment, "Allows for handling of key events". That's the purpose of this patch, not the method. So, please explain *when* this is called. Especially, this won't be called without eKeyPress event.

I think that the argument shouldn't be const.  If it's handled, the method implementation should be able to consume its default.

Finally, if you don't make this pure-virtual method, you can avoid to override this in nsBaseWidget (for avoiding rebuild time when TextEvents.h is changed, it should be implemented in nsBaseWidget.cpp, though).
Attachment #8864970 - Flags: review?(masayuki) → review-
(Assignee)

Comment 142

2 years ago
Thank you for your detailed review, Masayuki! I've addressed all of your feedback with the exception of the two points below. I was hoping to get some clarification before posting an updated patch. Thanks in advance for your feedback!

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #141)
> Comment on attachment 8864970 [details] [diff] [review]
> 2. Handle async event dispatch in e10s
> >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> >--- a/widget/cocoa/TextInputHandler.h
> >+++ b/widget/cocoa/TextInputHandler.h
> >@@ -536,47 +536,54 @@ protected:
> > [...]
> >-    explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
> >+    explicit KeyEventState(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0) :
> >+                             mKeyEvent(nullptr),
> >+                             mUniqueId(0)
> >     {
> 
> [...] And I don't understand why aUniqueId should be optional argument
> here. If it's not, we can remove explicit keyword too.

I made this optional because we're only interested in a unique ID for key down events. I didn't want to touch key up events unnecessarily. Should I change this?

> >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
> >   nsIDocument* doc = mFrameElement->OwnerDoc();
> >   nsIPresShell* presShell = doc->GetShell();
> >   NS_ENSURE_TRUE(presShell, IPC_OK());
> >   nsPresContext* presContext = presShell->GetPresContext();
> >   NS_ENSURE_TRUE(presContext, IPC_OK());
> > 
> >   AutoHandlingUserInputStatePusher userInpStatePusher(localEvent.IsTrusted(),
> >                                                       &localEvent, doc);
> >-
> >+#ifdef XP_MACOSX
> >+  if (!localEvent.mFlags.mDefaultPrevented &&
> >+      !localEvent.mFlags.mIsSynthesizedForTests) {
> >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> >+    if (widget) {
> >+      widget->PostHandleKeyEvent(&localEvent);
> >+      localEvent.StopPropagation();
> >+    }
> >+  }
> >+#endif
> 
> Additionally, the event will be dispatched into chrome DOM tree after here.
> That must be wrong. If it might be handled by UI or Add-ons, it should have
> higher priority than system settings. So, this block should be performed
> after EventDispatcher::Dispatch().

There are a couple of things here: This here only deals with the reply key event received by the parent (we're in TabParent::RecvReplyKeyEvent()). We only just started asking the child to dispatch a reply event in this patch, so there is no existing code that expects this to be handled in a different form. In previous version, I had this block after EventDispatcher::Dispatch() like you suggested. However, this didn't work because the event always came back with DefaultPrevented() being true. I just debugged this and we always call PreventDefault() on the event in nsXBLPrototypeHandler::DispatchXULKeyCommand(). See stack below. So, should we leave the sequence as it is in my patch because we didn't get reply key events before, or should we fix the fact that we call PreventDefault() in nsXBLPrototypeHandler::DispatchXULKeyCommand()? If the latter, could you tell me what the condition should be to either call or not call PreventDefault() on the event?

* thread #1, queue = 'com.apple.main-thread', stop reason = watchpoint 1
  * frame #0: 0x0000000111a2f835 XUL`mozilla::BaseEventFlags::PreventDefault(this=0x00007fff5e535494, aCalledByDefaultHandler=true) at BasicEvents.h:178
    frame #1: 0x0000000111a25165 XUL`mozilla::WidgetEvent::PreventDefault(this=0x00007fff5e535468, aCalledByDefaultHandler=true) at BasicEvents.h:457
    frame #2: 0x0000000111a5e612 XUL`mozilla::dom::Event::PreventDefaultInternal(this=0x0000000106346480, aCalledByDefaultHandler=true) at Event.cpp:509
    frame #3: 0x0000000111a5e48a XUL`mozilla::dom::Event::PreventDefault(this=0x0000000106346480) at Event.cpp:475
    frame #4: 0x0000000111aa2725 XUL`mozilla::dom::KeyboardEvent::PreventDefault(this=0x0000000106346480) at KeyboardEvent.h:32
    frame #5: 0x0000000112a162ee XUL`nsXBLPrototypeHandler::DispatchXULKeyCommand(this=0x00000001075d36c0, aEvent=0x0000000106346480) at nsXBLPrototypeHandler.cpp:516
    frame #6: 0x0000000112a000f7 XUL`nsXBLPrototypeHandler::ExecuteHandler(this=0x00000001075d36c0, aTarget=0x000000014be26280, aEvent=0x0000000106346480) at nsXBLPrototypeHandler.cpp:228
    frame #7: 0x0000000112a2e67f XUL`nsXBLWindowKeyHandler::WalkHandlersAndExecute(this=0x00000001062be280, aKeyEvent=0x0000000106346480, aEventType=0x0000000106392160, aFirstHandler=0x0000000107770900, aCharCode=107, aIgnoreModifierState=0x00007fff5e534860, aExecute=true, aOutReservedForChrome=0x0000000000000000) at nsXBLWindowKeyHandler.cpp:755
    frame #8: 0x0000000112a2cbac XUL`nsXBLWindowKeyHandler::WalkHandlersInternal(this=0x00000001062be280, aKeyEvent=0x0000000106346480, aEventType=0x0000000106392160, aHandler=0x0000000107770900, aExecute=true, aOutReservedForChrome=0x0000000000000000) at nsXBLWindowKeyHandler.cpp:627
    frame #9: 0x0000000112a2c903 XUL`nsXBLWindowKeyHandler::WalkHandlers(this=0x00000001062be280, aKeyEvent=0x0000000106346480, aEventType=0x0000000106392160) at nsXBLWindowKeyHandler.cpp:298
    frame #10: 0x0000000112a2dc4a XUL`nsXBLWindowKeyHandler::HandleEvent(this=0x00000001062be280, aEvent=0x0000000106346480) at nsXBLWindowKeyHandler.cpp:477
    frame #11: 0x0000000111a7aca1 XUL`mozilla::EventListenerManager::HandleEventSubType(this=0x000000010e14be90, aListener=0x000000010817e238, aDOMEvent=0x0000000106346480, aCurrentTarget=0x0000000104f57000) at EventListenerManager.cpp:1147
    frame #12: 0x0000000111a7b990 XUL`mozilla::EventListenerManager::HandleEventInternal(this=0x000000010e14be90, aPresContext=0x000000010e175000, aEvent=0x00007fff5e535468, aDOMEvent=0x00007fff5e535200, aCurrentTarget=0x0000000104f57000, aEventStatus=0x00007fff5e535208) at EventListenerManager.cpp:1321
    frame #13: 0x0000000111a6e56d XUL`mozilla::EventListenerManager::HandleEvent(this=0x000000010e14be90, aPresContext=0x000000010e175000, aEvent=0x00007fff5e535468, aDOMEvent=0x00007fff5e535200, aCurrentTarget=0x0000000104f57000, aEventStatus=0x00007fff5e535208) at EventListenerManager.h:376
    frame #14: 0x0000000111a634b8 XUL`mozilla::EventTargetChainItem::HandleEvent(this=0x0000000107ce0238, aVisitor=0x00007fff5e5351f0, aCd=0x00007fff5e5352e8) at EventDispatcher.cpp:315
    frame #15: 0x0000000111a62fdf XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5e5352e0, aVisitor=0x00007fff5e5351f0, aCallback=0x0000000000000000, aCd=0x00007fff5e5352e8) at EventDispatcher.cpp:488
    frame #16: 0x0000000111a6311e XUL`mozilla::EventTargetChainItem::HandleEventTargetChain(aChain=0x00007fff5e5352e0, aVisitor=0x00007fff5e5351f0, aCallback=0x0000000000000000, aCd=0x00007fff5e5352e8) at EventDispatcher.cpp:517
    frame #17: 0x0000000111a64a32 XUL`mozilla::EventDispatcher::Dispatch(aTarget=0x0000000108011f20, aPresContext=0x000000010e175000, aEvent=0x00007fff5e535468, aDOMEvent=0x0000000000000000, aEventStatus=0x0000000000000000, aCallback=0x0000000000000000, aTargets=0x0000000000000000) at EventDispatcher.cpp:825
    frame #18: 0x0000000112853248 XUL`mozilla::dom::TabParent::RecvReplyKeyEvent(this=0x000000010806a800, aEvent=0x00007fff5e5363b0) at TabParent.cpp:2004
[...]
    frame #47: 0x00000001016c2bf1 firefox`main(argc=5, argv=0x00007fff5e53da30, envp=0x00007fff5e53da60) at nsBrowserApp.cpp:307
    frame #48: 0x00000001016c2b04 firefox`start + 52
Flags: needinfo?(masayuki)
(In reply to Stephen A Pohl [:spohl] from comment #142)
> Thank you for your detailed review, Masayuki! I've addressed all of your
> feedback with the exception of the two points below. I was hoping to get
> some clarification before posting an updated patch. Thanks in advance for
> your feedback!
> 
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #141)
> > Comment on attachment 8864970 [details] [diff] [review]
> > 2. Handle async event dispatch in e10s
> > >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> > >--- a/widget/cocoa/TextInputHandler.h
> > >+++ b/widget/cocoa/TextInputHandler.h
> > >@@ -536,47 +536,54 @@ protected:
> > > [...]
> > >-    explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
> > >+    explicit KeyEventState(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0) :
> > >+                             mKeyEvent(nullptr),
> > >+                             mUniqueId(0)
> > >     {
> > 
> > [...] And I don't understand why aUniqueId should be optional argument
> > here. If it's not, we can remove explicit keyword too.
> 
> I made this optional because we're only interested in a unique ID for key
> down events. I didn't want to touch key up events unnecessarily. Should I
> change this?

KeyEventState() is created only in PushKeyEvent() and it's called only by HandleKeyDown(). So, nobody won't omit the optional argument in this case.

> > >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
> > >   nsIDocument* doc = mFrameElement->OwnerDoc();
> > >   nsIPresShell* presShell = doc->GetShell();
> > >   NS_ENSURE_TRUE(presShell, IPC_OK());
> > >   nsPresContext* presContext = presShell->GetPresContext();
> > >   NS_ENSURE_TRUE(presContext, IPC_OK());
> > > 
> > >   AutoHandlingUserInputStatePusher userInpStatePusher(localEvent.IsTrusted(),
> > >                                                       &localEvent, doc);
> > >-
> > >+#ifdef XP_MACOSX
> > >+  if (!localEvent.mFlags.mDefaultPrevented &&
> > >+      !localEvent.mFlags.mIsSynthesizedForTests) {
> > >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> > >+    if (widget) {
> > >+      widget->PostHandleKeyEvent(&localEvent);
> > >+      localEvent.StopPropagation();
> > >+    }
> > >+  }
> > >+#endif
> > 
> > Additionally, the event will be dispatched into chrome DOM tree after here.
> > That must be wrong. If it might be handled by UI or Add-ons, it should have
> > higher priority than system settings. So, this block should be performed
> > after EventDispatcher::Dispatch().
> 
> There are a couple of things here: This here only deals with the reply key
> event received by the parent (we're in TabParent::RecvReplyKeyEvent()). We
> only just started asking the child to dispatch a reply event in this patch,
> so there is no existing code that expects this to be handled in a different
> form. In previous version, I had this block after
> EventDispatcher::Dispatch() like you suggested. However, this didn't work
> because the event always came back with DefaultPrevented() being true.

Hmm, always? Sounds odd...

> I just debugged this and we always call PreventDefault() on the event in
> nsXBLPrototypeHandler::DispatchXULKeyCommand(). See stack below. So, should
> we leave the sequence as it is in my patch because we didn't get reply key
> events before, or should we fix the fact that we call PreventDefault() in
> nsXBLPrototypeHandler::DispatchXULKeyCommand()? If the latter, could you
> tell me what the condition should be to either call or not call
> PreventDefault() on the event?

I like the latter unless it hits serious problem (e.g., needs big design change, etc).

According to your stack trace, nsXBLWindowKeyHandler::WalkHandlersAndExecute() found a <key> element which matches with the keyboard event. Is there a shortcut key which you test? If yes, how about a shortcut keys which are not used by Firefox? Meeting a XUL key handler with the keyboard event, preventing its default must be correct thing.

On the other hand, I'm confused at the former case. If shortcut key is conflict with Firefox's and System's, should we prefer which one? I thought it's former.
Flags: needinfo?(masayuki)
(Assignee)

Comment 144

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #143)
> (In reply to Stephen A Pohl [:spohl] from comment #142)
> > Thank you for your detailed review, Masayuki! I've addressed all of your
> > feedback with the exception of the two points below. I was hoping to get
> > some clarification before posting an updated patch. Thanks in advance for
> > your feedback!
> > 
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> > comment #141)
> > > Comment on attachment 8864970 [details] [diff] [review]
> > > 2. Handle async event dispatch in e10s
> > > >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> > > >--- a/widget/cocoa/TextInputHandler.h
> > > >+++ b/widget/cocoa/TextInputHandler.h
> > > >@@ -536,47 +536,54 @@ protected:
> > > > [...]
> > > >-    explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
> > > >+    explicit KeyEventState(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0) :
> > > >+                             mKeyEvent(nullptr),
> > > >+                             mUniqueId(0)
> > > >     {
> > > 
> > > [...] And I don't understand why aUniqueId should be optional argument
> > > here. If it's not, we can remove explicit keyword too.
> > 
> > I made this optional because we're only interested in a unique ID for key
> > down events. I didn't want to touch key up events unnecessarily. Should I
> > change this?
> 
> KeyEventState() is created only in PushKeyEvent() and it's called only by
> HandleKeyDown(). So, nobody won't omit the optional argument in this case.

I was referring to the two instances where KeyEventState objects are created on the stack, in TextInputHandler::HandleKeyUpEvent[1] and TextInputHandler::DispatchKeyEventForFlagsChanged[2]. We don't currently have a meaningful unique ID there. Would you prefer that I pass 0 for aUniqueId in those instances?

[1] http://searchfox.org/mozilla-central/source/widget/cocoa/TextInputHandler.mm#1795
[2] http://searchfox.org/mozilla-central/source/widget/cocoa/TextInputHandler.mm#2165

> > > >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
> > > >   nsIDocument* doc = mFrameElement->OwnerDoc();
> > > >   nsIPresShell* presShell = doc->GetShell();
> > > >   NS_ENSURE_TRUE(presShell, IPC_OK());
> > > >   nsPresContext* presContext = presShell->GetPresContext();
> > > >   NS_ENSURE_TRUE(presContext, IPC_OK());
> > > > 
> > > >   AutoHandlingUserInputStatePusher userInpStatePusher(localEvent.IsTrusted(),
> > > >                                                       &localEvent, doc);
> > > >-
> > > >+#ifdef XP_MACOSX
> > > >+  if (!localEvent.mFlags.mDefaultPrevented &&
> > > >+      !localEvent.mFlags.mIsSynthesizedForTests) {
> > > >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> > > >+    if (widget) {
> > > >+      widget->PostHandleKeyEvent(&localEvent);
> > > >+      localEvent.StopPropagation();
> > > >+    }
> > > >+  }
> > > >+#endif
> > > 
> > > Additionally, the event will be dispatched into chrome DOM tree after here.
> > > That must be wrong. If it might be handled by UI or Add-ons, it should have
> > > higher priority than system settings. So, this block should be performed
> > > after EventDispatcher::Dispatch().
> > 
> > There are a couple of things here: This here only deals with the reply key
> > event received by the parent (we're in TabParent::RecvReplyKeyEvent()). We
> > only just started asking the child to dispatch a reply event in this patch,
> > so there is no existing code that expects this to be handled in a different
> > form. In previous version, I had this block after
> > EventDispatcher::Dispatch() like you suggested. However, this didn't work
> > because the event always came back with DefaultPrevented() being true.
> 
> Hmm, always? Sounds odd...
> 
> > I just debugged this and we always call PreventDefault() on the event in
> > nsXBLPrototypeHandler::DispatchXULKeyCommand(). See stack below. So, should
> > we leave the sequence as it is in my patch because we didn't get reply key
> > events before, or should we fix the fact that we call PreventDefault() in
> > nsXBLPrototypeHandler::DispatchXULKeyCommand()? If the latter, could you
> > tell me what the condition should be to either call or not call
> > PreventDefault() on the event?
> 
> I like the latter unless it hits serious problem (e.g., needs big design
> change, etc).
> 
> According to your stack trace,
> nsXBLWindowKeyHandler::WalkHandlersAndExecute() found a <key> element which
> matches with the keyboard event. Is there a shortcut key which you test? If
> yes, how about a shortcut keys which are not used by Firefox? Meeting a XUL
> key handler with the keyboard event, preventing its default must be correct
> thing.
> 
> On the other hand, I'm confused at the former case. If shortcut key is
> conflict with Firefox's and System's, should we prefer which one? I thought
> it's former.

The part that I haven't been able to figure out here is: if there was a <key> element which matches a keyboard event, I believe it should also have been found by the child process when it was first processing the event. Why would the child process be unable to find an XBL handler, reply with a reply key event that is not DefaultPrevented(), but then the parent process finds an XBL handler and calls PreventDefault() on the event? Shouldn't the XBL handlers be the same between child and parent process? I have mostly been testing with a custom shortcut of CMD + K because I didn't think we had a built-in handler for this shortcut.
Flags: needinfo?(masayuki)
(In reply to Stephen A Pohl [:spohl] from comment #144)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #143)
> > (In reply to Stephen A Pohl [:spohl] from comment #142)
> > > Thank you for your detailed review, Masayuki! I've addressed all of your
> > > feedback with the exception of the two points below. I was hoping to get
> > > some clarification before posting an updated patch. Thanks in advance for
> > > your feedback!
> > > 
> > > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> > > comment #141)
> > > > Comment on attachment 8864970 [details] [diff] [review]
> > > > 2. Handle async event dispatch in e10s
> > > > >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> > > > >--- a/widget/cocoa/TextInputHandler.h
> > > > >+++ b/widget/cocoa/TextInputHandler.h
> > > > >@@ -536,47 +536,54 @@ protected:
> > > > > [...]
> > > > >-    explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
> > > > >+    explicit KeyEventState(NSEvent* aNativeKeyEvent, uint32_t aUniqueId = 0) :
> > > > >+                             mKeyEvent(nullptr),
> > > > >+                             mUniqueId(0)
> > > > >     {
> > > > 
> > > > [...] And I don't understand why aUniqueId should be optional argument
> > > > here. If it's not, we can remove explicit keyword too.
> > > 
> > > I made this optional because we're only interested in a unique ID for key
> > > down events. I didn't want to touch key up events unnecessarily. Should I
> > > change this?
> > 
> > KeyEventState() is created only in PushKeyEvent() and it's called only by
> > HandleKeyDown(). So, nobody won't omit the optional argument in this case.
> 
> I was referring to the two instances where KeyEventState objects are created
> on the stack, in TextInputHandler::HandleKeyUpEvent[1] and
> TextInputHandler::DispatchKeyEventForFlagsChanged[2]. We don't currently
> have a meaningful unique ID there. Would you prefer that I pass 0 for
> aUniqueId in those instances?
> 
> [1]
> http://searchfox.org/mozilla-central/source/widget/cocoa/TextInputHandler.
> mm#1795
> [2]
> http://searchfox.org/mozilla-central/source/widget/cocoa/TextInputHandler.
> mm#2165

Oh, thanks. I forgot creating the instance on the stack. Okay, fine to use optional argument.

> > > > >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
> > > > >   nsIDocument* doc = mFrameElement->OwnerDoc();
> > > > >   nsIPresShell* presShell = doc->GetShell();
> > > > >   NS_ENSURE_TRUE(presShell, IPC_OK());
> > > > >   nsPresContext* presContext = presShell->GetPresContext();
> > > > >   NS_ENSURE_TRUE(presContext, IPC_OK());
> > > > > 
> > > > >   AutoHandlingUserInputStatePusher userInpStatePusher(localEvent.IsTrusted(),
> > > > >                                                       &localEvent, doc);
> > > > >-
> > > > >+#ifdef XP_MACOSX
> > > > >+  if (!localEvent.mFlags.mDefaultPrevented &&
> > > > >+      !localEvent.mFlags.mIsSynthesizedForTests) {
> > > > >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> > > > >+    if (widget) {
> > > > >+      widget->PostHandleKeyEvent(&localEvent);
> > > > >+      localEvent.StopPropagation();
> > > > >+    }
> > > > >+  }
> > > > >+#endif
> > > > 
> > > > Additionally, the event will be dispatched into chrome DOM tree after here.
> > > > That must be wrong. If it might be handled by UI or Add-ons, it should have
> > > > higher priority than system settings. So, this block should be performed
> > > > after EventDispatcher::Dispatch().
> > > 
> > > There are a couple of things here: This here only deals with the reply key
> > > event received by the parent (we're in TabParent::RecvReplyKeyEvent()). We
> > > only just started asking the child to dispatch a reply event in this patch,
> > > so there is no existing code that expects this to be handled in a different
> > > form. In previous version, I had this block after
> > > EventDispatcher::Dispatch() like you suggested. However, this didn't work
> > > because the event always came back with DefaultPrevented() being true.
> > 
> > Hmm, always? Sounds odd...
> > 
> > > I just debugged this and we always call PreventDefault() on the event in
> > > nsXBLPrototypeHandler::DispatchXULKeyCommand(). See stack below. So, should
> > > we leave the sequence as it is in my patch because we didn't get reply key
> > > events before, or should we fix the fact that we call PreventDefault() in
> > > nsXBLPrototypeHandler::DispatchXULKeyCommand()? If the latter, could you
> > > tell me what the condition should be to either call or not call
> > > PreventDefault() on the event?
> > 
> > I like the latter unless it hits serious problem (e.g., needs big design
> > change, etc).
> > 
> > According to your stack trace,
> > nsXBLWindowKeyHandler::WalkHandlersAndExecute() found a <key> element which
> > matches with the keyboard event. Is there a shortcut key which you test? If
> > yes, how about a shortcut keys which are not used by Firefox? Meeting a XUL
> > key handler with the keyboard event, preventing its default must be correct
> > thing.
> > 
> > On the other hand, I'm confused at the former case. If shortcut key is
> > conflict with Firefox's and System's, should we prefer which one? I thought
> > it's former.
> 
> The part that I haven't been able to figure out here is: if there was a
> <key> element which matches a keyboard event, I believe it should also have
> been found by the child process when it was first processing the event. Why
> would the child process be unable to find an XBL handler, reply with a reply
> key event that is not DefaultPrevented(), but then the parent process finds
> an XBL handler and calls PreventDefault() on the event? Shouldn't the XBL
> handlers be the same between child and parent process?

Ah, sorry, I realized an important thing about TabParent::RecvReplyKeyEvent().

First, shortcut keys of Firefox UI are registered only in chrome process (IIUC). When a keydown/keypress is dispatched into the DOM tree in the chrome process, top level window checks if it matches a <key> element or something. If it matches, nsXBLWindowKeyHandler marks the event as "wants reply from the remote process", check |mWantReplyFromContentProcess|. Then, its propagation is stopped in the parent process.  Then, EventStateManager dispatches the event to focused remote process, and PuppetWidget dispatches the event in to the DOM tree in the remote process. Finally, PuppetWidget in the remote process sends the result to TabParent when mWantReplyFromContentProcess is true.

So, in other words, when TabParent::RecvReplyKeyEvent() is called, the key event should have matched with a <key> element or something. So, in such case, I think that we shouldn't execute the customized shortcut key. So, moving the code after EventDispatcher::DispatchEvent() must be correct.

> I have mostly been
> testing with a custom shortcut of CMD + K because I didn't think we had a
> built-in handler for this shortcut.

On Windows, Accel+K sets focus to the searchbar. I don't know about Firefox for macOS (I don't find the definition yet). How about Cmd+Opt+(Enter|Backspace|Escape, etc)? Almost all of Cmd+foo are already used.
Flags: needinfo?(masayuki)
Oops, but your patch does set mWantReplyFromContentProcess to true before dispatch. I forgot that.

So, I'd like you to test it with other key combination. I guess Cmd+K isn't a good example.
(Assignee)

Comment 148

2 years ago
I sent this patch to try (see comment 147) but there is a huge backlog of test runs for OSX. I'm sending this for review under the assumption that the tests for OSX will come back green.

There was one remaining question, see below:

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #141)
> Comment on attachment 8864970 [details] [diff] [review]
> 2. Handle async event dispatch in e10s
> 
> >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> >--- a/widget/cocoa/TextInputHandler.h
> >+++ b/widget/cocoa/TextInputHandler.h
> >@@ -1723,16 +1724,19 @@ TextInputHandler::HandleKeyDownEvent(NSE
> >     // 1. If key events were nested during calling interpretKeyEvents, it means
> >     //    that IME did something.  Then, we should do nothing.
> >     // 2. If one or more commands are called like "deleteBackward", we should
> >     //    dispatch keypress event at that time.  Note that the command may have
> >     //    been a converted or generated action by IME.  Then, we shouldn't do
> >     //    our default action for this key.
> >     if (!(interpretKeyEventsCalled &&
> >           IsNormalCharInputtingEvent(keypressEvent))) {
> >+      // Inform the child process that this is an event that we want a reply
> >+      // from.
> >+      keypressEvent.mFlags.mWantReplyFromContentProcess = true;
> >       currentKeyEvent->mKeyPressDispatched =
> >         mDispatcher->MaybeDispatchKeypressEvents(keypressEvent, status,
> >                                                  currentKeyEvent);
> >       currentKeyEvent->mKeyPressHandled =
> >         (status == nsEventStatus_eConsumeNoDefault);
> >       currentKeyEvent->mKeyPressDispatched = true;
> >       MOZ_LOG(gLog, LogLevel::Info,
> >         ("%p TextInputHandler::HandleKeyDownEvent, keypress event dispatched",
> >@@ -2527,16 +2531,20 @@ TextInputHandler::DoCommandBySelector(co
> >            "FAILED, due to BeginNativeInputTransaction() failure "
> >            "at dispatching keypress", this));
> >       return Destroyed();
> >     }
> > 
> >     WidgetKeyboardEvent keypressEvent(true, eKeyPress, widget);
> >     currentKeyEvent->InitKeyEvent(this, keypressEvent);
> > 
> >+    // Inform the child process that this is an event that we want a reply
> >+    // from.
> >+    keypressEvent.mFlags.mWantReplyFromContentProcess = true;
> 
> Hmm, I don't like setting the flag specifically. Please do it in
> IMEInputHandler::WillDispatchKeyboardEvent(). I guess that you want to allow
> to fallback it to system shortcut keys only when:
> * it doesn't cause text input
> * it's not consumed by IME
> * it's an eKeyPress event and first eKeyPress event of a key sequence (an
> eKeyDown may be followed by multiple eKeyPress events if it causes two or
> more characters)
> 
> So, I think that you should set it to true only when:
> * aKeyboardEvent.mMessage is eKeyPress
> * aIndexOfKeypress is 0
> * insertString isn't empty string

Could you clarify what you meant by "insertString isn't empty string" here? I assumed that this meant that we should check that insertString wasn't null and that insertString->Length() > 0. However, I found that insertString was always null, no matter the custom shortcut combination that I chose. I've tried Cmd + Option + Return, Cmd + Option + Up Arrow, Cmd + Option + Shift + L,  Cmd + Option + Shift + N etc. This patch currently doesn't check insertString at all and this seems to work as expected, but I wanted to double-check with you if this did the right thing, or if there is a bug and insertString shouldn't be null here.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #146)
> Oops, but your patch does set mWantReplyFromContentProcess to true before
> dispatch. I forgot that.
> 
> So, I'd like you to test it with other key combination. I guess Cmd+K isn't
> a good example.

You were right, it wasn't! I went through all the menus and couldn't see a shortcut of Cmd + K, but I now realize that this was a hidden shortcut that activates the search bar. The same must have been true for many other custom shortcuts that I tested. Thank you!
Attachment #8864970 - Attachment is obsolete: true
Attachment #8867022 - Flags: review?(masayuki)
(In reply to Stephen A Pohl [:spohl] from comment #148)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #141)
> > Comment on attachment 8864970 [details] [diff] [review]
> > 2. Handle async event dispatch in e10s
> > 
> > >diff --git a/widget/cocoa/TextInputHandler.h b/widget/cocoa/TextInputHandler.h
> > >--- a/widget/cocoa/TextInputHandler.h
> > >+++ b/widget/cocoa/TextInputHandler.h
> > >@@ -1723,16 +1724,19 @@ TextInputHandler::HandleKeyDownEvent(NSE
> > >     // 1. If key events were nested during calling interpretKeyEvents, it means
> > >     //    that IME did something.  Then, we should do nothing.
> > >     // 2. If one or more commands are called like "deleteBackward", we should
> > >     //    dispatch keypress event at that time.  Note that the command may have
> > >     //    been a converted or generated action by IME.  Then, we shouldn't do
> > >     //    our default action for this key.
> > >     if (!(interpretKeyEventsCalled &&
> > >           IsNormalCharInputtingEvent(keypressEvent))) {
> > >+      // Inform the child process that this is an event that we want a reply
> > >+      // from.
> > >+      keypressEvent.mFlags.mWantReplyFromContentProcess = true;
> > >       currentKeyEvent->mKeyPressDispatched =
> > >         mDispatcher->MaybeDispatchKeypressEvents(keypressEvent, status,
> > >                                                  currentKeyEvent);
> > >       currentKeyEvent->mKeyPressHandled =
> > >         (status == nsEventStatus_eConsumeNoDefault);
> > >       currentKeyEvent->mKeyPressDispatched = true;
> > >       MOZ_LOG(gLog, LogLevel::Info,
> > >         ("%p TextInputHandler::HandleKeyDownEvent, keypress event dispatched",
> > >@@ -2527,16 +2531,20 @@ TextInputHandler::DoCommandBySelector(co
> > >            "FAILED, due to BeginNativeInputTransaction() failure "
> > >            "at dispatching keypress", this));
> > >       return Destroyed();
> > >     }
> > > 
> > >     WidgetKeyboardEvent keypressEvent(true, eKeyPress, widget);
> > >     currentKeyEvent->InitKeyEvent(this, keypressEvent);
> > > 
> > >+    // Inform the child process that this is an event that we want a reply
> > >+    // from.
> > >+    keypressEvent.mFlags.mWantReplyFromContentProcess = true;
> > 
> > Hmm, I don't like setting the flag specifically. Please do it in
> > IMEInputHandler::WillDispatchKeyboardEvent(). I guess that you want to allow
> > to fallback it to system shortcut keys only when:
> > * it doesn't cause text input
> > * it's not consumed by IME
> > * it's an eKeyPress event and first eKeyPress event of a key sequence (an
> > eKeyDown may be followed by multiple eKeyPress events if it causes two or
> > more characters)
> > 
> > So, I think that you should set it to true only when:
> > * aKeyboardEvent.mMessage is eKeyPress
> > * aIndexOfKeypress is 0
> > * insertString isn't empty string
> 
> Could you clarify what you meant by "insertString isn't empty string" here?
> I assumed that this meant that we should check that insertString wasn't null
> and that insertString->Length() > 0.

I meant that mInsertedString of KeyEventState is empty.
https://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/widget/cocoa/TextInputHandler.mm#4748,4759

> However, I found that insertString was
> always null, no matter the custom shortcut combination that I chose. I've
> tried Cmd + Option + Return, Cmd + Option + Up Arrow, Cmd + Option + Shift +
> L,  Cmd + Option + Shift + N etc. This patch currently doesn't check
> insertString at all and this seems to work as expected, but I wanted to
> double-check with you if this did the right thing, or if there is a bug and
> insertString shouldn't be null here.

If it's not empty, the key combination caused inputting some text. I think that key combination which causes text input shouldn't work as shortcut key. (Although, it can work as shortcut key when focus isn't in any editor, but it may be error prone for the user. Additionally, marking keypress event as "want reply" makes the propagation cost increased. Especially, it takes longer time to perform proper command in chrome process since it needs to be dispatched to remove process.)

In other words, when you press Cmd, as far as I know, any keyboard layout doesn't input text. So, it always empty string in such case.
(Assignee)

Comment 150

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #149)
> I meant that mInsertedString of KeyEventState is empty.

Thank you. I'm taking this to mean that mInsertString is either null or empty. I've updated the patch accordingly and will send to try for OSX only.
Attachment #8867022 - Attachment is obsolete: true
Attachment #8867022 - Flags: review?(masayuki)
Attachment #8867034 - Flags: review?(masayuki)
Comment on attachment 8867034 [details] [diff] [review]
2. Handle async event dispatch in e10s

>@@ -2802,16 +2803,22 @@ IMEInputHandler::WillDispatchKeyboardEve
>   // nothing here.
>   if (!aData) {
>     return;
>   }
> 
>   KeyEventState* currentKeyEvent = static_cast<KeyEventState*>(aData);
>   NSEvent* nativeEvent = currentKeyEvent->mKeyEvent;
>   nsAString* insertString = currentKeyEvent->mInsertString;
>+  if (aKeyboardEvent.mMessage == eKeyPress && aIndexOfKeypress == 0 &&
>+      (!insertString || insertString->Length() == 0)) {

Use nsAString::IsEmpty() instead of Length() == 0.

>+    // Inform the child process that this is an event that we want a reply
>+    // from.
>+    aKeyboardEvent.mFlags.mWantReplyFromContentProcess = true;
>+  }

I think that in |else| case for this (and aIndexOfKeypress == 0), don't you need to remove the native key event from sNativeKeyEventsMap?

Additionally, some key events don't cause keypress events, e.g.,

* when WidgetKeyboardEvent::ShouldCauseKeypressEvents() returns false.
* when there is composition.

Additionally, nsIWidget::PostHandleKeyEvent() may not be called even if it sets mWantReplyFromContentProcess to true, e.g.,

* when the remote process is crashed during keypress.
* TabParent is detached before receiving the reply from remote process.

>+void
>+nsChildView::PostHandleKeyEvent(mozilla::WidgetKeyboardEvent* aEvent)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  // We always allow keyboard events to propagate to keyDown: but if they are
>+  // not handled we give menu items a chance to act. This allows for handling of
>+  // custom shortcuts. Note that existing shortcuts cannot be reassigned yet and
>+  // will have been handled by keyDown: before we get here.
>+  NSEvent* cocoaEvent =
>+    [sNativeKeyEventsMap objectForKey:@(aEvent->mUniqueId)];
>+  [sNativeKeyEventsMap removeObjectForKey:@(aEvent->mUniqueId)];
>+  if (!cocoaEvent) {
>+    return;
>+  }

So, I think there should be sHandledUniqueKeyEventId. Then, PostHandleEvent() should remove native events between (sHandledUniqueKeyEventId + 1) and (aEvent->mUniqueId).


Sorry for that I didn't realize this issue at previous review.
Attachment #8867034 - Flags: review?(masayuki) → review-
(Assignee)

Comment 153

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #152)
> Comment on attachment 8867034 [details] [diff] [review]
> 2. Handle async event dispatch in e10s
> 
> >@@ -2802,16 +2803,22 @@ IMEInputHandler::WillDispatchKeyboardEve
> >   // nothing here.
> >   if (!aData) {
> >     return;
> >   }
> > 
> >   KeyEventState* currentKeyEvent = static_cast<KeyEventState*>(aData);
> >   NSEvent* nativeEvent = currentKeyEvent->mKeyEvent;
> >   nsAString* insertString = currentKeyEvent->mInsertString;
> >+  if (aKeyboardEvent.mMessage == eKeyPress && aIndexOfKeypress == 0 &&
> >+      (!insertString || insertString->Length() == 0)) {
> 
> Use nsAString::IsEmpty() instead of Length() == 0.

Fixed.

> >+    // Inform the child process that this is an event that we want a reply
> >+    // from.
> >+    aKeyboardEvent.mFlags.mWantReplyFromContentProcess = true;
> >+  }
> 
> I think that in |else| case for this (and aIndexOfKeypress == 0), don't you
> need to remove the native key event from sNativeKeyEventsMap?

If keyboard events are dispatched in fast succession, I think we still want to the native event to be around when the child process sends its reply key event to a previous key event. We would like to still dispatch previous native key events to the native menu system in this scenario.

> Additionally, some key events don't cause keypress events, e.g.,
> 
> * when WidgetKeyboardEvent::ShouldCauseKeypressEvents() returns false.
> * when there is composition.

I haven't noticed this to cause any issues, but would you like me to handle this somehow? 

> Additionally, nsIWidget::PostHandleKeyEvent() may not be called even if it
> sets mWantReplyFromContentProcess to true, e.g.,
> 
> * when the remote process is crashed during keypress.
> * TabParent is detached before receiving the reply from remote process.

This is correct, and that's why I'm only holding on to a maximum of 10 native key events. In the case that these don't get cleared because of one of the two scenarios here, they will get cleared eventually once 10 subsequent key events have been processed. We may not be able to forward a custom shortcut to the native menu system in the scenarios that you mention, but I believe those should be rather rare. Did you have any other concerns that I didn't think about?

> >+void
> >+nsChildView::PostHandleKeyEvent(mozilla::WidgetKeyboardEvent* aEvent)
> >+{
> >+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> >+
> >+  // We always allow keyboard events to propagate to keyDown: but if they are
> >+  // not handled we give menu items a chance to act. This allows for handling of
> >+  // custom shortcuts. Note that existing shortcuts cannot be reassigned yet and
> >+  // will have been handled by keyDown: before we get here.
> >+  NSEvent* cocoaEvent =
> >+    [sNativeKeyEventsMap objectForKey:@(aEvent->mUniqueId)];
> >+  [sNativeKeyEventsMap removeObjectForKey:@(aEvent->mUniqueId)];
> >+  if (!cocoaEvent) {
> >+    return;
> >+  }
> 
> So, I think there should be sHandledUniqueKeyEventId. Then,
> PostHandleEvent() should remove native events between
> (sHandledUniqueKeyEventId + 1) and (aEvent->mUniqueId).
> 
> 
> Sorry for that I didn't realize this issue at previous review.

My solution to this was to keep a maximum of ten native events in our sNativeKeyEventsMap. Especially with multiple child processes, I thought that it might be possible that we don't get reply key events back in the same sequence that we originally dispatched them to child processes. This is done by the line [sNativeKeyEventsMap removeObjectForKey:@(sUniqueKeyEventId - 10)]; in keyDown: in nsChildView.mm. Also, note comment 129 where Markus liked this approach, but I'm happy to adjust if necessary.
Attachment #8867034 - Attachment is obsolete: true
Attachment #8867386 - Flags: review?(masayuki)
Ah, okay. I've not realized |[sNativeKeyEventsMap removeObjectForKey:@(sUniqueKeyEventId - 10)];|. Okay, it's file if it doesn't cause memory leak being detected during mochitests.
(Assignee)

Comment 155

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ab3e4bdcb957b1fed5f0495f6d9f95e7b3f3cb
Bug 429824: Let OSX's native menu system handle keyboard shortcuts that we did not handle ourselves via XBL prototype handlers. This allows for handling of custom shortcuts, but does not allow for reassignment of existing shortcuts. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc96471117ab6243119c8593b5f562e413036f81
Bug 429824: Properly forward native OSX events to the native menu bar if they haven't been handled by the child process in e10s. r=mstange,masayuki

Comment 156

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92ab3e4bdcb9
https://hg.mozilla.org/mozilla-central/rev/dc96471117ab
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1365880
Depends on: 1365825
Depends on: 1366626
Shouldn't this be nominated to release note?
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 158

2 years ago
I was hesitant about nominating this because bug 1333781 still limits this feature quite a bit, but I guess why not.

Release Note Request (optional, but appreciated)
[Why is this notable]: Users on OSX and macOS can now assign custom shortcuts to Firefox menu items.
[Affects Firefox for Android]: no
[Suggested wording]: Users on OSX and macOS can now assign custom shortcuts to Firefox menu items via System Preferences > Keyboard > Shortcuts. Default shortcuts in Firefox will continue to take precedence and cannot be reassigned at this time.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: needinfo?(spohl.mozilla.bugs)
adding to 55beta release notes
Depends on: 1397711
You need to log in before you can comment on or make changes to this bug.