192223 - Cancel download button should not be click through

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, enhancement, P3)

Description

[Stephane Moureau]

If you click on the Cancel download button (x) in the Download window when that
window is in (local/global) background the download is cancelled.

Until the Cancel button is enhanced to became a Reload when a download has been
interrupted or completed, the click through is really nasty.
Build 2003020607

Comment 1

[Bruce Davidson]

Confirming this on the latest nightly (2003081002). If you're on a 56k modem
cancelling the download of the latest nightly just because the button was in the
background is definitely a "potentially harmful" action, see the HIG chapter 5.

Comment 2

[Josh Aas]

taking

Comment 3

[Josh Aas]

Can't do much about this now since our cancel button is an NSToolbarItem. We
would have to subclass it and somehow replace its internal view with a subclass
of NSView. I'm not gonna go there. I don't think anybody else wants to either.

Comment 4

[Josh Aas]

I have a solution to this bug, but I'm not sure its wanted. NSToolbar items are
clickthrough by default - do we really want the download manager to behave
differently? This comes down to safety vs. consistency.

Simple solution - do nothing in the action method if the window is in the
background. Its a very small patch.

Comment 5

[Stuart Morgan]

As the original description suggests, the real fix is to make downloads
continuable. I really hate buttons that don't work in the background when most
do (such as the 10.3 Finder volume eject buttons).

On the other hand, we could do this as a temporary measure then remove it again
after downloads are restartable.

Comment 6

[Bruce Davidson]

Hmm... it must be possible. Mail.app has a non-click through delete button on
its toolbar, but its other toolbar buttons are click through. Could intercepting
"become foreground window", "no longer foreground window" messages and
enabling/disabling the button be another approach?

Comment 7

[Nick Kreeger]

Looks like mail.app un-enables the delete button if the window is not the
current selected window, maybe an easy patch, I will investigate

Comment 8

[Wevah (Nate Weaver)]

(In reply to comment #7)
> Looks like mail.app un-enables the delete button if the window is not the
> current selected window, maybe an easy patch, I will investigate

Check out the NSToolbarItemValidation protocol.

Comment 9

[Nick Kreeger]

Will make the pause/resume button the same way..

Comment 10

[Nick Kreeger]

Attachment: Turns off click through for all buttons

Turns off pause/resume, open, remove, cancel.. checks for keywindow

Comment 11

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

I want to echo Stuart's statement in comment 5; now that we (presumably) have
pause/resume working, there's little need to disable cancel (unless cancel can't
yet be resumed?), and no need to disable any of the other buttons. 

It's *very* handy to be able to open/show/clean up items without having to bring
the downloads manager to the foreground, and comment 10 would be a real step
backwards.  We should fix bugs, not disable functionality to work around bugs.

Comment 12

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

(In reply to comment #11)
> I want to echo Stuart's statement in comment 5; now that we (presumably) have
> pause/resume working, there's little need to disable cancel (unless cancel can't
> yet be resumed?), and no need to disable any of the other buttons. 
> 
> It's *very* handy to be able to open/show/clean up items without having to bring
> the downloads manager to the foreground, and comment 10 would be a real step
> backwards.  We should fix bugs, not disable functionality to work around bugs.

For me it's all about loss of data. Say we don't disable click through for any
of the buttons. I, as a 56k user (hypothetically), accidentally hit cancel, then
clean up. I've just lost my download and download location. I can't resume that.
Also, if I remember correctly, cancelling currently deletes the partial file.
That may be wrong, so please correct me. 

We can potentially interrupt a user's "flow" if they accidentally hit any button
except "reveal". They can lose data from a cancel, a remove/clean up (in case
they wanted to remember which downloads they'd done recently), and a pause (in
case the server doesn't correctly support pause/resume). In addition, "open" can
be potentially dangerous/unexpected if a user doesn't realize they're opening
the file or even that they downloaded it (our download manager could be in the
background when they click a link).

Does that make sense? We should have more discussion about this before
implementing, that's for sure.

Comment 13

[Wevah (Nate Weaver)]

I don't think that *all* of the buttons need to be disabled when the window is
not key, but the cancel button definitely should be.

Comment 14

[Nick Kreeger]

To make it more clear, this is what the attached patch does:

Disables these buttons for click through:
1. Remove
2. Cancel
3. Pause/Resume
4. Open

These buttons are still enabled for click through:
1. Clean Up
2. Show

Comment 15

[Mike Pinkerton (not reading bugmail)]

cancel is still "dangerous" in that it's not easily resumable (that's another
bug that needs work). so i agree with kreeger's summary in the previous comment.

Comment 16

[Mike Pinkerton (not reading bugmail)]

i'm ok with moving "open" to the list of allowed clickthroughs. i agree it's
useful and we can only do so much to prevent users from downloading and opening
things that are bad.

Comment 17

[Nick Kreeger]

Attachment: Allows open to be click-through

Patch now allows click-through

Comment 18

[Simon Fraser [no longer active]]

So this patch actually disables the buttons when the window is not frontmost?
That's a bit weird, and gives the wrong feedback. I think a custom view with an
-acceptsFirstMouse: override would be preferrable. Is it really hard?

Comment 19

[Wevah (Nate Weaver)]

Probably not, but this is what pretty much every other app with a delete button
in the toolbar does (e.g., Mail).

Comment 20

[Nick Kreeger]

I agree with Wevah, most mac apps have this so someone doesnt accidently press
delete.. An alternative option is to check to see if we are in the key window in
the actual selector that the button executes |ie. -(IBAction) cancel:(id)sender|
and allow the button to actually execute if its the key window, that would get
around having to screw with writing a custom view, and will probly use much less
code just for this simple feature

Comment 21

[Wevah (Nate Weaver)]

IIRC, though, the window becomes key/main before the action is fired.

Comment 22

[Mike Pinkerton (not reading bugmail)]

console app disables the "clear" button when the window is in the bg. bringing
it to the fg enables the button.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 191481 [details] [diff] [review]
Allows open to be click-through

r=pink.

Comment 24

[Mike Pinkerton (not reading bugmail)]

landed.