433719 - Menu commands are double-processed in Camino when plugins are focused

Status: RESOLVED FIXED

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

Description

[Stuart Morgan]

This is a regression from bug 428047:
1) Focus a plugin (I've tested Quicktime and Flash; presumably others are the same)
2) Press command-N
3) Get two new windows.

FWIW, if it's between this behavior, and just never sending events to plugins if there is a menu command that would be triggered (as I gather used to be the case), I'd much rather have the latter.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I've now tested a bunch of key combinations (though not all of them),
and command-n is the only one I've found that gets processed twice.
Which means that we could conceivably hack a special case for
command-n ... though I'd really prefer not to.

Please let me know if there are any others I've missed -- particularly
on non-US keyboards.

Comment 2

[Josh Aas]

Attachment: fix v1.0

This works by unconditionally returning YES from performKeyEquivalent if an embedder's menus return YES for handled. We still inform the plugin of the event even if the embedder's menus handled the event, so long as the plugin as focus.

This patch should be safe enough for the 1.9.0 branch because it only changes our behavior if an embedder's native menus handle an event. It won't affect any XUL-based applications like Firefox.

Comment 3

[Stuart Morgan]

Comment on attachment 333978 [details] [diff] [review]
fix v1.0

The variable name is a bit confusing; it's odd that "return returnYesUncondationally" can return NO. What about something more like "handledByEmbeddor"?

r=smorgan either way.

Comment 4

[Josh Aas]

Comment on attachment 333978 [details] [diff] [review]
fix v1.0

I'll change the variable name on checkin.

Comment 5

[Josh Aas]

Attachment: trunk fix v1.0

Comment 6

[Josh Aas]

Landed on trunk. We need this in 1.9.0.x so that Camino can ship off of the 1.9.0 branch. Risk is minimal because the logic doesn't change except in the embedding case.

Comment 7

[Stuart Morgan]

We'd really like to get this in the next week or so if possible, as we're hoping to ship our alpha in early/mid September and this is perhaps the most noticeable regression that I see in normal use.

Comment 8

[Stuart Morgan]

Requesting blocking. This bug is one of a handful of Cocoa Widget regressions that I feel leave 1.9.0 unsuitable for stable release by Cocoa embeddors; it's frustrating that in over a month no progress has been made on landing an essentially zero-risk fix for it.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 334196 [details] [diff] [review]
trunk fix v1.0

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 10

[Josh Aas]

landed for 1.9.0.4

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Could someone from the Camino team please verify that this is fixed. Using Camino 1.6.4, I haven't been able to reproduce the original issue.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.4pre) Gecko/2008101818 Camino/2.0a1 (like Firefox/3.0.4pre)

Clicking in a Flash video and pressing Cmd-N no longer produces two new windows.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Changing this to resolved and changing the keyword from "fixed1.9.0.4" to "verified1.9.0.4". The "verified" resolution is used for Trunk in this case, not for 1.9.0.4.

Thanks for the quick verification. It's appreciated!