382138 - Shortcut keys don't work until the appropriate menu was opened before

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a5pre) Gecko/20070526 Thunderbird/3.0a1pre ID:0000000000 [cairo]

Shortcut keys like Cmd+A, Cmd+C, Cmd+U, and others don't work when you have started Thunderbird. To get them working you have to open the appropriate menu before. Closing it immediately and hitting the shortcut again you can see it is working. Is there a problem in registering these shortcuts? It only happens for Thunderbird. Firefox doesn't show this issue.

Steps:
1. Open Thunderbird
2. Select a message
3. Hit Cmd+A => no text is selected
4. Open menu 'Edit' and close it immediately
5. Hit Cmd+A again => no text is selected
6. Open menu 'Edit|Select' and close it immediately
7. Hit Cmd+A again => all text is selected

Cmd+A and other shortcuts should work directly after starting Thunderbird. It seems that they aren't registered? This is only an issue on trunk. My Thunderbird 2.0 works fine here.

Comment 1

[Karsten Düsterloh]

I see this, too, with a current custom built Suiterunner MailNews on Mac.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is an essential part so requesting blocking1.9.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This should be a blocker.

Comment 5

[Benjamin Smedberg]

This is a blocker if it's still happening, though I'm pretty sure it has been fixed. Please somebody test and mark WFM or mark the duplicate.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Both this bug (bug 382138) and bug 388226 still happen in today's
Thunderbird trunk nightly (2007-08-31-03-trunk).  (Bug 388226 was reported
on the branch.  But neither bug happens with Thunderbird 2.0.0.6.)

I'm puzzled why this was assigned to me:  I know nothing about
Thunderbird-specific code, and the equivalents of these bugs don't seem to
effect current Minefield nightlies.

(I _have_ noticed other strange menu behavior in Minefield, though -- for
which I'll open another bug.  When I do I'll post a comment here.)

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> (I _have_ noticed other strange menu behavior in Minefield, though -- for
> which I'll open another bug.  When I do I'll post a comment here.)

False alarm -- the problem was fixed in today's Minefield nightly.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Interestingly, some key combinations work just fine without having to open
a menu first -- e.g. Command-N, Command-W, Command-M, Command-?, Command-Q.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Bug 356314 comment #0's first STR (which effects SeaMonkey's MailNews
component) may be a dup of this bug.

Comment 10

[Phil Ringnalda (:philor)]

Mmm, tangled focus and events and overlays, oh my! Thunderbird's stuff is pretty much the same as SeaMonkey's, since that's where we stole it:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/base/content/mailWindowOverlay.xul&rev=1.222&mark=77-91&#77 (which I first read as "<!-- Premature optimization...") is undoubtedly what makes it work after you first open the Edit menu - create-menu-edit fires, and things get hooked up to properly update for focus and select, not just when the menu opens. But apparently in Cocoa-widgets world only, either the events in http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/editMenuOverlay.xul&rev=1.6&mark=11-17#11 don't make it to those commandsets, or they aren't actually "there" until the first time the menu opens, or... something.

Comment 12

[Wayne Woods]

Looks like everything in the Edit menu is affected, as well as Cmd+U and Cmd+p.
The latter is in the same menu as other shortcuts that *do* work, but the rest
aren't. And while this is probably obvious, it's never actually been stated
that the regression date on the trunk was between 20060928 and 20060929 (=
Cocoa widgets switched on).

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #10)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/base/content/mailWindowOverlay.xul&rev=1.222&mark=77-91&#77

Phil, the same commandsets are already defined in utilityOverlay.xul. They are overwritten by the ones in mailWindowOverlay.xul.

http://mxr.mozilla.org/seamonkey/source/mail/base/content/utilityOverlay.xul#88

After removing both commandsets in mailWindowOverlay.xul all commands within the Edit menu are working fine for me without having to open the menu.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, could it be possible that one of the multiple declaration raises this problem? As I said it will work after removing the first one. But it's strange that there was no change for a longer period. I really would like to see it fixed but haven't a glue where to start and what's the right way.

Comment 16

[David :Bienvenu]

Interesting - I don't know why the duplicates would make a difference, but empirically it sounds like they do. 

Comment 17

[Karsten Düsterloh]

There aren't "duplicate definitions" as such, bug 108761 introduced them as performance optimizations (see file history in the SeaMonkey version in <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul&rev=1.343&mark=85-99#80>).
Just throwing these out will unfix bug 108761...

Some shortcut keys working while others don't (e.g. Cmd-P being broken in the File menu while the rest works) is just coincidence: the working keys are not disabled in xul, while e.g. Cmd-P (Printing) has disabled=true.

So the actual problem is that the state of these keys is not set correctly (iow: *never* set before opening the respective menu), which I think is due to imperfect/incomplete changes in the course of the cocoa transition, especially here: <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/cocoa/nsMenuX.mm&rev=1.60&mark=598-614#595>

Comment 18

[Karsten Düsterloh]

Attachment: Testcase (needs chrome rights): Accel-A/C work for all; Accel-I works for none; Accel-N doesn't work on Mac, but on Linux

So, after some more digging, this problem is that the Cocoa switch fixed a bug which both SM and TB rely on: for performance reasons, certain keys call goDoCommand directly in their oncommand handler instead of using the actual command element. Cocoa Mac denies the use of commands with disabled=true, while Linux (and presumably Windows) just passes the key stroke to the command controller, which doesn't care for the attribute.

The attached testcase simulates the code paths we have eg. for cmd_print in SM/TB.

Comment 19

[[e-mail]]

(In reply to comment #18)
>a bug which both SM and TB rely on
That's not a bug, it's a feature :-P

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No blocker for 1.9 anymore? Asking for renomination.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

The other apps can fix this in their own code. See comment 18.

Comment 22

[[e-mail]]

(In reply to comment #21)
>The other apps can fix this in their own code.
What, so someone breaks something on the Mac, and now everyone else has to work around the new broken behaviour?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Renominating per comment 22, sorry.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #22)
> (In reply to comment #21)
> >The other apps can fix this in their own code.
> What, so someone breaks something on the Mac, and now everyone else has to work
> around the new broken behaviour?

No, read comment 18:

> So, after some more digging, this problem is that the Cocoa switch fixed a bug
> which both SM and TB rely on: for performance reasons, certain keys call

We *fixed* a bug. All SM and TB have to do is go and fix it on their end, as well.

(In reply to comment #23)
> Renominating per comment 22, sorry.

Still doesn't block.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Beltzner, so should this bug be morphed to Thunderbird now? If there is no bug anymore under Core there is nothing to fix at all. If it would be the right way, there should also be filed a new bug for SeaMonkey.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Maybe I'm missing something but the "bug fix" described in comment 18 breaks existing optimizations (which might be fine) but worse yet introduces inconsistencies between the platforms.  This makes writing extensions that much harder.  If we think that the disabled attribute should affect the behavior here, it should affect it on all platforms, not just mac.

That doesn't sound like much of a bug fix to me, to be honest, just fallout from a landing that regressed stuff.

I'm not going to renominate again, since I suspect that the fact that Firefox is unaffected means it'll just get minused (though it's a core XUL bug, not an app issue).  But I do think this isn't being handled right, as is the case with a lot of our regressions.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Mike, see comment 27?

Comment 29

[Samuel Sidler (old account; do not CC)]

(In reply to comment #27)
> I'm not going to renominate again, since I suspect that the fact that Firefox
> is unaffected means it'll just get minused (though it's a core XUL bug, not an
> app issue).  But I do think this isn't being handled right, as is the case with
> a lot of our regressions.

I will. If drivers are minusing bugs that are core issues and, more specifically, regressions from previous landings, that's a huge problem.

Re-nominating.

Comment 30

[Robert Sayre]

(In reply to comment #29)
> 
> Re-nominating.

Take it to m.d.platform. Don't get into edit wars.

We certainly aren't going to block the Firefox release on an issue that doesn't affect Firefox. I don't think blocking 1.9- means we won't take a fix for other Gecko apps, which have their own release schedules.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Robert and Beltzner, would my proposal in comment 26 be a good solution in better handling this bug?

Comment 32

[Phil Ringnalda (:philor)]

No. Do not move this bug to Thunderbird until someone who actually understands the intended interactions between the widget layer and XUL says that Cocoa accidentally fixed a bug which should have been fixed, and files bugs on the other widget toolkits to make them break, err, I mean fix this too.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I don't think blocking 1.9- means we won't take a fix 

In this case, sure it does.  Since it's minused, none of the people involved in causing this regression will work on it (as they've publicly said in other regressions they've caused).  No one else knows the code involved.  Once we ship Gecko 1.9 with this, we won't be able to change it on the branch because of compat issues (at least at that point I'd certainly avoid any changes to this on the branch).  That's if we even remember this bug exists, since it got taken off all the tracking lists.  And assuming anyone bothers to try to fix regressions on branch at all; given how 1.8 has worked, I wouldn't bet on it.

And in the meantime, this will be screwing over non-Firefox apps and extensions: authors will write code on Windows, just to have it break on Mac.  I guess we don't care much about extension authors either.

To be honest, it looks to me like the analysis in comment 18 (specifically, deciding that the current behavior is not a bug) is just wrong, but that assuming it's right is expedient for purposes of driving down the blocker list. I can understand that, but we should still have the decency to admit that this is a core bug that needs a core fix, even if we don't think we have the resources to fix it, instead of just playing the ostrich game.

Comment 34

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #33)
> To be honest, it looks to me like the analysis in comment 18 (specifically,
> deciding that the current behavior is not a bug) is just wrong, but that

Strongly agreed.  For lack of a spec, things that make platforms more divergent in their handling of what is supposed to be a cross-platform user-interface language are bugs.  If there is a spec that says otherwise, we should be fixing all platforms at once.

Comment 35

[Brendan Eich [:brendan]]

"1.9" has to mean more than "what Firefox wants", and bz's right -- the likely consequence of not making a blocker to restore platform parity is that this bug won't get fixed, and then can't get fixed because it set a new de-facto standard of sorts.

This isn't hard to see. It should not be hard to face up to. It does not require a newsgroup parlay -- it's an ancient and central "constitutional" question in the Mozilla platform plus XP apps (plural) milestone system. Mozilla 1.9 (or Gecko 1.9 if you prefer) as a milestone release, and XUL as an XP UI language, both deserve more consideration than blocking1.9-.

/be

Comment 36

[Robert Sayre]

(In reply to comment #35)
> "1.9" has to mean more than "what Firefox wants",

No it doesn't. Just ifdef it already.

> and bz's right -- the likely
> consequence of not making a blocker to restore platform parity is that this bug
> won't get fixed, and then can't get fixed because it set a new de-facto
> standard of sorts.

And yet no one has explained why we might want to fix it. Just back and forth about "wrong", and *none* of it affects the app we actually need to ship--unless it really does break extensions in a bad way. In that case, it seems like a simple explanation would do, without a bunch of "Gecko" religion. Gecko is not a product.

> 
> This isn't hard to see. It should not be hard to face up to. It does not
> require a newsgroup parlay -- it's an ancient and central "constitutional"
> question in the Mozilla platform plus XP apps (plural) milestone system.
> Mozilla 1.9 (or Gecko 1.9 if you prefer) as a milestone release, and XUL as an
> XP UI language, both deserve more consideration than blocking1.9-.

As I said above, I'm not aware of a plan to "release" Gecko.

Simple formula for disagreeing with a blocker decision:

 * explain why the issue is serious
 * avoid questioning the motivations of others

Comment 37

[Michael Ventnor]

(In reply to comment #36)
> As I said above, I'm not aware of a plan to "release" Gecko.

Then why do we even have a blocking1.9 flag? That should imply that bugs marked with + are blocking *something*, the 1.9 bit implies Gecko 1.9.

> > "1.9" has to mean more than "what Firefox wants",
> 
> No it doesn't. Just ifdef it already.

The Mozilla codebase is poisoned with enough ifdefs as it is without adding more, and forcing our embedders to add more, purely because a couple of people want the Firefox 3 release to be rushed along.

Comment 38

[Robert Sayre]

(In reply to comment #37)
>
> purely because a couple of people
> want the Firefox 3 release to be rushed along.

Yeah, it sure is moving fast. :/

Tell you what, I'll renom this and unCC myself from this complete waste of time.

It sounds like there are two objections here

 a) the original change is wrong
 b) we can't make the original change because of other apps

If a) is the case, I don't see why all the project politics are required. I don't think b) is a reason to block.

And if any of this delays the Firefox 3 release for 60 seconds, everyone in this bug gets a dunce cap.

Comment 39

[Brendan Eich [:brendan]]

We used to have #ifdef MOZ_PHOENIX a lot. Almost not at all now:

http://lxr.mozilla.org/mozilla/search?string=MOZ_PHOENIX

Going backwards costs -- it costs people in bug habitat breeding real bugs (I'm not going to waste time doing CVS archeology right now to dig them up), hacking on two sides of the #ifdef to keep the code sane, and readability.

It has cost XUL developers pain too, not just add-on developers but core ones. I remember spending time with bryner sorting out a MOZ_PHOENIX autocomplete bug in 2004 involving code forked from xpfe. Not going back there.

If 1.9 really is only about Firefox 3 then we can get rid of the whole milestone plan, and stop talking about Gecko and worrying about the rv: value in the User-Agent header.

This is tempting in the short term: "product" is everything -- "project" can be neglected. Trade-offs based on this thinking should and do happen, on an exceptional basis. In any longer term thinking, it is the road to ruin. You will be overruled at some point if you go to far down this road.

We "release" Gecko every Mozilla milestone, with a CVS tag that recommends others to pull and build products on the back end. We worry about what the Gecko rv: in the User Agent means for this reason. We're a big open source project, not just a one-product effort.

For why we might want to fix this bug, re-read comment 27, seconded in comment 34. Respond to the point, not the person.

I didn't question anyone's motives in comment 35, but I'm calling your style of non-argument out. Dismissing or ignoring crucial arguments, imputing religion or empty moralizing to others, trivializing bones of contention -- these are not right.

Whatever your motives (I'm sure they are close to the laudable "ship a great Firefox 3 as soon as makes sense on its own terms"), you are doing the wrong thing, rhetorically and socially more than technically, but technically too.

Mid-aired with a complaint about more than 60 seconds' time wasted -- that's a laugh. Rob, you've caused many minutes of that waste, for yourself and others. Take a look in the mirror.

I'd like to hear from XUL folks like Neil Deakin.

/be

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #38)
> It sounds like there are two objections here
> 
>  a) the original change is wrong
>  b) we can't make the original change because of other apps
> 
> If a) is the case, I don't see why all the project politics are required. I
> don't think b) is a reason to block.

Boris clearly said (a) and everyone who can read gets that. There's no argument.

(b) has been a reason to block past releases, including Firefox 1.0 which was made off of a branch shared with Thunderbird. We've fixed platform and product disparity bugs at the last minute, treating them as blocking even.

Nevertheless, to cool things off I suggest we make this bug wanted1.9 and get the right people, the ones who should own issue (a), on it.

/be

Comment 41

[Robert Sayre]

(In reply to comment #39)
> 
> For why we might want to fix this bug, re-read comment 27, seconded in comment
> 34. Respond to the point, not the person.

That comment says it's a bug because it makes Mac different from other platforms. There are already many desirable cases of this, so that in itself is not evidence of a bug.

But I really could care less.

Comment 42

[Brendan Eich [:brendan]]

(In reply to comment #41)
> (In reply to comment #39)
> > 
> > For why we might want to fix this bug, re-read comment 27, seconded in comment
> > 34. Respond to the point, not the person.
> 
> That comment says it's a bug because it makes Mac different from other
> platforms. There are already many desirable cases of this, so that in itself is
> not evidence of a bug.

Ok, now you are arguing. I'll take what I can get and let XUL owners work this out, but it's absolutely a bigger deal than an intentional HIG-matching tweak or whatever. It's pretty obviously unintended, and disabled having platform-dependent meaning looks like a mistake to me.

> But I really could care less.

Evidently ;-) ("couldn't care less" is the right phrase). How's that dunce cap fitting? Mine is itchy, I'm taking it off now.

/be

Comment 43

[Asaf Romano (gone)]

Hey Beltzner. What bug did you or anyone **fix** in comment 24? FWIW, the current cocoa cocoa code still breaks more than it fixes, see bug 419726 for example (in which, technically, cmd+option+f isn't working because command+F is dis . We do break legitimate XUL usage since cocoa was enabled on trunk.

If I'm reading this right, any code which adds <key oncommand="..."/> elements to xul windows won't work. This is a _very_ common pattern in extensions.

Comment 44

[Robert Sayre]

(In reply to comment #42)
> 
> It's pretty obviously unintended, and disabled having
> platform-dependent meaning looks like a mistake to me.

Sure, I didn't resolve the bug. But it's not like this one issue is ruining an otherwise perfect cross-platform story. This is obvious to anyone who's written a XUL app (even super simple ones), and it's hard to block on it.

This issue is confused by bz's bogus claim that bugginess and blocking are the same thing because of unattributed public statements. As you say, let the XUL module owners work out which patches can stay in the tree. Release drivers can't save you.

I'm not having reading comprehension problems, and I don't have my head in a dark hole.

Comment 45

[Phil Ringnalda (:philor)]

Attachment: Hackaround for Tb [checked in]

Tiresome as this bug is becoming, it's still not quite as tiresome as the last 1.5 years of "{keyboard} - {beep} - {open menu} - {close menu} - {keyboard}" - let's just give up.

Comment 46

[Asaf Romano (gone)]

From phil's patch, for people who don't write or read code:
"Widget: Cocoa broke the way we override the commandsets
 for globalEditMenuItems and selectEditMenuItems with our own versions
 which know not to update multilple times while a message is being loaded,
 so on OS X we just live with the multiple updates."

Moving this to Widget: Cocoa again.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Robert, I never said that bugginess and blocking are the same thing.  I said that as far as I could tell the original blocking- decision was made based on incorrect information and that instead of knee-jerk insisting that it was correct we should reevaluate given correct information.

Comment 48

[jag (Peter Annema)]

So here are my observations:

1) I open SeaMonkey Mail/News and go to the "Subject or ..." input field.
2) I hit cmd+v, I get a beep and nothing was pasted.
3) I click on the Edit menu, click it again to close it.
4) I hit cmd+v, and my clipboard's buffer was pasted.
5) I hit cmd+z, I get a beep and my action wasn't undone.
6) I click on the Edit menu, click it again to close it.
7) I hit cmd+z, my action was undone.

It looks to me that the problem here is that if a menuitem starts out disabled, until the native menuitem has been enabled by opening its menu, we don't properly enable it when we enable the command. On Mac, when the native menuitem associated with a shortcut key is disabled the command won't be executed.

In that case this isn't about a bug being fixed and breaking existing code, it's a real regression from the switch to cocoa.

Comment 49

[Asaf Romano (gone)]

Jag, fwiw, this is a little bit more complicated, mail/news code enables _just the key_ until you open the menu, the command stays disabled.

Comment 51

[jag (Peter Annema)]

Mano: Ah, I see. So then the question becomes whether |goDoCommand(...)| should work at all if the command or associated menuitem are disabled.

While you could argue that it shouldn't (in which case we'll have to go fix the other platforms and notify XUL developers, and that should be done in a bug specifically about this issue, not here), apparently the 'de facto' spec says that it should just work, and since existing code depends on it for performance reasons, we want to have it continue to work, so I would consider it not working on Mac a regression. It seems then that the most sane thing to do is to unregress this for 1.9 and deal (if so desired) with the "but it's a bug, not a feature" issue post 1.9.

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just a short note: the last duped bug 420048 reports this behavior for Firefox 3.

Comment 53

[Mike Beltzner [:beltzner, not reading bugmail]]

Boy, a lot can happen in a bug in a day!

I'm going to try and read comment 22 through comment 52 (especially comment 43, thanks Mano!) in a positive light as people arguing passionately for things they care about and believe in.

At any rate: the driver decision here was initially based on the report in comment 18, which indicated that the original fix resolved a longtime error which other applications and extensions had worked around. If that report is false, or misleading, let's focus on that.

Renominating for blocking1.9? was the right thing to do, but a clear explanation of where reasoning went wrong (without rhetoric or leaping to assumptions) and why the bug should block.

I'm still a little torn on this bug, based on my lack of understanding. This was presented to me as "we fixed a long time problem" but it looks more like "we broke the ability to write cross-platform UI code in XUL".

Re-assigning to Neil Deakin who I'll talk to tomorrow for some more detail.

Comment 54

[Josh Aas]

I started looking into this today, I didn't want to weigh in until I had gone through the code and test cases myself and gained my own understanding from the code itself. I'll hopefully be able to finish my exploration within the next week, but I would like to hear Neil Deakin's take on it.

Comment 55

[Neil Deakin]

Attachment: testcase

There may be several issues here:

1. When a <key> element is created that corresponds to what Cocoa thinks is a 'key equivalent', a keyboard shortcut associated with a menuitem, it instead handles that keyboard shortcut as if the menuitem had been selected instead of the key pressed. As the menuitem is disabled, the key is disabled as well. This also has the effect of firing the command event on the associated <menuitem> instead of the <key>. You can see this behaviour in this testcase attached here. This is a Cocoa bug; it should treating the action as a key press and firing the event on the <key>.
2. Thunderbird doesn't update the disabled state on its <command> elements until the menus containing them are opened, so they stay disabled initially. It's possible that this is the intended behaviour though, and is what is described in the code as "an optimization". However, because the menuitem for this key equivalent is disabled, Cocoa thinks the keyboard shortcut is disabled and doesn't interpret the key. Phil's patch fixes this for the Edit menu commands (but not for other commands like View Source) by switching back to the command updater in utilityOverlay.xul which updates on focus changes, which is the way Firefox and other applications are expected to use. 

Note also that the other testcase https://bugzilla.mozilla.org/attachment.cgi?id=294861 does not demonstrate this bug. It has no menus in it, and behaves the same (and correctly) on both 1.9 and 1.8.

Although Thunderbird is doing some odd things in the name of optimization, I think the real bug here is in Cocoa. Specifically, by removing the early return  at http://lxr.mozilla.org/mozilla/source/widget/src/cocoa/nsChildView.mm#4576 , the shortcuts work again. (Thus, from that code block, this might be a Leopard only bug.)

Comment 56

[Mike Beltzner [:beltzner, not reading bugmail]]

Assigning to Josh, and based on Neil's analysis, making it a P2 blocker.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #55)
> , the shortcuts work again. (Thus, from that code block, this might be a
> Leopard only bug.)

Neil, when I filed this bug I used OS X 10.4.x. Now I'm on 10.4.11 and it still happens for me. So it's not only for Leopard.

Comment 58

[jag (Peter Annema)]

So one option is to remove all of lines 4558 through 4581 (the code that tries to dispatch to the menu), which I've tested and seems to work, except I suspect (didn't check) that may cause us to no longer high-light the menu item in the menu bar when you use a shortcut that's present in there.

Another option is, for 10.4, to check if the menu item is disabled before calling performKeyEquivalent (we wouldn't want the beep) and in that case fall through to line 4586 and on. 10.5 already does something like this so I suspect we can just remove the 10.5 specific else block.

Mano pointed out that a potential problem with not going through the native menu code is that if e.g. Paste (cmd+v) is disabled we might end up triggering web content that's set up to listen for cmd+v. I dunno if that's really a problem, I would imagine that they key handling code in processKeyDownEvent would still end up checking the key/command for disabled and consider the key (combo) handled, but it's something to keep in mind.

Btw, can mGeckoChild become null after the call to DispatchWindowEvent or is that code the wrong way around: http://lxr.mozilla.org/mozilla/source/widget/src/cocoa/nsChildView.mm#4374

Comment 59

[jag (Peter Annema)]

Yeah, like I suspected. Commenting out that block of code (it got moved down a few lines, I'll attach a patch for clarity in a bit) makes the relevant top-level menu no longer high-light when you hit cmd+c, cmd+v, cmd+t etc. It's preferable over the current situation though.

Comment 60

[jag (Peter Annema)]

Attachment: Don't try to go through the Mac native menu code, use Mozilla's key handling

Comment 61

[Josh Aas]

I agree that we should fix this, particularly as outlined in comment #55. It is a bug, the history of which is probably quite long and tangled. Some aspects pre-date Cocoa. Our native menu impl is fully of nasty tradeoffs, though it has been improving steadily. I have known about this particular issue for a while but I did not think it was a problem of the magnitude people are making it out to be here. I was probably wrong in not giving it higher priority.

We need to make disabled menu items still work via keyboard command (this particular bug), and we need to do so without regressing native blink and beep behavior. I know how to do that, but it is too risky for this late in 1.9. It involves keeping a hash table of active keyboard commands and their associated menu item and parent menu objects. We also need to have native menu items call an action method that does nothing when they are activated by a key press, and instead send a normal key press event into Gecko from key handlers.

Disabling our use of the native menu system like jag's patch in comment #60 does is unacceptable. Practically what we would gain from fixing this bug in that way is not worth what we would lose, which is native UI behavior (primarily beeping and flashing, basic components of the Mac OS X experience).

I actually already started writing code at one point to fix this problem, I kept my work in bug 398514. I didn't finish the work because it is complicated, risky work that is not of the type I though we should be taking so late in the game - especially given my impression of the seriousness of the problem. It wasn't affecting any major Mozilla applications as far as I knew at the time, and menus on the Mac have always required some special care from XUL authors.

I know how to fix the bug and if drivers decide it is serious enough to take a patch I can do it. However, it would take a good amount of time and testing and is certainly not rc material. Historically this type of patch has resulted in regressions that take time to iron out, I strongly recommend not doing it for 1.9. Keep in mind that what we are requiring XUL authors to do because we have this bug is 1) something most people do anyway and 2) something that will still work after we fix this bug. Nobody will have to code in response to a fix our our end.

Comment 62

[Boris Zbarsky [:bzbarsky]]

If we can't actually fix this the right way, can we at least give apps the choice of sacrificing the beep-n-flash for a significant performance win if they choose to do so?  I realize Firefox will go with the beep-n-flash (because there is no performance at stake), but there is no reason that Thunderbird should fall on that side of the decision.

Comment 63

[Boris Zbarsky [:bzbarsky]]

If we go that route, do we also need to fix the various Firefox/Thunderbird bugs this causes (bug 419726, bug 420048, others?) and whatever extensions are broken (per comment 43)?

Comment 64

[Josh Aas]

(In reply to comment #62)
> If we can't actually fix this the right way, can we at least give apps the
> choice of sacrificing the beep-n-flash for a significant performance win if
> they choose to do so?

What did you have in mind? An ifdef that effectively toggles jag's patch in comment #60? A pref? I don't think either is worth it.

(In reply to comment #63)
> If we go that route

What route are you referring to?

Comment 65

[Boris Zbarsky [:bzbarsky]]

Presumably a pref, unless you want to break people running off a common xulrunner.  It's clearly not worth it to Firefox.  It might be worth it to the apps you broke.

> What route are you referring to?

Not fixing this bug.

Comment 66

[Josh Aas]

If we don't fix this bug  then yes, we should probably make an attempt to fix the bugs you listed. As for a pref, I have no idea how significant the optimization is and I doubt Thunderbird would choose the optimization over the native UI experience anyway. I'd say no on the pref unless we got at least one app that would actually use it or some indication that an optimization is really so tempting that future apps might really want it.

What we could do is take the Thunderbird patch and allow the tbird owners to decide if they want to turn such a pref on when they approve that patch. If they want to turn it on we can add it.

Comment 67

[Mike Shaver (:shaver emeritus)]

"Not worth a pref" means very very little value, given the cost of adding a pref, and I think we've seen that this has significant impact on other apps.

Agree that we shouldn't rip the menu code apart right now, especially since I doubt your other patch includes any automated tests to help mitigate regression risk.  Disagree strongly that we shouldn't provide a pref here to let other dependent apps get the behaviour they need, and which we unilateraly (and, it seems now clear, unnecessarily) regressed.

If any of the other app representatives are in agreement, I think they should file a bug to pref control the behaviour, and then I think we should move this to wanted-next and make the new pref-control bug a b5 blocker.

Comment 68

[Mike Shaver (:shaver emeritus)]

The optimization here and impact on user experience is enormous, AIUI, but we shouldn't presume, and in light of philor's comments I don't think we have to.

Comment 69

[Josh Aas]

We need to file a separate bug for the tbird patch in this bug. I'll file a bug on a pref.

Comment 70

[Josh Aas]

The pref is bug 422686.

Comment 71

[jag (Peter Annema)]

After having run with my patch for a while I have found myself in a few situations where e.g. cmd+w didn't do anything and it took me hitting that combo a few more times to realize that yes, I had indeed pressed the right keys, and hard enough. Clicking somewhere to get focus in the right place made cmd+w work, but the lack of beep feedback is annoying, so this patch as-is isn't quite good enough. Then again, it was never meant to go in as-is :-)

If we can somehow convey "key combo has handler and wasn't executed" back to there then hooking up a beep so we'll at least have aural feedback shouldn't be too hard. I guess we can continue this discussion in bug 422686.

Comment 72

[jag (Peter Annema)]

Josh: so with a pref check in place, is there a way to detect (with additional modifications to processKeyDownEvent?) the difference between:

* key (combo) has no handler, or is disabled, or wasn't handled.
* key (combo) has a handler and was handled

so I can NSBeep() appropriately?

Comment 73

[jag (Peter Annema)]

Oops, sorry, meant to post this in bug 422686.

Comment 74

[David :Bienvenu]

Comment on attachment 306846 [details] [diff] [review]
Hackaround for Tb [checked in]

thx, sr=bienvenu, but please correct the spelling of multiple before you land :-)

+     which know not to update multilple times while a message is being loaded,

Comment 75

[Phil Ringnalda (:philor)]

Comment on attachment 306846 [details] [diff] [review]
Hackaround for Tb [checked in]

mail/base/content/mailWindowOverlay.xul 1.234

Comment 77

[Josh Aas]

Fixed in Gecko 1.9 by the final patch for bug 398514.

Comment 78

[Phil Ringnalda (:philor)]

Attachment: Back out Tb hackaround [checked in]

Yay!

Comment 79

[David :Bienvenu]

Comment on attachment 314292 [details] [diff] [review]
Back out Tb hackaround [checked in]

thx, Phil

Comment 80

[Greg K.]

Verified fixed using Mac Thunderbird version 3.0a1pre (2008040903).

Comment 81

[Phil Ringnalda (:philor)]

Comment on attachment 314292 [details] [diff] [review]
Back out Tb hackaround [checked in]

mail/base/content/mailWindowOverlay.xul 1.238

Comment 82

[mws]

Command + Shift + W still has this problem on 3b5.

Bug 426139