228699 - Download Manager should close on CTRL+Y / ESC

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement, P3)

Description

[athakur]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031216 Firebird/0.7+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031216 Firebird/0.7+

All of the sidebar windows can be closed using the same button that opens them
(for example, pressing "CTRL-B" opens the bookmarks window, hitting "CTRL-B"
closes it again).  The new download manager window does not follow the same
behavior.

I think the download manager should follow the same behaviour both for
consistency and ease of use.

Reproducible: Always

Steps to Reproduce:
1. Hit CTRL-E to bring up the Download Manager
2. Hit CTRL-E again

Actual Results:  
Nothing happens

Expected Results:  
Download Manager window should close and focus return to the browser window from
which the Download Manager was initially called

Comment 1

[Mike Connor [:mconnor]]

focus returning to the browser window it was spawned from is bad if you're in a
second or third window, because then you need to switch back to where you're
hitting Ctrl-E

-> NEW

Comment 2

[Ayo]

I'm sure the framework is present in the browser to remember the state at which
the browser is layered and basic window managment. So that if you press CTRL+E
and you had no Firebird Windows maximized it would just close the download
manager and leave the screen, which would cause the Firbird to lose focus.

Besides closing download manager with the same starting key combination is the
most intuitive response.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

target. 

Comment 4

[Ali Ebrahim]

*** Bug 236398 has been marked as a duplicate of this bug. ***

Comment 5

[Ali Ebrahim]

If we're doing this for the Download Manager window, does it make sense to have
Ctrl+I close to Page Info window, and just implement this system of
same-shortcut-to-close-window for this class of windows?

Comment 6

[Petey]

Not necessarily.  The reason for the Ctrl-E toggle for the DM is the precedent
that was set when the component was a sidebar.

Comment 7

[Joe M]

I think Ali's suggestion is not a bad idea. Perhaps pressing a dialog's hotkey
should dismiss the dialog if it has focus; otherwise it could open or bring the
dialog to the front (like Winamp 5's "Bring to front/hide Winamp" global hotkey,
if anyone uses that).

Comment 8

[Roasted]

Minor add: when making CTRL-E a toggle the relating toolbar button for the
downloads window should act the same way (toggling and showing a pressed state
and a released state)

Comment 9

[Jeff Walden [:Waldo]]

See bug 233440 - Change DL Manager Keybinding.

Comment 10

[Steffen Wilberg]

*** Bug 242882 has been marked as a duplicate of this bug. ***

Comment 11

[Steffen Wilberg]

Attachment: patch for to make ctrl-y close it

Simply adds a second close keybinding to the downloads manager (in addition to
ctrl-w). The effect on hitting ctrl-y is this:
If the downloads manager is not open, it is opened and gets focus (no change
here).
If the downloads manager is already open, but doesn't have focus, it is brought
up front and becomes focus (no change here as well).
If the downloads manager has focus, it is closed, just like hitting ctrl-w or
clicking on the close window widget.

So you can hit ctrl-y to have a short look in the manager, and hit ctrl-y again
to get rid of it.

Comment 12

[Steffen Wilberg]

Comment on attachment 150398 [details] [diff] [review]
patch for to make ctrl-y close it

Doesn't include the toolbarbutton yet.

Comment 13

[(not currently active) Ted Mielczarek]

*** Bug 248135 has been marked as a duplicate of this bug. ***

Comment 14

[Daniel]

I request to RESOLVE this bug via changing CTRL+Y back to CTRL+E. It is
important USABILITY ISSUE!

You cannot close DM by one hand pressing CTRL+Y. Try it.

But you can to close it easily via CTRL+E.

Comment 15

[Mike Connor [:mconnor]]

Typing in all caps is annoying.  So is going offtopic and ranting about things
outside of the context of a specific bug.

Comment 16

[Daniel]

(In reply to comment #15)
> Typing in all caps is annoying.  So is going offtopic and ranting about things
> outside of the context of a specific bug.

And you are the greatest man on the earth.

Comment 17

[Mike Connor [:mconnor]]

(In reply to comment #16)
> And you are the greatest man on the earth.

its always good to know I still have fans.

<backontopic/>

Steffen, any update on this?  I'd like to see this sooner rather than later :)

Comment 18

[Steffen Wilberg]

Comment on attachment 150398 [details] [diff] [review]
patch for to make ctrl-y close it

Let's start with this one, before bsmedberg's large patch-bitrot (toolkit
locale move) takes place.

Comment 19

[Steffen Wilberg]

The toolbarbutton is much harder to fix. It's possible to detect whether the
download manager is open or not, by getMostRecentWindow(Download:Manager). So I
could easily make the button close the download manager if it's open. 
But is that what we want? If the downloads manager is already open but hidden
behind the main window, it should IMHO not be closed, but brought up front and
become focus, like ctrl-y does with my patch applied. On the other hand, it
wouldn't be that bad if the toolbarbutton toggled open and close, no matter
where the download manager is.

There are some z-order functions in nsIWindowMediator. So we could possibly
detect whether the download manager is on top, and only close it in that case.
The problem is that in the moment you click on the toolbarbutton, the main
window is up front. But how can we know whether the download manager was brought
behind the main window through clicking on the toolbarbutton, or whether it was
there all the time? I don't know if "onmousedown" helps here.

It's possible to make the download manager always stay on top (alwaysRaised
flag), but that's pretty annoying.

Comment 20

[Mike Connor [:mconnor]]

The answer depends on whether we're going to have a pressed state for the
downloads toolbar button.

If we do:
Ctrl-Y/toolbar button should act as (effectively) a show/hide toggle.

If we don't:
clicking the toolbar button should be a single action (open/bring to front).
Ctrl-Y can still be used to close it if the Download Manager window has focus,
but if not we should bring it into focus.

I like the toggle concept better, we just need to implement the pressed state
for it to be coherent.

Comment 21

[Steffen Wilberg]

Attachment: toggle: almost done

Here's the toggle. Click: open. Second click: close. Ctrl-y does the same. No
matter if the download manager is on top or behind, has focus or not, it is
closed if it's already open, and is opened if it's not open yet.

The toolbarbutton now has a pressed state, which is the same for all windows. I
need an observer to inform the other windows.

The only issue I noticed is that the button doesn't get the pressed state when
adding it to the toolbar and the download manager is open.

Comment 22

[Steffen Wilberg]

Attachment: toggle v1.0

This fixes the issue I mentioned before by setting the "checked" attribute on
the always-present "Tools:Downloads" command, instead of the downloads-button
itself. Renamed the observer from "Tools:Downloads", which could be confusing,
to DM-open-close. Moved the notifyObserver() calls to downloads.js since I need
the close observer there anyway.

Comment 23

[Steffen Wilberg]

Attachment: toggle v1.0.1

removed a now unnecessary if.

Comment 24

[Steffen Wilberg]

Attachment: toogle v1.0.2

I really hate removing traces of other changes by hand.

Comment 25

[Steffen Wilberg]

Comment on attachment 152695 [details] [diff] [review]
toogle v1.0.2

Here you are!

Comment 26

[Steffen Wilberg]

Requesting blocking RC1 due to localization impact, and because I wouldn't mind
some public testing.

Comment 27

[:deanis74]

If it were up to me, I'd mark this won't fix.  See bug 176177 comment 33 and bug
176177 comment 35.  But it's not up to me it's up to Ben, so I'll be quiet.

Comment 28

[Jay Patel [:jay]]

I think the current behavior of the toolbar button and ctrl-y is fine.  If we
want to be able to close the window, why no just make the esc key or crtl-e
close it (see bug 233529).  Would that work?

Comment 29

[Steffen Wilberg]

We can of course just make Ctrl-Y close it, without changing the toolbarbutton.
See comment 11.

Comment 30

[Steffen Wilberg]

Comment on attachment 152695 [details] [diff] [review]
toogle v1.0.2

>Index: mozilla/browser/base/content/browser.js
>+  init: function()
...
>+    // copy the state of the download manager button from the opening window, if any
>+    if (window.opener && window.opener.document.getElementById("Tools:Downloads").getAttribute("checked") == "true")
>+      document.getElementById("Tools:Downloads").setAttribute("checked", true);

This has to be replaced by:
>   // set the state of the download manager button
>   if (this._getDM())
>     document.getElementById("Tools:Downloads").setAttribute("checked", "true");
because not every opener has "Tools:Downlaods", for example the extension/theme
manager when clicking on the u.m.o link.

Comment 31

[Steffen Wilberg]

*** Bug 233529 has been marked as a duplicate of this bug. ***

Comment 32

[Steffen Wilberg]

From the dupe: The download manager should be closed on ESC as well.

Comment 33

[Steffen Wilberg]

Attachment: toggle v1.0.3

- makes the changes I mentioned in comment 30.
- makes the download manager close on ESC (like Bookmarks Manager, Page Info,
Options etc.)

Comment 34

[Steffen Wilberg]

Attachment: toggle v1.0.4 (toolkit locale move)

I knew it.

Comment 35

[Jay Patel [:jay]]

Closing the Downloads manager window with Ctrl-E, Esc and the Downloads toolbar
button fails our basic functionality tests:
http://testrunner.mozilla.org/test.cgi?run_id=243#892

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

l10n changes please land soon. 

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 153186 [details] [diff] [review]
toggle v1.0.4 (toolkit locale move)

Please don't request approval until you've received necessary reviews. Thanks.

Comment 38

[Manoj]

Sorry to throw a spanner in the works but Ctrl+Y is the traditional windows
shortcut for Redo Change/Editing (Edit->Redo). Is assigning Ctrl+Y to the
download manager an oversight or has the functionality been overloaded
intentionally.

Comment 39

[Steffen Wilberg]

Manoj, bug 233440 changed the key from Ctrl+E to Ctrl+Y. Redo still works in
textareas and in the bookmarks manager. It doesn't work in the bookmarks
sidebar, but it never worked there.

Comment 40

[Ben Goodger (use ben at mozilla dot org for email)]

Oops. Steffen, can you remove the section that closes the dlmgr from the browser
window when the dlmgr isn't focused? The correct behavior when the window is
open in the background when Ctrl+Y is pressed from a browser window is to bring
it forward. We weren't talking about changing that - only allowing it to be
closed with Ctrl+Y from the dlmgr window. 

As for the key collision.. oops. How about we change it back to E... but add a
Shift modifier. Ctrl+Shift+E. 

Comment 41

[Steffen Wilberg]

Comment on attachment 153186 [details] [diff] [review]
toggle v1.0.4 (toolkit locale move)

no toggle then.

Comment 42

[Steffen Wilberg]

Ctrl+Shift+E is dangerous because it's next to Ctrl+Shift+W, which is "close the
current window without prompting".
Ctrl+Shift+Y is a bit difficult to press with one hand on non-German keyboards.
Ctrl+Shift+D would be nice, but doesn't seem to work!?

Comment 43

[Steffen Wilberg]

Attachment: just make make Ctrl-y and ESC close it

This is mostly the first patch, plus ESC.

Comment 44

[Steffen Wilberg]

Comment on attachment 155752 [details] [diff] [review]
just make make Ctrl-y and ESC close it

Explanation is in comment 11 and comment 42.

Comment 45

[Steffen Wilberg]

"Note that you cannot use Shift-Ctrl-A-thru-F or Shift-Ctrl-0-thru-9 for your
own purposes, as these combinations are used to enter unicode characters in text
fields."
http://developer.gnome.org/projects/gup/hig/2.0/input-keyboard.html#shortcuts

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 155752 [details] [diff] [review]
just make make Ctrl-y and ESC close it

r+a=[e-mail]

Comment 47

[Ben Goodger (use ben at mozilla dot org for email)]

(Ctrl+Shift+D is "add bookmark silently")

Comment 48

[Steffen Wilberg]

Checked in br & trunk 08/15/2004 05:59.

(In reply to comment #47)
> (Ctrl+Shift+D is "add bookmark silently")
Huh? Not in my tree.