Closed Bug 392188 Opened 12 years ago Closed 10 years ago

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

Categories

(Core :: Widget: Cocoa, enhancement)

All
macOS
enhancement
Not set

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)

5.98 KB, patch
Details | Diff | Splinter Review
841 bytes, patch
enndeakin
: review+
Details | Diff | Splinter Review
800 bytes, patch
enndeakin
: review+
Details | Diff | Splinter Review
885 bytes, patch
enndeakin
: review+
Details | Diff | Splinter Review
4.02 KB, patch
Details | Diff | Splinter Review
26.52 KB, patch
Details | Diff | Splinter Review
14.59 KB, 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.
Firefox's behavior is pretty standard on Mac.  I'm surprised that Keynote and Pages act differently.
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.
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attached patch fix (obsolete) — Splinter Review
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: nobody → mstange
Status: NEW → ASSIGNED
Attachment #326174 - Flags: review?(smichaud)
Assignee: mstange → joshmoz
Status: ASSIGNED → NEW
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
Hardware: Macintosh → All
Version: unspecified → Trunk
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.
(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 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-
Duplicate of this bug: 442535
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".
Blocks: 465590
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.
Thanks Neil, that's really interesting. I added your comment to bug 449956.
Markus, any update here?
Status: NEW → ASSIGNED
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
Assignee: mstange → joshmoz
Assignee: joshmoz → nobody
Duplicate of this bug: 383908
Markus, do we have any of these points filed as separate bugs?
No. And I don't think these points make sense on their own.
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.
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?
On Windows and Linux, mousemove events are normally sent to background windows already. Maybe not on Mac?
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.
Attached patch part 1, v1: widget part (obsolete) — Splinter Review
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)
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)
Tab close buttons are destructive, so we should probably disallow them for click-through.
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.
This should maybe go into bug 404267, but here's where the action is.
Attachment #448492 - Attachment is patch: true
Attachment #448492 - Attachment mime type: application/octet-stream → text/plain
Blocks: 404267
I'll start writing tests now.
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?
I don't see anything wrong with allowing content XUL to influence click-through behavior - do you?
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+
(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 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+
Attached patch part 1, updated to trunk (obsolete) — Splinter Review
Attachment #448487 - Attachment is obsolete: true
Attached patch part 2, iterative (obsolete) — Splinter Review
Attachment #448490 - Attachment is obsolete: true
Attachment #451730 - Flags: feedback?(enndeakin)
Attachment #448490 - Flags: feedback?(enndeakin)
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)
Attachment #448491 - Flags: review?(enndeakin)
Attachment #448492 - Flags: review?(enndeakin)
Attachment #448493 - Flags: review?(enndeakin)
Attachment #448494 - Flags: review?(roc)
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 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+
(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 on attachment 451728 [details] [diff] [review]
part 1, updated to trunk

Looks good to me.
Attachment #451728 - Flags: ui-review?(limi) → ui-review+
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.
Attachment #448491 - Attachment is obsolete: true
Attachment #454821 - Flags: review?(enndeakin)
Attachment #448491 - Flags: review?(enndeakin)
Attachment #448492 - Attachment is obsolete: true
Attachment #454822 - Flags: review?(enndeakin)
Attachment #448492 - Flags: review?(enndeakin)
Attachment #448493 - Attachment is obsolete: true
Attachment #454823 - Flags: review?(enndeakin)
Attachment #448493 - Flags: review?(enndeakin)
(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
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)
Blocks: 470130
This should also fix bug 557986.
Blocks: 557986
Attachment #454823 - Flags: review?(enndeakin) → review+
Attachment #454822 - Flags: review?(enndeakin) → review+
Attachment #454821 - Flags: review?(enndeakin) → review+
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)
Attachment #451728 - Attachment is obsolete: true
Attachment #454827 - Attachment is obsolete: true
Attachment #456314 - Flags: review?(joshmoz)
Attachment #456314 - Flags: review?(joshmoz) → review+
Depends on: 557449
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?
(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+
The test covers it pretty much, I don't think we need anything else.
Flags: in-litmus? → in-litmus-
Target Milestone: --- → mozilla2.0b2
Blocks: 581031
Blocks: 581876
Depends on: 604190
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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.