Closed
Bug 392188
Opened 17 years ago
Closed 14 years ago
Switching applications: Firefox needs double click if not active window (buttons aren't click-through)
Categories
(Core :: Widget: Cocoa, enhancement)
Tracking
()
VERIFIED
FIXED
mozilla2.0b2
People
(Reporter: i-am-will, Assigned: mstange)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-needed)
Attachments
(7 files, 10 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
When you're using 2 apps, and switching between both I noticed that apps like Keynote and Pages tend to do directly what you clicked, but firefox needs an extra click.
Reproducible: Always
Steps to Reproduce:
1. Start firefox, start pages
2. Do something in firefox, then click in pages window
3. Then try to e.g. reload the firefox page.
Actual Results:
You need to click on firefox to make it active, and click again to do what you want it to do.
Expected Results:
Directly execute my command, e.g. reload.
Maybe nice to make this 'optional' to not interfere with people who like this behavior in a webbrowser.
Comment 1•17 years ago
|
||
Firefox's behavior is pretty standard on Mac. I'm surprised that Keynote and Pages act differently.
Comment 2•17 years ago
|
||
Actually, standard toolbar items on the mac operate that way, as do close buttons and such. Tab switching in Adium as well.
Clicking on refresh when the window doesn't have focus should cause the page to refresh.
At least, IMO.
Comment 3•17 years ago
|
||
I agree with Colin here (confirming this bug). Some apps disallow specific buttons and actions from being "click-through", but on a whole, they're allowed.
Summary: Switching applications: Firefox needs double click if not active window → Switching applications: Firefox needs double click if not active window (buttons aren't click-through)
Updated•17 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•17 years ago
|
||
I wonder if this is a widget level thing?
Just be sure to implement it optional (maybe default on). Guys could have gotten used to this.
==
Maybe this is also very related:
http://www.youtube.com/watch?v=lGCCMx2ck2E
something i noticed a while ago, that when using expose firefox still thinks it's using the full screen. I demonstrate this by dragging a file on the screen, and firefox (still in expose-mode) thinks it should start acting.
I have a similar problem. If using cmd+tab to go between applications, IF a Firefox window appears, it is in active and I must click on the title bar. Clicking on any other part of the window does not bring it to the front.
MOST times, I get no active window at all (but Firefox in the menu) and then must cmd+tilde to go through the pages. Using the WINDOW menu will not allow me to get to the current page. Even if only one window is open, I have to hit cmd+tilde.
Assignee | ||
Comment 7•16 years ago
|
||
This patch makes click-through work - both on browser chrome and on web content.
The question is: Do we want it?
I think we do. All Cocoa applications I've tested support click-through - Carbon applications (e.g. Finder and iTunes) don't.
Safari only supports click-through on chrome, not on web content. Opera supports it on both.
Unsurprisingly, the patch also fixes bug 404267.
Assignee | ||
Updated•16 years ago
|
Assignee: mstange → joshmoz
Status: ASSIGNED → NEW
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
Hardware: Macintosh → All
Version: unspecified → Trunk
Assignee | ||
Updated•16 years ago
|
Assignee: joshmoz → mstange
(In reply to comment #7)
> The question is: Do we want it?
> I think we do. All Cocoa applications I've tested support click-through -
> Carbon applications (e.g. Finder and iTunes) don't.
Click-through is automatic in Cocoa (and must be disabled where click-through is undesirable), whereas in Carbon click-through had to be enabled specifically on controls.
> Safari only supports click-through on chrome, not on web content. Opera
> supports it on both.
I'd be inclined to argue that we should disable click-through on <input type="button|image|submit|cancel"> and on <button> in content, since those are likely to be destructive (or at least non-undoable; think click-through to purchase) and since we can't infer safeness since we don't control the context (whereas in chrome we can make that decision when building chrome), but other form controls and scrollbars should be safe for click-through in content.
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #8)
> I'd be inclined to argue that we should disable click-through on <input
> type="button|image|submit|cancel"> and on <button> in content, since those are
> likely to be destructive
Yeah, doing that would probably better - but a lot harder. Do you think we can get away with postponing that functionality for a later bug?
Comment 10•16 years ago
|
||
Comment on attachment 326174 [details] [diff] [review]
fix
I'm inclined to think we shouldn't do this until we can do it right.
And I agree with Smokey that we should probably only selectively
enable click-through.
Also, since this is a major (and very visible) change in UI behavior,
it will need _lots_ of testing -- "live" (open-ended) testing, not
automated testing. It'll be a while before I have time to do this (a
month or two) ... and I won't feel comfortable with this change until
I _have_ done it.
Finally, Markus, you do something really peculiar in your patch -- you
take a mouse-down event and send it as a mouse-moved event. You
should avoid that (if at all possible). A lot of the event-handling
infrastructure isn't visible on the surface (for example every NSEvent
has an undocumented EventRef variable). So even if the documented
NSEvent fields make it seem you can get away with this, you may be
wreaking havoc somewhere under the hood.
Attachment #326174 -
Flags: review?(smichaud) → review-
Comment 12•16 years ago
|
||
I'm most interested in being able to initiate a drag from an unfocused window like in Finder and ff2. I don't care much about web content click through per-se, but I'd be willing to live with it if it meant I could start a drag without focus.
Perhaps the code could disallow web click through but allow "drag through". An easier solution might be to have three settings for the pref, "no click through", "chrome click through only", and "all click through".
Comment 13•16 years ago
|
||
Note that Mozilla used to have the capability to drag from inactive windows long (long long) ago, but the code isn't being used any more. The are remnants of this in the windows widget code.
What this does is fire a NS_MOUSE_ACTIVATE event and the responder sets the acceptActivation field to false if it doesn't want the window to come to the front. On Windows, a number of flags can be set which identify whether the window should be activated and/or the mouse event fired or ignored, so this managed to allow drags from background windows.
I'm not sure how Mac specifies this, but we should either make use of this or a similar technique such that UI elements can either set an attribute or respond to an event to indicate what behaviour is desired.
Assignee | ||
Comment 14•16 years ago
|
||
Thanks Neil, that's really interesting. I added your comment to bug 449956.
Assignee | ||
Comment 16•16 years ago
|
||
I'm not working on this at the moment. It's not really trivial:
1. We need to decide which areas accept click-through and which don't.
2. In order to block certain events from reaching certain areas, we need to
create a new attribute for mouse events (maybe in nsIDOMNSMouseEvent)
3. We need to make sure that blocking of the :active pseudo class works, too.
4. We also need to mark the corresponding mouse up and mouse click events, not
only the mouse down event.
5. We should decide what to do with mouse move events on background windows.
If a background window accepts click-through, this is usually reflected
with correct feedback on mouse over.
Status: ASSIGNED → NEW
Updated•16 years ago
|
Assignee: mstange → joshmoz
Comment 18•16 years ago
|
||
Markus, do we have any of these points filed as separate bugs?
Assignee | ||
Comment 19•16 years ago
|
||
No. And I don't think these points make sense on their own.
Comment 20•15 years ago
|
||
I think what should happen here is:
- if the window is already frontmost, do the normal behaviour
- if the window is in the background, fire an event, say mouseactivate, when the mouse is pressed down on it.
- the default behaviour of the event is to activate the window and fire the mouse events as normal.
- if the event is cancelled, the window is not brought to the front. Platforms that don't support this could ignore it. The event cannot be cancelled by content script.
- a field, preventMouseEvent, can be set to true to prevent the mousedown/up/etc events from firing.
- another field could be added to handle delayed drag activation for bug 449956.
- something similar could be done when rolling up popups so as to allow more control over whether the mouse event is ignored or not.
Assignee | ||
Comment 21•15 years ago
|
||
Sounds good!
Here's another thing to think about: If a button allows click-through, it should ideally give appropriate mouse-over feedback prior to being clicked, which requires that mouse *move* events are also sent to background windows, too. With the approach you suggested we could send a mouseactivate event for every mouse move event... but then the name "mouseactivate" no longer fits. Thoughts?
Comment 22•15 years ago
|
||
On Windows and Linux, mousemove events are normally sent to background windows already. Maybe not on Mac?
Assignee | ||
Comment 23•15 years ago
|
||
We explicitly block them on Mac, because as long as we don't allow click-through (this bug), it doesn't make sense to have mouse-over feedback. It's all the same thing :)
In comment 21 I was trying to say that we just need to apply your idea from comment 20 to mouse move events, too, not only to mousedown/up/click events.
Assignee | ||
Comment 24•14 years ago
|
||
Send an NS_MOUSE_ACTIVATE event for every mousemove or mousedown event on background windows. If the activate event hasn't been preventDefault()ed, send the mouse event.
At the moment NS_MOUSE_ACTIVATE events don't get processed in any way, so this patch alone will simply accept background window mouse events everywhere.
Assignee: nobody → mstange
Attachment #326174 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #448487 -
Flags: review?(joshmoz)
Assignee | ||
Comment 25•14 years ago
|
||
Add a XUL attribute called "allowclickthrough". By default, everything allows click-through, but you can override that behavior from the inside by setting the attribute to "always" or "never". For example, the <browser> of a tab shouldn't accept click-through, so it will get allowclickthrough="never", but scrollbars inside the browser should still allow it (see bug 404267), so they'll get allowclickthrough="always".
Set the event status to nsEventStatus_eConsumeNoDefault when click-through is not allowed so that widget doesn't send the mouse event.
Attachment #448490 -
Flags: review?(Olli.Pettay)
Attachment #448490 -
Flags: feedback?(enndeakin)
Assignee | ||
Comment 26•14 years ago
|
||
Assignee | ||
Comment 27•14 years ago
|
||
Tab close buttons are destructive, so we should probably disallow them for click-through.
Assignee | ||
Comment 28•14 years ago
|
||
This is what native applications seem to be doing. They also usually block click-through to sidebars, and since all the XUL sidebars I know are trees, this matches quite well.
Assignee | ||
Comment 29•14 years ago
|
||
This should maybe go into bug 404267, but here's where the action is.
Assignee | ||
Updated•14 years ago
|
Attachment #448492 -
Attachment is patch: true
Attachment #448492 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 30•14 years ago
|
||
I'll start writing tests now.
Comment 31•14 years ago
|
||
Comment on attachment 448490 [details] [diff] [review]
part 2, v1: allowclickthrough attribute
>+static bool
>+NodeAllowsClickThrough(nsINode* aNode)
>+{
>+ if (!aNode)
>+ return true;
>+
>+ if (aNode->IsElement() && aNode->AsElement()->IsXUL()) {
>+ mozilla::dom::Element* element = aNode->AsElement();
>+ static nsIContent::AttrValuesArray strings[] =
>+ {&nsGkAtoms::always, &nsGkAtoms::never, nsnull};
>+ switch (element->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::allowclickthrough,
>+ strings, eCaseMatters)) {
>+ case 0:
>+ return true;
>+ case 1:
>+ return false;
>+ }
>+ }
>+ return NodeAllowsClickThrough(nsContentUtils::GetCrossDocParentNode(aNode));
>+}
Could you please change this from recursive to iterative.
Recursion could become quite deep.
And do we want that content pages can affect to allowclickthrough behavior?
Or should there be also a check that the XUL element is either in chrome document
or native anonymous?
Assignee | ||
Comment 32•14 years ago
|
||
I don't see anything wrong with allowing content XUL to influence click-through behavior - do you?
Comment 33•14 years ago
|
||
Comment on attachment 448487 [details] [diff] [review]
part 1, v1: widget part
I don't have an opinion on the larger issue here but I'm fine with this widget code.
Attachment #448487 -
Flags: review?(joshmoz) → review+
Comment 34•14 years ago
|
||
(In reply to comment #32)
> I don't see anything wrong with allowing content XUL to influence click-through
> behavior - do you?
Maybe not. I was just wondering whether you had thought about that.
Could you still change the method to iterative.
Comment 35•14 years ago
|
||
Comment on attachment 448490 [details] [diff] [review]
part 2, v1: allowclickthrough attribute
With iterative method, r=me, but would be great to get Enn's comments about this.
Attachment #448490 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 36•14 years ago
|
||
Attachment #448487 -
Attachment is obsolete: true
Assignee | ||
Comment 37•14 years ago
|
||
Attachment #448490 -
Attachment is obsolete: true
Attachment #451730 -
Flags: feedback?(enndeakin)
Attachment #448490 -
Flags: feedback?(enndeakin)
Assignee | ||
Comment 38•14 years ago
|
||
Comment on attachment 451728 [details] [diff] [review]
part 1, updated to trunk
Alex, can you UI-review this change? Here's a tryserver build for testing that also includes a new toolbarbutton patch for bug 559033:
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-0641541dff84/tryserver-macosx/firefox-3.7a6pre.en-US.mac.dmg
Change summary:
Before the patches in this bug all clicks on background windows were ignored - the window becomes active, but the click has no effect. With these patches, elements in the window chrome become clickable even on inactive windows. The following things still block click-through: the browser content area, tab close buttons and XUL trees. Scrollbars allow click-through even when they're inside a non-click-through area.
Native behavior:
Native Carbon applications usually don't allow click-through on anything. This also reflects on their visuals: For example, black text in background windows becomes gray, clear pill buttons become more transparent, and glyphs on window frame buttons dim. Examples of Carbon windows are the Finder on 10.5 and the iTunes preferences window. (The iTunes main window allows click-through even though it's Carbon because iTunes explicitly overrides the default Carbon behavior there. This is new in iTunes 9 - iTunes 8 didn't allow click-through in its main window, if I recall correctly.)
Native *Cocoa* applications, however, enable click-through by default. An inactive Cocoa window looks more clickable than an inactive Carbon window: In Cocoa apps, controls don't "dim" - they don't become transparent, they just lose their blue fill. Examples for Cocoa apps are the Finder on 10.6 and pretty much every other Mac OS X app you can think of.
I mentioned bug 559033 because there I'm going to change the inactive appearance of the main window toolbarbuttons: When the window becomes inactive, the button itself becomes lighter, but the glyph on it stays dark (at least for buttons that aren't disabled). That's how click-through buttons signal their click-throughability; you can also see this in 10.6 Finder and Safari.
For native-themed controls, like pill buttons, <select> boxes and checkboxes, we already have the click-through look, but not the click-through behavior. The patches in this bug make us consistent.
(Except for the browser content area: Controls there still look clickable, but they aren't. Webkit does the same.)
Comparison to other browsers:
Both Safari and Chrome allow click-through to their window frame but not to their content area. They even allow click-through on tab close buttons, which I think is not a good idea.
Apple HIG about click-through:
http://developer.apple.com/mac/library/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGWindows/XHIGWindows.html#//apple_ref/doc/uid/20000961-TPXREF20
Attachment #451728 -
Flags: ui-review?(faaborg)
Assignee | ||
Updated•14 years ago
|
Attachment #448491 -
Flags: review?(enndeakin)
Assignee | ||
Updated•14 years ago
|
Attachment #448492 -
Flags: review?(enndeakin)
Assignee | ||
Updated•14 years ago
|
Attachment #448493 -
Flags: review?(enndeakin)
Assignee | ||
Updated•14 years ago
|
Attachment #448494 -
Flags: review?(roc)
Comment 39•14 years ago
|
||
Comment on attachment 451728 [details] [diff] [review]
part 1, updated to trunk
I'm on windows now, redirecting to Limi for the review
Attachment #451728 -
Flags: ui-review?(faaborg) → ui-review?(limi)
Comment 40•14 years ago
|
||
Comment on attachment 451730 [details] [diff] [review]
part 2, iterative
Looks ok. I was expecting an additional event though, but this seems good too.
You could just use 'clickthough' instead of 'allowclickthrough'.
The possible values are:
- always: click and focus window
- never: ignore click and focus window
The intent of not using a boolean is that we could use additional values, right?
Attachment #451730 -
Flags: feedback?(enndeakin) → feedback+
Assignee | ||
Comment 41•14 years ago
|
||
(In reply to comment #40)
> You could just use 'clickthough' instead of 'allowclickthrough'.
Yeah that sounds good, too.
> The possible values are:
> - always: click and focus window
> - never: ignore click and focus window
>
> The intent of not using a boolean is that we could use additional values,
> right?
The intent was that this can express three states:
- always: ...
- never: ...
- not set: do what the parent does.
I didn't want to use true, false and not set because for boolean XUL attributes you usually assume that false is the same as not set.
But leaving room for additional values is also a good reason.
Comment on attachment 448494 [details] [diff] [review]
part 6, v1: allow click-through to scrollbars even inside otherwise blocked areas
Looks good.
Should we allow clickthrough on resizers/scrollbar-corners too?
Attachment #448494 -
Flags: review?(roc) → review+
Comment 43•14 years ago
|
||
Comment on attachment 451728 [details] [diff] [review]
part 1, updated to trunk
Looks good to me.
Attachment #451728 -
Flags: ui-review?(limi) → ui-review+
Comment 44•14 years ago
|
||
I obviously can't speak to code quality, but the behavior summary seems sane. Just noting it here, since there were multiple attachments, but only one ui-r requested.
Assignee | ||
Comment 45•14 years ago
|
||
Attachment #451730 -
Attachment is obsolete: true
Assignee | ||
Comment 46•14 years ago
|
||
Attachment #448491 -
Attachment is obsolete: true
Attachment #454821 -
Flags: review?(enndeakin)
Attachment #448491 -
Flags: review?(enndeakin)
Assignee | ||
Comment 47•14 years ago
|
||
Attachment #448492 -
Attachment is obsolete: true
Attachment #454822 -
Flags: review?(enndeakin)
Attachment #448492 -
Flags: review?(enndeakin)
Assignee | ||
Comment 48•14 years ago
|
||
Attachment #448493 -
Attachment is obsolete: true
Attachment #454823 -
Flags: review?(enndeakin)
Attachment #448493 -
Flags: review?(enndeakin)
Assignee | ||
Comment 49•14 years ago
|
||
(In reply to comment #42)
> Should we allow clickthrough on resizers/scrollbar-corners too?
Good idea! Doing that here. Clicking resizers accidentally doesn't do anything, so it's not destructive, and dragging resizers in background windows is hardly ever accidental.
Attachment #448494 -
Attachment is obsolete: true
Assignee | ||
Comment 50•14 years ago
|
||
Here's an additional patch on top of part 1 that fixes some issues I found while I wrote the test:
- Click-through detection on panels doesn't work. Add a workaround.
- If mousedown was blocked, mouseup should be blocked, too.
- If the first click of a double-click was blocked, the second click shouldn't
send a double click. Decreasing the click count will fix that.
fixes bug 470130
Attachment #454827 -
Flags: review?(joshmoz)
Assignee | ||
Comment 51•14 years ago
|
||
This should also fix bug 557986.
Updated•14 years ago
|
Attachment #454823 -
Flags: review?(enndeakin) → review+
Updated•14 years ago
|
Attachment #454822 -
Flags: review?(enndeakin) → review+
Updated•14 years ago
|
Attachment #454821 -
Flags: review?(enndeakin) → review+
Comment 52•14 years ago
|
||
Comment on attachment 454827 [details] [diff] [review]
part 7: fix up mouseup and doubleclick
This is hard to review as it isn't against trunk code. Please request review on the full patch for Cocoa widgets.
Attachment #454827 -
Flags: review?(joshmoz)
Assignee | ||
Comment 53•14 years ago
|
||
Attachment #451728 -
Attachment is obsolete: true
Attachment #454827 -
Attachment is obsolete: true
Attachment #456314 -
Flags: review?(joshmoz)
Attachment #456314 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 54•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e5c19ff58cf9
http://hg.mozilla.org/mozilla-central/rev/e18854fb9aed
http://hg.mozilla.org/mozilla-central/rev/ffbbedb69cac
http://hg.mozilla.org/mozilla-central/rev/3a1683e3fcfa
http://hg.mozilla.org/mozilla-central/rev/e491889be2ec
http://hg.mozilla.org/mozilla-central/rev/ced41ebe9e75
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 55•14 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre
Marcia, do we add things like that to the Snow Leopard test suite on Litmus?
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Comment 56•14 years ago
|
||
(In reply to comment #55)
> Marcia, do we add things like that to the Snow Leopard test suite on Litmus?
As seen we have automated tests for that implementation. Markus, would that need manual tests or have we covered that feature completely?
Flags: in-testsuite+
Assignee | ||
Comment 57•14 years ago
|
||
The test covers it pretty much, I don't think we need anything else.
Updated•14 years ago
|
Flags: in-litmus? → in-litmus-
Target Milestone: --- → mozilla2.0b2
Comment 58•13 years ago
|
||
Looks like part 6 landed using the wrong attribute in scrollbar.xml (http://hg.mozilla.org/mozilla-central/rev/f1cd1c78ebd3) - allowclickthrough vs. clickthrough. I filed bug 738097.
Updated•11 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•