Last Comment Bug 179041 - Audit click-through state of toolbar buttons
: Audit click-through state of toolbar buttons
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Toolbars & Menus (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal with 1 vote (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
http://bugzilla.mozilla.org/enter_bug...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-11-08 01:04 PST by Stephane Moureau
Modified: 2006-08-15 08:40 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (3.20 KB, patch)
2006-06-22 23:18 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (3.13 KB, patch)
2006-08-12 14:36 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (5.00 KB, patch)
2006-08-12 16:04 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Patch (5.00 KB, patch)
2006-08-12 16:08 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Patch (5.18 KB, patch)
2006-08-12 18:32 PDT, froodian (Ian Leue)
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Stephane Moureau 2002-11-08 01:04:36 PST
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 User image Simon Fraser 2003-01-01 23:15:53 PST
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 User image Lorin Rivers 2003-04-11 12:12:33 PDT
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 User image Mike Pinkerton (not reading bugmail) 2003-08-11 18:15:02 PDT
that's how cocoa toolbars work, all apps behave that way. 

morphing this bug to what we should really be fixing.
Comment 4 User image Paul Davidson 2005-04-12 19:34:20 PDT
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 User image Samuel Sidler (old account; do not CC) 2006-03-23 21:28:06 PST
Updating summary to reflect what this bug is.

-> Toolbars and Menus (targeted for 1.1)
Comment 6 User image froodian (Ian Leue) 2006-06-16 22:09:55 PDT
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 User image froodian (Ian Leue) 2006-06-22 23:18:11 PDT
Created attachment 226764 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-12 11:35:44 PDT
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 User image froodian (Ian Leue) 2006-08-12 14:36:46 PDT
Created attachment 233403 [details] [diff] [review]
Patch

This is the bitrot police.
Comment 10 User image Stuart Morgan 2006-08-12 15:14:30 PDT
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 User image froodian (Ian Leue) 2006-08-12 16:04:46 PDT
Created attachment 233406 [details] [diff] [review]
Patch

Also got rid of some redundant code while I was there.
Comment 12 User image froodian (Ian Leue) 2006-08-12 16:08:43 PDT
Created attachment 233411 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-12 18:07:07 PDT
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 User image froodian (Ian Leue) 2006-08-12 18:32:14 PDT
Created attachment 233420 [details] [diff] [review]
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 User image Stuart Morgan 2006-08-12 19:28:10 PDT
Comment on attachment 233420 [details] [diff] [review]
Patch

r=me
Comment 16 User image Mike Pinkerton (not reading bugmail) 2006-08-14 05:37:53 PDT
Comment on attachment 233420 [details] [diff] [review]
Patch

sr=pink
Comment 17 User image Mark Mentovai 2006-08-15 08:40:25 PDT
Checked in on the trunk and MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.