Closed Bug 228699 Opened 19 years ago Closed 18 years ago

Download Manager should close on CTRL+Y / ESC

Categories

(Toolkit :: Downloads API, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.7.4

People

(Reporter: athakur, Assigned: steffen.wilberg)

References

()

Details

(Keywords: fixed-aviary1.0, Whiteboard: [have patch])

Attachments

(1 file, 7 obsolete files)

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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
QA Contact: mconnor
Hardware: PC → All
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.
target. 
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Firebird0.10
*** Bug 236398 has been marked as a duplicate of this bug. ***
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?
Not necessarily.  The reason for the Ctrl-E toggle for the DM is the precedent
that was set when the component was a sidebar.
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).
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)
See bug 233440 - Change DL Manager Keybinding.
Summary: Download Manager should close on CTRL-E → Download Manager should close on CTRL+Y
*** Bug 242882 has been marked as a duplicate of this bug. ***
Summary: Download Manager should close on CTRL+Y → Download Manager should close on CTRL+Y / toolbar button
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.
Assignee: bugs → steffen.wilberg
Comment on attachment 150398 [details] [diff] [review]
patch for to make ctrl-y close it

Doesn't include the toolbarbutton yet.
Attachment #150398 - Attachment description: patch → patch for to make ctrl-y close it
*** Bug 248135 has been marked as a duplicate of this bug. ***
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.
Typing in all caps is annoying.  So is going offtopic and ranting about things
outside of the context of a specific bug.
(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.
(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 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.
Attachment #150398 - Flags: review?(mconnor)
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.
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.
QA Contact: mconnor → bmo
Attached patch toggle: almost done (obsolete) — Splinter Review
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.
Attachment #150398 - Attachment is obsolete: true
Attachment #150398 - Flags: review?(mconnor)
Attached patch toggle v1.0 (obsolete) — Splinter Review
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.
Attachment #152652 - Attachment is obsolete: true
Attached patch toggle v1.0.1 (obsolete) — Splinter Review
removed a now unnecessary if.
Attachment #152691 - Attachment is obsolete: true
Attached patch toogle v1.0.2 (obsolete) — Splinter Review
I really hate removing traces of other changes by hand.
Attachment #152694 - Attachment is obsolete: true
Comment on attachment 152695 [details] [diff] [review]
toogle v1.0.2

Here you are!
Attachment #152695 - Flags: review?(mconnor)
Requesting blocking RC1 due to localization impact, and because I wouldn't mind
some public testing.
Flags: blocking-aviary1.0RC1?
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.
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?
We can of course just make Ctrl-Y close it, without changing the toolbarbutton.
See comment 11.
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.
*** Bug 233529 has been marked as a duplicate of this bug. ***
From the dupe: The download manager should be closed on ESC as well.
Summary: Download Manager should close on CTRL+Y / toolbar button → Download Manager should close on CTRL+Y / ESC / toolbar button
Attached patch toggle v1.0.3 (obsolete) — Splinter Review
- makes the changes I mentioned in comment 30.
- makes the download manager close on ESC (like Bookmarks Manager, Page Info,
Options etc.)
Attachment #152695 - Attachment is obsolete: true
Attachment #152695 - Flags: review?(mconnor)
Attachment #153072 - Flags: review?(mconnor)
I knew it.
Attachment #153072 - Attachment is obsolete: true
Attachment #153072 - Flags: review?(mconnor)
Attachment #153186 - Flags: review?(mconnor)
Whiteboard: http://testrunner.mozilla.org/test.cgi?run_id=244#892
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
l10n changes please land soon. 
Flags: blocking-aviary1.0RC1?
Flags: blocking-aviary1.0RC1+
Flags: blocking-aviary1.0-
Flags: blocking-aviary1.0+
Attachment #153186 - Flags: approval-aviary?
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.
Attachment #153186 - Flags: approval-aviary?
Whiteboard: http://testrunner.mozilla.org/test.cgi?run_id=244#892 → [have patch]
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.
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.
Priority: P4 → P3
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 on attachment 153186 [details] [diff] [review]
toggle v1.0.4 (toolkit locale move)

no toggle then.
Attachment #153186 - Attachment is obsolete: true
Attachment #153186 - Flags: review?(mconnor)
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!?
Summary: Download Manager should close on CTRL+Y / ESC / toolbar button → Download Manager should close on CTRL+Y / ESC
This is mostly the first patch, plus ESC.
Comment on attachment 155752 [details] [diff] [review]
just make make Ctrl-y and ESC close it

Explanation is in comment 11 and comment 42.
Attachment #155752 - Flags: review?(bugs)
"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 on attachment 155752 [details] [diff] [review]
just make make Ctrl-y and ESC close it

r+a=ben@mozilla.org
Attachment #155752 - Flags: review?(bugs)
Attachment #155752 - Flags: review+
Attachment #155752 - Flags: approval-aviary+
(Ctrl+Shift+D is "add bookmark silently")
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.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.