179041 - Audit click-through state of toolbar buttons

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Stephane Moureau]

When a window is not the frontmost one, clicking on it does not make anything that
bringing the window to the front except when you make a click on one icon inf
the Toolbar.
Eg: Clicking on the Sidebar bar, brings the window to the front and toggle the
Sidebar.
That's not the case if you click on an item (eg: button) of a form, the window
is bring to the front but the command is not executed.
Build 1107 OS X 10.2.1

Comment 1

[Simon Fraser [no longer active]]

We should standardize which UI elements act on a background click. Cocoa has a
way to do this (a method on NSView iirc).

Comment 2

[Lorin Rivers]

I don't like that the toolbar items are active when the window is not. I find
myself wondering why I'm looking at the bookmarks management UI when what I
wanted was to simply bring that window to the front (I find myself clicking the
mis-named "toolbar" item) because it seems to be the bit I can see and ID often.

Comment 3

[Mike Pinkerton (not reading bugmail)]

that's how cocoa toolbars work, all apps behave that way. 

morphing this bug to what we should really be fixing.

Comment 4

[Paul Davidson]

If I'm not mistaken, Cocoa lets you choose which toolbar buttons have
click-through and which don't. I also think it's recommended not to allow any
unwanted action to be clicked through (actived when the window is inactive).

The way my toolbar is set up, I have "Bookmarks" and the Progress icon to the
right of the address bar. There's no functional reason I would want to edit
bookmarks or go to the Camino website (which clicking the Progress icon does)
when the application is in the background; however, I frequently click them by
accident because there's not always much of the Camino window showing when I
want to activate it.

In short, there's no reason for the Camino toolbar to allow click-through, and
plenty of reasons not to. I consider this a bug.

Anyone who doesn't know what's wrong with click-through should read John
Gruber's comments: http://daringfireball.net/2003/05/the_problems_with_clickthrough

Comment 5

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

Updating summary to reflect what this bug is.

-> Toolbars and Menus (targeted for 1.1)

Comment 6

[froodian (Ian Leue)]

Per IRC, we should disable click-through for the "close tab" button from the main toolbar, and the "trash" button from the downloads toolbar.

The downloads window's "Remove", "Cancel", and "Pause/Resume" buttons already have click-through disabled from bug 192223.

Should "Clean up" be disabled too, since it's destructive until we can restart and resume cancelled/failed downloads?  I certainly enjoy being able to click-through to it, but I'm not sure it's really "correct" in this situation.  Thoughts?

Comment 7

[froodian (Ian Leue)]

Attachment: Patch

Does everything in the above comment (including the clean up item, with a note to make it click-through again once it's not destructive)

Comment 8

[Stuart Morgan]

Comment on attachment 226764 [details] [diff] [review]
Patch

This looks reasonable, but I unfortunately didn't get to it before it rotted.  Could you respin?

Comment 9

[froodian (Ian Leue)]

Attachment: Patch

This is the bitrot police.

Comment 10

[Stuart Morgan]

Comment on attachment 233403 [details] [diff] [review]
Patch

This doesn't actually work for the trash icon, since there's no case to call that method in validateToolbarItem: (only the menu version).

Also, the key window checks need to be in validateToolbarItem:, not in the shouldAllowFoo methods, because otherwise they will disable items in the context menu when the window is in the background, which there's no need to do. Can you fix cancel and remove when you change the patch?

Comment 11

[froodian (Ian Leue)]

Attachment: Patch

Also got rid of some redundant code while I was there.

Comment 12

[froodian (Ian Leue)]

Attachment: Patch

Because of the construction of |validateToolbarItem|, that if clause isn't actually redundant.  It seems like a less-than ideal way to write that method, but I'll leave that for another day.

Comment 13

[Stuart Morgan]

Comment on attachment 233411 [details] [diff] [review]
Patch

>   // validate items not dependent on the current selection
>   if (action == @selector(cleanUpDownloads:)) {
>     unsigned pcControllersCount = [mProgressViewControllers count];
>     for (unsigned i = 0; i < pcControllersCount; i++)
>     {
>       ProgressViewController* curController = [mProgressViewControllers objectAtIndex:i];
>       if ((![curController isActive]) || [curController isCanceled])
>-        return YES;
>+        return [[self window] isKeyWindow]; //XXX Go back to 'YES' once we can resume cancelled/failed downloads
>     }
>     return NO;
>   }

Never put off until tomorrow, and all that ;)  Checking inside the loop doesn't make sense, so just make the first thing in the |if| block a return NO if ![[self window isKeyWindow].

Other than that, looks good.

Comment 14

[froodian (Ian Leue)]

Attachment: Patch

I can't figure out a way to get rid of the |showWindow| validation check without making it more ugly elsewhere.  However, I did shift a couple things to make it obvious what's going on.

Oh yeah, and addresses smorgan's comment. ;)

Comment 15

[Stuart Morgan]

Comment on attachment 233420 [details] [diff] [review]
Patch

r=me

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 233420 [details] [diff] [review]
Patch

sr=pink

Comment 17

[Mark Mentovai]

Checked in on the trunk and MOZILLA_1_8_BRANCH.