Closed Bug 179041 Opened 22 years ago Closed 18 years ago

Audit click-through state of toolbar buttons

Categories

(Camino Graveyard :: Toolbars & Menus, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.5

People

(Reporter: stf, Assigned: froodian)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 4 obsolete files)

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
We should standardize which UI elements act on a background click. Cocoa has a
way to do this (a method on NSView iirc).
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
that's how cocoa toolbars work, all apps behave that way. 

morphing this bug to what we should really be fixing.
Assignee: saari → pinkerton
Summary: Click a Toolbar icon brings window to front and execute the command → When window is bg, clicks out of content perform action, clicks inside just bring window to front
Target Milestone: --- → Camino1.2
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
Updating summary to reflect what this bug is.

-> Toolbars and Menus (targeted for 1.1)
Component: General → Toolbars & Menus
QA Contact: winnie → toolbars
Summary: When window is bg, clicks out of content perform action, clicks inside just bring window to front → Audit click-through state of toolbar buttons
Target Milestone: Camino1.2 → Camino1.1
Assignee: mikepinkerton → stridey
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?
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #226764 - Flags: review?(bugzilla)
Attachment #226764 - Flags: review?(bugzilla) → review?(nick.kreeger)
Status: NEW → ASSIGNED
Attachment #226764 - Flags: review?(nick.kreeger) → review?(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?
Attachment #226764 - Flags: review?(stuart.morgan)
Attached patch Patch (obsolete) — Splinter Review
This is the bitrot police.
Attachment #226764 - Attachment is obsolete: true
Attachment #233403 - Flags: review?(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?
Attachment #233403 - Flags: review?(stuart.morgan) → review-
Attached patch Patch (obsolete) — Splinter Review
Also got rid of some redundant code while I was there.
Attachment #233403 - Attachment is obsolete: true
Attachment #233406 - Flags: review?(stuart.morgan)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #233406 - Attachment is obsolete: true
Attachment #233411 - Flags: review?(stuart.morgan)
Attachment #233406 - Flags: review?(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.
Attachment #233411 - Flags: review?(stuart.morgan) → review-
Attached patch PatchSplinter Review
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. ;)
Attachment #233411 - Attachment is obsolete: true
Attachment #233420 - Flags: review?(stuart.morgan)
Comment on attachment 233420 [details] [diff] [review]
Patch

r=me
Attachment #233420 - Flags: superreview?(mikepinkerton)
Attachment #233420 - Flags: review?(stuart.morgan)
Attachment #233420 - Flags: review+
Comment on attachment 233420 [details] [diff] [review]
Patch

sr=pink
Attachment #233420 - Flags: superreview?(mikepinkerton) → superreview+
Checked in on the trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: