Audit click-through state of toolbar buttons

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Toolbars & Menus
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Stephane Moureau, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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

Comment 2

14 years ago
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

Comment 4

12 years ago
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)

Updated

11 years ago
Assignee: mikepinkerton → stridey
(Assignee)

Comment 6

11 years ago
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?
(Assignee)

Comment 7

11 years ago
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)
Attachment #226764 - Flags: review?(bugzilla)
(Assignee)

Updated

11 years ago
Attachment #226764 - Flags: review?(bugzilla) → review?(nick.kreeger)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #226764 - Flags: review?(nick.kreeger) → review?(stuart.morgan)

Comment 8

11 years ago
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)
(Assignee)

Comment 9

11 years ago
Created attachment 233403 [details] [diff] [review]
Patch

This is the bitrot police.
Attachment #226764 - Attachment is obsolete: true
Attachment #233403 - Flags: review?(stuart.morgan)

Comment 10

11 years ago
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-
(Assignee)

Comment 11

11 years ago
Created attachment 233406 [details] [diff] [review]
Patch

Also got rid of some redundant code while I was there.
Attachment #233403 - Attachment is obsolete: true
Attachment #233406 - Flags: review?(stuart.morgan)
(Assignee)

Comment 12

11 years ago
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.
Attachment #233406 - Attachment is obsolete: true
Attachment #233411 - Flags: review?(stuart.morgan)
Attachment #233406 - Flags: review?(stuart.morgan)

Comment 13

11 years ago
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-
(Assignee)

Comment 14

11 years ago
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. ;)
Attachment #233411 - Attachment is obsolete: true
Attachment #233420 - Flags: review?(stuart.morgan)

Comment 15

11 years ago
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+
Whiteboard: [needs checkin]

Comment 17

11 years ago
Checked in on the trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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.