54488 - [Mac] Non-draggable widgets in background windows should look disabled

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Matthew T (active 1999-2002)]

When a window is not the frontmost window of the frontmost app, all widgets in it 
should look disabled (as is done with native apps), to signify that simply 
clicking on the widget will not do anything to the widget (it will bring the 
window to the front instead).

The exception is for any widgets which can be dragged anywhere (e.g. the proxy 
icon, bookmarks etc); these should be left enabled to indicate that they can be 
dragged even when the window is not frontmost.

It *might* be worth really and truly disabling the widgets while they are in a 
background window, which would fix bug 6582 for free. However, depending on the 
way the code is architected, to do that you would probably need to keep some sort 
of list of which widgets were disabled and which enabled, so you could restore 
the correct states when the window became the frontmost again. And that might be 
more trouble than it was worth.

Comment 1

[Peter Trudelle]

This is arguably invalid while bug 6582 exists, since the buttons currently
correctly show clickability. Toolbars and toolbar buttons may also be draggable,
as are scrollbars now. I'm not sure that means they should be draggable while in
background, unless you could drag them to another window.

->future

Comment 2

[Matthew T (active 1999-2002)]

Background buttons do not currently correctly display clickability, if Simon's 
comment in bug 6582 that `the first click in a background window simply brings 
the window to the front' is currently correct for Mozilla.

As for dragability, I obviously wasn't precise enough ... For `any widgets which 
can be dragged anywhere', read `any widgets which can be dragged outside their 
own window'.

Comment 3

[Peter Trudelle]

The click is currently used by the widget, at least on Win32.

Comment 4

[Peter Trudelle]

->pinkerton, leaving future target.

Comment 5

[Boris Zbarsky [:bzbarsky]]

*** Bug 71724 has been marked as a duplicate of this bug. ***

Comment 6

[Henri Sivonen (:hsivonen)]

On Mac OS X, it is OK to support click-through for some widgets if it makes
sense and is non-destructive. This is documented on pp. 60-61 in the Macworld
draft of the Aqua HIG.
http://developer.apple.com/techpubs/macosx/SystemOverview/AquaHIGuidelines/

Comment 7

[Matthew T (active 1999-2002)]

That behavior on Mac OS X: (1) is an extension of behavior already found in Mac 
OS Classic (notably in Finder and Launcher icons), (2) in its extended form is 
(in my humble opinion) yet another silly design decison in Aqua, (3) is
*optional*, and (4) should really be in a separate bug from this one. (See also 
bug 51323.)

Comment 8

[Peter of the Norse]

It's not just for OS X apps. Any Mac APPL can do it if "Accept Background
Events" is on. Until this bug is fixed, it might be a good idea to turn it on. 

Comment 9

[Matthew T (active 1999-2002)]

Hyatt, I discussed this bug with Ben and he said you're the man. Apparently we 
need:
1.  an array of controls for each window, hashed by id, so we can quickly
    toggle them on/off without having to explore the entire DOM;
2.  a controller to be a required part of the binding for every window, so that
    relevant widgets can be enabled/disabled when the window gains/loses focus;
3.  a platform-specific way of telling which controls should be disabled when
    the window loses focus (including interrogating them to see whether they
    can be dragged outside their own window).

Comment 10

[Matthew T (active 1999-2002)]

Attachment: Screenshot showing enabled/disabled elements, including <groupbox> and <description>

Comment 11

[Matthew T (active 1999-2002)]

*** Bug 76214 has been marked as a duplicate of this bug. ***

Comment 12

[John Morrison]

*** Bug 124736 has been marked as a duplicate of this bug. ***

Comment 13

[Greg K.]

*** Bug 141478 has been marked as a duplicate of this bug. ***

Comment 14

[Jesse Ruderman]

See also bug 38646, "mousedown on something draggable shouldn't give focus to
mozilla".

Comment 15

[Jo Hermans]

*** Bug 217489 has been marked as a duplicate of this bug. ***

Comment 16

[Wayne Woods]

Should a separate report be filed for the Firefox version of this bug, or will
one fix take care of both? If there's a Firefox bug report, I haven't seen it yet.

Comment 17

[Matthew T (active 1999-2002)]

Wayne, that "XP Toolkit/Widgets" is in the "Browser" product is an accident of 
history. The component applies to Firefox just as much. No separate bug necessary.

Comment 18

[Mark Mentovai]

Attachment: Patch

The only native controls we currently use are scrollbars, and it's proper to activate/deactivate scrollbar appearance along with the windows that contain them.  kEventWindowActivated/kEventWindowDeactivated handlers are installed to address this, and the window's active state when the control is created is used to set the control's initial active state.

Comment 19

[Matthew T (active 1999-2002)]

That's a good start -- but when I reported this bug even Mozilla's scrollbars weren't native, so I wasn't referring just to non-existent native controls! XUL toolbar buttons, checkboxes, radiobuttons, option menus, and so on should similarly look unavailable when in background windows; and all those same controls (except, matching an OS X oddity, scrollbars) should actually *be* unavailable when in background windows. The exceptions are things that can be dragged from the background window, such as bookmarks and mail messages.

Comment 20

[Mark Mentovai]

Attachment: Patch v2 (checked in on trunk)

This is almost the same as v1, except it uses ::HiliteControl, which I'd ordinarily avoid, to better integrate with other callers to ::HiliteControl elsewhere in widget/src/mac.  The only functional difference from v1 is that this will avoid enabling a scrollbar that should be disabled because it's not scrollable - see nsNativeScrollbar::GetControlHiliteState().

Comment 21

[Mark Mentovai]

(In reply to comment #19)
> That's a good start -- but when I reported this bug even Mozilla's scrollbars
> weren't native, so I wasn't referring just to non-existent native controls!

OK, I'll just do the scrollbars for now.  Non-native widgets will need some more support (and will remain a problem in cocoa widgets).  Native form controls are still a problem too, because we're just slapping them up with DrawThemeButton (even in cocoa).  I'll leave this bug open for possible subsequent fixes after the scrollbars.

Comment 22

[Mark Mentovai]

Comment on attachment 223329 [details] [diff] [review]
Patch v2 (checked in on trunk)

a1.8.1? on me as a reminder

Comment 23

[José Jeria]

Could this have caused bug 339482?

Comment 24

[Mark Mentovai]

No, regression range is wrong.

Comment 25

[Mark Mentovai]

Attachment: 1.8 branch version (includes 339370 and 339482)

Checked in on MOZILLA_1_8_BRANCH.

Comment 26

[Phil Ringnalda (:philor)]

*** Bug 338482 has been marked as a duplicate of this bug. ***

Comment 27

[Scott MacGregor]

*** Bug 242678 has been marked as a duplicate of this bug. ***

Comment 28

[Colin Barrett [:cbarrett]]

I'd be interested in working on this bug. Do you mind if I take it?

Comment 29

[Mark Mentovai]

Be my guest.

Comment 30

[Colin Barrett [:cbarrett]]

bug taken. I'll work on this after I finish bug 370439.

Comment 32

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

Moving this to a relevant component.  This is now a regression for scrollbars (from bug 370439).

Comment 33

[Mike Schroepfer]

Colin - ping..

Comment 34

[Damon Sicore (:damons)]

This is being moved to b2 per Mac Status meeting target milestone triage.

Comment 35

[Josh Aas]

Attachment: fix v0.8

This is a start. It works well in most cases, but it doesn't work for the popup window case (e.g. the URL bar popup). Scrollbars always appear inactive in popups because the windows don't actually have cocoa key status. I'll have to figure out a different metric for determining inactive state.

What is the total list of controls that need to change their appearance when a window becomes inactive? I'll have to make this work for them too.

Comment 36

[Josh Aas]

Attachment: fix v0.9

This includes a couple changes that fix all of my known issues about scrollbars. This makes the enabled state work for popup windows and makes the active/inactive enabled state override all other states (like nothing-to-scroll).

Comment 37

[Josh Aas]

Comment on attachment 277597 [details] [diff] [review]
fix v0.9

I'm going to add a couple comments to explain the invalidate calls in nsChildView.mm.

Roc, does this approach look OK?

Comment 38

[Colin Barrett [:cbarrett]]

Looks good.

Anything that sets kThemeStateActive or kThemeTrackActive should use the kThemeStateInactive or kThemeTrackInactive constant when drawing in the background, I believe. Searching for those two constants should get you everything.

BTW, should we send out a general event or have some sort of CSS property for wether or not a window is active? It seems like other UI elements that imitate or want to act like native controls would find that useful. Perhaps as a spinoff bug?

Comment 39

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

> Roc, does this approach look OK?

Yeah. You might want to do some Tp profiling to ensure that frameIsInActiveWindow doesn't show up.

> BTW, should we send out a general event or have some sort of CSS property for
> wether or not a window is active?

A CSS property for this might be interesting, but I suggest you raise it with the WG --- it's not 1.9 material anyway.

Comment 40

[Colin Barrett [:cbarrett]]

I'd guess what you'd actually add want to be adding would be a psedudoclass. And I hadn't even thought about using it in content (my use cases were all chrome). That could get really interesting.

Comment 41

[Josh Aas]

Attachment: fix v0.9.5

This makes things work for popup menus, checkboxes, and radio buttons in addition to scrollbars. There are some stylistic and minor re-factoring changes that need to make it into the final version of this patch, and also this won't work for embedding.

Comment 42

[Colin Barrett [:cbarrett]]

DrawScale should also have this. See the Delay control in the General pane of Finder's preferences.

DrawTab and DrawTabPanel as well, see the Displays pane of System Preferences.

Those are just the ones I could find direct examples for. Anywhere we set kThemeStateActive or kThemeTrackActive a FooDrawInfo struct should be doing this. This is important for chrome too, not just content.

Comment 43

[Josh Aas]

Attachment: fix v1.0

There are two issues that this patch points out but doesn't cause. They should be filed and fixed separately.

1. Inactive native form controls can appear washed out (compare against Safari). This is more apparent with inactive controls I think because they are more transparent.
2. This patch works in Camino, but Camino doesn't always inform its embedding views that the window has lost key status. Thus, occasionally you'll see controls looking active that should be look inactive.

Comment 44

[Colin Barrett [:cbarrett]]

Comment on attachment 277940 [details] [diff] [review]
fix v1.0

There are two places I noticed that we have a comment about HIThemeSetFill not being available on 10.3 Should take care of that eventually (follow up bug?)

I dug in a little on the "washed out" look, and it's a limitation in Carbon -- check out TextWrangler, it does a similar sort of behavior. I otool'd and nm'd pretty thoroughly -- Cocoa uses [cell setControlTint:NSClearControlTint] to do its drawing, and I found no analog in Carbon.

If we want the "right" behavior, we'll need to use NSCell to draw certain elements -- checkboxes and radio buttons are the ones that will probably be the most noticable. Luckily, they are the easiest to draw with NSCell for us since they can really only be drawn at one size.

But that's all follow up bug material :) Other than Carbon being dumb, the patch looks good :)

Comment 45

[Colin Barrett [:cbarrett]]

(In reply to comment #44) 
> I dug in a little on the "washed out" look, and it's a limitation in Carbon --
> check out TextWrangler, it does a similar sort of behavior. I otool'd and nm'd
> pretty thoroughly -- Cocoa uses [cell setControlTint:NSClearControlTint] to do
> its drawing, and I found no analog in Carbon.

I should add that HITheme doesn't use or respect the "control tint" mechanism. _NSSetDefaultControlTint(NSClearControlTint) works fine in a test Cocoa app that just draws a checkbox, but doesn't affect HITheme drawing at all. Sad.

Comment 46

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

What about performance --- do you have any idea what this does to Tp?

Comment 47

[Josh Aas]

Since the invalidates are only called on activate/deactivate, I think the Tp impact is minimal. Just the extra code running to determine active status of the window.

Comment 48

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

Yeah, but that code runs every time we paint a form control, which is quite often.

Oh well, just check it in and watch Tp carefully I guess.

Comment 49

[Josh Aas]

landed on trunk

Comment 50

[Josh Aas]

I backed this out because I noticed that windows can get stuck in a state where they don't redraw their controls and they always appear inactive.

Comment 51

[Damon Sicore (:damons)]

Targeting M9 as this bug will block beta.

Comment 52

[Josh Aas]

Moving this to M10 due for resource reasons.

Comment 54

[Mike Schroepfer]

Given that this pre-dates 1.9 and hasn't gotten wrapped up moving to wanted list..

Comment 55

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

(In reply to comment #54)
> Given that this pre-dates 1.9 and hasn't gotten wrapped up moving to wanted
> list..

Scrollbars used to work properly in 1.8.1 and in 1.9 pre-scrollbar rewrite; they do not now. 

(Other widgets, however, were never fixed properly before, so they aren't a regression.)

Comment 56

[Josh Aas]

In summary, this is a regression for Cocoa widgets in 1.9 vs. 1.8.1. It is not a regression from Carbon in 1.8.1. In any case, this patch is still pretty much good to go for when we have the rest of our widgets cleaned up. I think the reason I originally backed it out is probably gone, now we're just waiting for new NSCell code to fix tinting.

Comment 57

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

(In reply to comment #56)
> In summary, this is a regression for Cocoa widgets in 1.9 vs. 1.8.1. It is not
> a regression from Carbon in 1.8.1.

No, it is a regression for Carbon widgets, too; see comment 20-comment 22 and comment 25.

Re-requesting blocking since it seems the rationale for denying blocking was an inadvertent view that "this never worked, not a regression."

Comment 58

[Josh Aas]

This is a regression and a bad one, I am strongly of the opinion that this needs to be fixed. I was wrong in comment #56.

Comment 59

[Mike Schroepfer]

(In reply to comment #58)
> This is a regression and a bad one, I am strongly of the opinion that this
> needs to be fixed. I was wrong in comment #56.
> 

K - if so needs to be higher than a p4..

Comment 60

[Josh Aas]

This is probably not going to make it for 1.9, tough call though. Sorry for the change of opinions again, but we just don't have the time.

Comment 61

[Markus Stange [:mstange]]

Attachment: josh's fix v1.0, updated to trunk

I've updated the patch to make it work with the current trunk code. The only difference to the original patch is in those parts of the code where we've switched to NSCell; it now looks like this:
  [mRadioButtonCell setEnabled:!inDisabled && frameIsInActiveWindow(aFrame)];

(In reply to comment #50)
> I backed this out because I noticed that windows can get stuck in a state
> where they don't redraw their controls and they always appear inactive.

This issue still persists, and I think I have found the reason. The call to Invalidate() is placed in viewsWindowDidBecomeKey. However, nsWindowMap.mm only calls viewsWindowDidBecomeKey when the view doesn't implement sendFocusEvent, see http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/cocoa/nsWindowMap.mm&rev=1.9#201
I don't know when that's the case; during my tests I haven't been able to hit
the breakpoint I placed in viewsWindowDidBecomeKey.

Comment 62

[Markus Stange [:mstange]]

Attachment: fix v1.1: move call to Invalidate into sendFocusEvent

I simply relocated mGeckoChild->Invalidate(PR_FALSE) to sendFocusEvent because that's always called when window focus changes. This patch works for me very well.

Comment 63

[Josh Aas]

There is another reason my patch was backed out, aside from stuck state. Some important form controls like selects don't have correct tinting in the background and it looks awful. I'll check it out again though and see if things have improved enough to go for it.

Comment 64

[Markus Stange [:mstange]]

Attachment: Screenshot of bugzilla search form in an inactive firefox window

Selects look good to me. Maybe the issue is gone.

Comment 65

[Markus Stange [:mstange]]

Attachment: Comparison of inactive selects in Safari and Firefox

I think I now know what you mean, but I don't think it looks awful.

Comment 66

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

If our choices here are blue scrollbars in background windows or slightly-washed-out widgets in background windows (ceteris paribus), the latter is less visually offensive, less confusing behaviorally, and more in harmony with Mac OS X.

That is, one is clearly wrong, and one is just slightly imperfect.

Comment 67

[Markus Stange [:mstange]]

Attachment: fix v1.2: Make PushButtons clear instead of disabled, don't draw focus rings for inactive widgets

In theory, we could avoid the washed-out look for all the widgets we draw using NSCell, using two different approaches:
 1. calling setControlTint:NSClearControlTint, as suggested by Colin in
    comment #44, or
 2. passing the right NSView to [buttonCell drawWithFrame:inView:], so that
    Cocoa can judge how to draw it (I think that's what Safari does).

However, neither approach worked for me.
So now we only do the inactive-but-not-disabled look for PushButtons using [mPushButtonCell setHighlighted:... && isInActiveWindow].

Comment 69

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

Josh, what's the status of your reviewing this (or is there someone else who can look at it)?

Comment 70

[Jose Fandos]

Could someone add "ue" to the keywords? It will make it easier to find.

Comment 72

[Josh Aas]

Comment on attachment 308973 [details] [diff] [review]
fix v1.2: Make PushButtons clear instead of disabled, don't draw focus rings for inactive widgets

Sorry about the review delay again, can you update this patch so it applies cleanly to current trunk?

Comment 73

[Markus Stange [:mstange]]

Attachment: fix v1.2, unbitrotted

Comment 74

[Josh Aas]

+  if (!aFrame) return YES;  

Please make this two lines, return on the second line. That is convention in Cocoa widgets, happens a couple of places.

Since your default value of YES is hard-coded in so many places in 'frameIsInActiveWindow', perhaps you should just make a "BOOL isActive = YES" and return that at the end. Just modify it with your logic "[nativeWindow isKeyWindow] || (windowType == eWindowType_popup)" before returning.

Otherwise this looks fine to me, please post an updated patch.

Might be weird to land this before we land active/inactive coloring for the unified toolbar.

Comment 75

[Markus Stange [:mstange]]

Thanks for the review, I'll post an updated patch as soon as bug 406730 lands, since it modifies nsNativeThemeCocoa.mm, too.

Comment 76

[Markus Stange [:mstange]]

Attachment: fix v1.3: address review comments

(In reply to comment #74)
> +  if (!aFrame) return YES;  
> 
> Please make this two lines, return on the second line. That is convention in
> Cocoa widgets, happens a couple of places.

Done.

> Since your default value of YES is hard-coded in so many places in
> 'frameIsInActiveWindow', perhaps you should just make a "BOOL isActive = YES"
> and return that at the end. Just modify it with your logic "[nativeWindow
> isKeyWindow] || (windowType == eWindowType_popup)" before returning.

But then I can't early-return and end up with nested if conditions, or am I missing something?

Comment 77

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

Comment on attachment 319024 [details] [diff] [review]
fix v1.3: address review comments

+static BOOL frameIsInActiveWindow(nsIFrame* aFrame)

I think this function is OK but should start with a capital letter.

Comment 79

[Markus Stange [:mstange]]

Comment on attachment 319024 [details] [diff] [review]
fix v1.3: address review comments

Requesting approval1.9:
Fixes a regression from Firefox 2 (scrollbars), very visible nativeness-issue, low risk.

Comment 80

[Mike Schroepfer]

Comment on attachment 319024 [details] [diff] [review]
fix v1.3: address review comments

a+ schrep

Comment 81

[Josh Aas]

landed

Comment 82

[Jesse Ruderman]

I think the fix here went too far.  In a recent Firefox build, form controls in background windows look disabled.  In the other Tiger apps I've tested, controls just lose their blueness.  The difference is easy to see with unchecked checkboxes and with the left part of <select>-like widgets.

Comment 83

[Markus Stange [:mstange]]

I would have liked to go for the clear look instead of the disabled look but failed - see comment 67. Furthermore, there are still some widgets we draw using Carbon; these don't support the clear style. Tiger's "native" Carbon applications also use the disabled look - see for example the iTunes and Finder preferences windows.

Comment 84

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

(In reply to comment #83)
> I would have liked to go for the clear look instead of the disabled look but
> failed - see comment 67. Furthermore, there are still some widgets we draw
> using Carbon; these don't support the clear style.

We should probably have a follow-up filed on comment 67, whether or not it's do-able now, if that's not already filed (I don't think I've seen it).

Comment 85

[Josh Aas]

Backed out due to Ts regression, we'll probably have to fix the problems and take this for 1.9.0.x.

Comment 86

[cris]

I would just to mention that this patch used to partially solve the problem of https://bugzilla.mozilla.org/show_bug.cgi?id=432131 

since this patch now is backed out, the other bug is quite obvious again.

Comment 87

[Markus Stange [:mstange]]

I tried to find the cause for the Ts regression but haven't succeeded yet. My attempts at profiling the patch didn't show any perceptible difference at all - Camino startup times stayed between 620 and 650ms, with and without patch.

However, since no widgets are drawn during startup, I suspect that the cause is the ->Invalidate call in sendFocusEvent. I found out that soundFocusEvent is called quite frequently (e.g. whenever you click into the content view or when tabbing through text fields), so we're probably redrawing too often.

Redraws are only necessary when the window's key state changes, so the invalidate call is probably best in nsWindowMap.mm's windowBecameKey / windowResignedKey. Placing it there also fixes bug 432131, so I've attached the patch there. It also fixes bug 432817 and bug 432866 (once this patch lands again).

I suggest landing the invalidation patch first before relanding this patch so we can see where the Ts regression comes from (if there is one).

Comment 88

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

Use Quartz Debug, turn on Autoflush Drawing, and then starting up Firefox you can see that we repaint the window several times, sometimes in weird states. You should be able to debug to find out where those redraws are coming from and why we paint in weird states sometimes.

Comment 89

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

(In reply to comment #87)
> I tried to find the cause for the Ts regression but haven't succeeded yet. My
> attempts at profiling the patch didn't show any perceptible difference at all -
> Camino startup times stayed between 620 and 650ms, with and without patch.

FWIW, half of the Firefox boxen saw no clear regression, either, and while cb-xserve01 showed the regression for Camino, (the much slower, and therefore typically more sensitive to perf changes) boxset did not.  It's certainly odd that only half of our machines were showing the problem; I don't know enough about the specs and build configurations of all the Firefox perf boxen to be able to speculate intelligently on whether those might be a factor in the half/half nature of the regression.

Comment 90

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

Am I wrong or is this bug about the Click-Trough feature, which is described here: 

http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_18_section_5.html#//apple_ref/doc/uid/20000961-TPXREF20

Comment 91

[Markus Stange [:mstange]]

Attachment: Comparison of disabled and transparent widgets in background windows

Click-through is bug 392188. But you're right, part of this bug is related to it: The question whether controls in background windows should look transparent or disabled. I've attached a screenshot.

We don't support click-through yet, so it makes sense to draw the widgets as disabled. My patch makes an exception for "push buttons" (they aren't drawn as disabled but transparent), which is kind of inconsequent... but black text on disabled buttons looks really weird.

Comment 97

[Markus Stange [:mstange]]

Attachment: fix v1.4: Update to trunk, reuse NativeWindowForFrame from bug 439354

This patch applies on top of that in bug 439354.

I'll fix radiobuttons, checkboxes and selects properly in bug 394892 and bug 399030.

Comment 99

[Markus Stange [:mstange]]

Attachment: fix v1.5: also draw as active in panel windows

Sorry for requesting yet another review on this patch, roc, but I noticed that the previous patch resulted in disabled-looking widgets in the "Customize Toolbar" panel.
The only part that has changed is this:

-static NSWindow* NativeWindowForFrame(nsIFrame* aFrame, int* aLevelsUp = NULL)
+static NSWindow* NativeWindowForFrame(nsIFrame* aFrame, int* aLevelsUp = NULL,
+                                      nsIWidget** aTopLevelWidget = NULL)
 {
   if (!aFrame)
     return nil;  
 
   nsIWidget* widget = aFrame->GetWindow();
   if (!widget)
     return nil;
 
   nsIWidget* topLevelWidget = widget->GetTopLevelWidget(aLevelsUp);
+  if (aTopLevelWidget)
+    *aTopLevelWidget = topLevelWidget;
 
   return (NSWindow*)topLevelWidget->GetNativeData(NS_NATIVE_WINDOW);
+}
+
+static BOOL FrameIsInActiveWindow(nsIFrame* aFrame)
+{
+  nsIWidget* topLevelWidget = NULL;
+  NSWindow* win = NativeWindowForFrame(aFrame, NULL, &topLevelWidget);
+  if (!topLevelWidget || !win)
+    return YES;
+
+  // XUL popups, e.g. the toolbar customization popup, can't become key windows,
+  // but controls in these windows should still get the active look.
+  nsWindowType windowType;
+  topLevelWidget->GetWindowType(windowType);
+  return [win isKeyWindow] || (windowType == eWindowType_popup);
 }

Comment 100

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/3a4db7f8bd83

Comment 101

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

Markus what about buttons? As I can see they are still not grayed-out with the latest trunk build. Take for example this Bugzilla page. All buttons e.g. Find, or Commit are not getting disabled when moving the focus to another window or application.

Comment 102

[Markus Stange [:mstange]]

They're not intended to get grayed-out. I want to use the clear look instead of the disabled look because that's what native Cocoa apps do.
Right now it looks a little odd because radio buttons, checkboxes and selects don't use the clear look, but the disabled look. I'll change that in bug 394892 and bug 399030.

In order to be really consistent, we should also enable click-through (bug 392188).

Comment 103

[mws]

On 10.5 when clicking on the help menu or clicking on stacks in the dock, the Firefox window remains in focus as expected but the window scrollbar and form widgets on the page are grayed until the help menu or stack is closed.

Comment 104

[Markus Stange [:mstange]]

I've noticed that, too - can you file a bug, mws, and mark it blocking this one? Thanks.

Comment 105

[mws]

(In reply to comment #104)
> I've noticed that, too - can you file a bug, mws, and mark it blocking this
> one? Thanks.

Ok, it's bug 458297.

Comment 106

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

Markus, there is another thing we behave differently as Safari. When Firefox loses the focus the location bar and all the search bars get a gray background. Will this issue be fixed by any of the bugs you are working on or should I create a new one?

Comment 107

[Markus Stange [:mstange]]

I haven't really decided about that, but I think we could do that in a "Overhaul background window appearance" bug after bug 392188 is fixed.

Comment 108

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

(In reply to comment #107)
> I haven't really decided about that, but I think we could do that in a
> "Overhaul background window appearance" bug after bug 392188 is fixed.

Finally filed that as bug 471082.

Comment 109

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

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090201020520