392188 - Switching applications: Firefox needs double click if not active window (buttons aren't click-through)

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[will-i-am]

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

[Jesse Ruderman]

Firefox's behavior is pretty standard on Mac.  I'm surprised that Keynote and Pages act differently.

Comment 2

[Colin Barrett [:cbarrett]]

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

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

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.

Comment 4

[Colin Barrett [:cbarrett]]

I wonder if this is a widget level thing?

Comment 5

[will-i-am]

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.

Comment 6

[Robbin]

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.

Comment 7

[Markus Stange [:mstange]]

Attachment: fix

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.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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.

Comment 9

[Markus Stange [:mstange]]

(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

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 12

[Robert Ginda]

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

[Neil Deakin]

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.

Comment 14

[Markus Stange [:mstange]]

Thanks Neil, that's really interesting. I added your comment to bug 449956.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Markus, any update here?

Comment 16

[Markus Stange [:mstange]]

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.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Markus, do we have any of these points filed as separate bugs?

Comment 19

[Markus Stange [:mstange]]

No. And I don't think these points make sense on their own.

Comment 20

[Neil Deakin]

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.

Comment 21

[Markus Stange [:mstange]]

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

[Neil Deakin]

On Windows and Linux, mousemove events are normally sent to background windows already. Maybe not on Mac?

Comment 23

[Markus Stange [:mstange]]

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.

Comment 24

[Markus Stange [:mstange]]

Attachment: part 1, v1: widget part

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.

Comment 25

[Markus Stange [:mstange]]

Attachment: part 2, v1: allowclickthrough attribute

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.

Comment 26

[Markus Stange [:mstange]]

Attachment: part 3, v1: block click through to <browser> by default

Comment 27

[Markus Stange [:mstange]]

Attachment: part 4, v1: block click-through to tab closebuttons

Tab close buttons are destructive, so we should probably disallow them for click-through.

Comment 28

[Markus Stange [:mstange]]

Attachment: part 5, v1: block click-through to trees by default

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.

Comment 29

[Markus Stange [:mstange]]

Attachment: part 6, v1: allow click-through to scrollbars even inside otherwise blocked areas

This should maybe go into bug 404267, but here's where the action is.

Comment 30

[Markus Stange [:mstange]]

I'll start writing tests now.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 32

[Markus Stange [:mstange]]

I don't see anything wrong with allowing content XUL to influence click-through behavior - do you?

Comment 33

[Josh Aas]

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.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

(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

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 36

[Markus Stange [:mstange]]

Attachment: part 1, updated to trunk

Comment 37

[Markus Stange [:mstange]]

Attachment: part 2, iterative

Comment 38

[Markus Stange [:mstange]]

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/[e-mail]-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

Comment 39

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 451728 [details] [diff] [review]
part 1, updated to trunk

I'm on windows now, redirecting to Limi for the review

Comment 40

[Neil Deakin]

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?

Comment 41

[Markus Stange [:mstange]]

(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 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 43

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 451728 [details] [diff] [review]
part 1, updated to trunk

Looks good to me.

Comment 44

[Alex Limi (:limi) — Firefox UX Team]

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.

Comment 45

[Markus Stange [:mstange]]

Attachment: part 2 with "clickthrough" instead of "allowclickthrough"

Comment 46

[Markus Stange [:mstange]]

Attachment: part 3 with "clickthrough" instead of "allowclickthrough"

Comment 47

[Markus Stange [:mstange]]

Attachment: part 4 with "clickthrough" instead of "allowclickthrough"

Comment 48

[Markus Stange [:mstange]]

Attachment: part 5 with "clickthrough" instead of "allowclickthrough"

Comment 49

[Markus Stange [:mstange]]

Attachment: part 6 with "clickthrough" instead of "allowclickthrough" and clickthrough="always" on scrollcorner / resizer

(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.

Comment 50

[Markus Stange [:mstange]]

Attachment: part 7: fix up mouseup and doubleclick

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

Comment 51

[Markus Stange [:mstange]]

Attachment: part 8: test changes

This should also fix bug 557986.

Comment 52

[Josh Aas]

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.

Comment 53

[Markus Stange [:mstange]]

Attachment: part 17: part 1 and 7 merged for ease of reviewing

Comment 54

[Markus Stange [:mstange]]

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

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 57

[Markus Stange [:mstange]]

The test covers it pretty much, I don't think we need anything else.

Comment 58

[:Gavin Sharp [email: [e-mail]]]

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.