372571 - Edit menu shortcuts don't work in Save file picker

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Steps to reproduce (in Firefox trunk):
1. Cmd+S
2. Try to use an editing shortcut (e.g. Left, Cmd+A)

Result: The "Edit" menu highlights temporarily but nothing happens in the file picker.

Expected: The shortcuts should work like they do everywhere else (including in the file picker as used in other applications).

Comment 2

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

Left (and Cmd-Left) and Up/Down do WFM in a 20071230 Minefield on 10.5.1/Intel, but Cmd-A and other shortcuts that appear in the Edit menu still exhibit comment 0.

See also bug 372571 about problems with the Open filepicker (those WFM, however).

Comment 3

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

Er, make that "See also bug 410921" :p

Comment 5

[Steuard Jensen]

This is a bigger issue than just the shortcut keys!  As described in bug 407586, the functionality is in fact disabled in the edit menu as well: functions like copy, paste, and select all are grayed out and entirely unavailable.

Or, almost so.  I've just this moment observed some particularly odd behavior regarding this bug (running FF3b3 on Mac OS 10.4).  If the cursor is in this bugzilla text field when I say "Save Page As...", the Paste option /is/ available in the Edit menu, but it pastes the text into the text field, not into the dialog box.  This occurs regardless of what text is selected (or not) in the Save As dialog.  Particularly odd is the fact that the keyboard shortcut does /not/ work when attempting to paste in that way!  (It doesn't paste either to the dialog box or to the text area.)

I would love, love, love to see this fixed in the final release of Firefox 3.  I don't have a lot of time for bug fixing at the moment, but if anyone who knows the code could give me an idea of where to start looking to fix this, there's some (small) chance that I could write a patch.  (I don't know when I'd find the time, but I'd at least try.)  But someone who knows what they're doing could presumably fix this much faster than I could (and with less hand-holding).

Comment 8

[Daniel E.]

What makes it so difficult to resolve this bug?

Comment 9

[Steuard Jensen]

At a guess, time.  This bug is labeled a P2: it's important enough that it needs to get fixed, but (as I understand it) it isn't the sort of thing that will require extensive beta testing.  That is, the fix will probably be something fairly simple with little chance of accidentally breaking other parts of the program.  So the primary developers make a point of fixing the more "fragile" or complicated bugs earlier than the easy ones.  (I suppose that there could actually be some really subtle focus issue involved that I'm just not aware of, but that seems less likely to me.)

Comment 10

[Josh Aas]

Right, I have a good idea of how to fix this but I haven't gotten to it yet.

Comment 11

[Josh Aas]

Attachment: fix v0.5

Saving my work. This has things working for the file picker, needs to be reorganized so it can work for other native dialogs like the printing dialog. This patch will break embedding, have to fix that up too.

Comment 12

[Josh Aas]

Attachment: fix v0.6

Comment 13

[Josh Aas]

Attachment: fix v1.0

This is the same as fix v0.6 but it does not include an implementation for the printing dialog. For some reason, probably having to do with our Carbon printing impl, the change just does nothing. We call Cocoa setMainMenu: as expected but the menu does not change before the printing dialog goes up. Since it isn't critical we should move on and file a followup bug about that.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Josh, this basically looks fine, but I found a bug:

1) Make one of the native app-modal dialogs appear (say by doing
   command-o or File : Open File) -- your custom menu becomes the main
   menu (as it should).

2) Choose Minefield : Hide Minefield or do command-h -- Minefield will
   get hidden (as it should).

3) Unhide Minefield (by clicking on its Dock icon or doing
   command-tab) -- Minefield will re-appear, but now the main menu
   will be set to the original (full) main menu (the one attached to
   the window above which the native app-modal dialog appeared).

Comment 15

[Josh Aas]

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

v1.0 has another major problem, if there is no key window after the modal dialog closes then you're stuck with the menu bar from the modal dialog. You can do this by closing all windows, hitting cmd-o to get an open file dialog, and hitting cancel.

Comment 16

[Josh Aas]

Attachment: fix v1.1

Fixes the hide/show bug that Steven found in comment #14 and the bug I found in comment #15.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 311499 [details] [diff] [review]
fix v1.1

This looks fine to me, and I didn't see any problems in my tests (on
OS X 10.5.2 and 10.4.11).

I've confirmed that it fixes the problems described in my comment #14
and Josh's comment #15.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not at all comfortable sr'ing this quickly; if someone knows this code better than me, please bounce over to them, otherwise I'll spend some time with it in the next day or so.

Comment 19

[Josh Aas]

A note on the localization comment in that patch - we should file a followup bug on that and fix it later. There is no easy fix and I don't want to spend time in the 1.9 end game fixing something that probably won't realistically affect many users.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, spent a bit reading through this -- I'm really not all that happy about adding those methods into nsIAppShell.idl.  Is there any other way to get to the same class?

Comment 21

[Josh Aas]

Eventually we want to access those outside of widget code, like from printingui. For that, we need them to be in some xpcom object, the two most obvious are toolkit and appshell. Appshell just seemed like the better of the two options.

Comment 22

[Josh Aas]

Attachment: fix v1.2

Skip exposing the API via XPCOM for now.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 311741 [details] [diff] [review]
fix v1.2

Ah, that works for me -- nsCocoaUtils sounds like a great place for it.  Thanks!

Comment 24

[Josh Aas]

landed on trunk

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko Minefield/3.0pre ID:2008032723

Comment 26

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5238 had been added to Litmus.