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

VERIFIED FIXED in mozilla1.9.1b1

Status

()

Core
Widget: Cocoa
P3
normal
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: Matthew Paul Thomas, Assigned: mstange)

Tracking

(Blocks: 1 bug, {polish, pp, regression})

Trunk
mozilla1.9.1b1
All
Mac OS X
polish, pp, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
wanted1.9.0.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not needed for 1.9])

Attachments

(7 attachments, 11 obsolete attachments)

90.50 KB, image/png
Details
12.50 KB, patch
Josh Aas
: review+
Details | Diff | Splinter Review
13.85 KB, patch
Mark Mentovai
: approval-branch-1.8.1+
Details | Diff | Splinter Review
74.71 KB, image/png
Details
4.54 KB, image/png
Details
219.24 KB, image/png
Details
28.47 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

17 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → Future
(Reporter)

Comment 2

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

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

Comment 4

17 years ago
->pinkerton, leaving future target.
Assignee: trudelle → pinkerton
Status: ASSIGNED → NEW
*** Bug 71724 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

16 years ago
Keywords: pp
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/
(Reporter)

Comment 7

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

16 years ago
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. 
(Reporter)

Comment 9

16 years ago
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).
(Reporter)

Comment 10

16 years ago
Created attachment 44734 [details]
Screenshot showing enabled/disabled elements, including <groupbox> and <description>
(Reporter)

Comment 11

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

Comment 12

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

Comment 13

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

Updated

15 years ago
Blocks: 73812

Updated

15 years ago
Keywords: mozilla1.2, polish

Updated

15 years ago
Keywords: mozilla1.2 → mozilla1.3

Updated

14 years ago
OS: Mac System 8.5 → MacOS X

Comment 14

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

Comment 15

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

Comment 16

13 years ago
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.
(Reporter)

Comment 17

13 years ago
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.

Updated

11 years ago
Assignee: mikepinkerton → mark
Component: XP Toolkit/Widgets → Widget: Mac

Comment 18

11 years ago
Created attachment 223227 [details] [diff] [review]
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.
Attachment #223227 - Flags: review?(joshmoz)

Updated

11 years ago
Attachment #223227 - Flags: review?(joshmoz) → review+
(Reporter)

Comment 19

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

11 years ago
Created attachment 223329 [details] [diff] [review]
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().
Attachment #223227 - Attachment is obsolete: true
Attachment #223329 - Flags: review?(joshmoz)

Comment 21

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

Updated

11 years ago
Attachment #223329 - Flags: review?(joshmoz) → review+

Comment 22

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

a1.8.1? on me as a reminder
Attachment #223329 - Attachment description: Patch v2 → Patch v2 (checked in on trunk)
Attachment #223329 - Flags: approval-branch-1.8.1?(mark)

Updated

11 years ago
Depends on: 339370

Comment 23

11 years ago
Could this have caused bug 339482?

Comment 24

11 years ago
No, regression range is wrong.

Updated

11 years ago
Depends on: 339482

Comment 25

11 years ago
Created attachment 223824 [details] [diff] [review]
1.8 branch version (includes 339370 and 339482)

Checked in on MOZILLA_1_8_BRANCH.
Attachment #223824 - Flags: approval-branch-1.8.1+

Updated

11 years ago
Attachment #223329 - Flags: approval-branch-1.8.1?(mark)

Updated

11 years ago
Depends on: 340027

Updated

11 years ago
Blocks: 262389
*** Bug 338482 has been marked as a duplicate of this bug. ***

Comment 27

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

Updated

11 years ago
Depends on: 353419
I'd be interested in working on this bug. Do you mind if I take it?

Comment 29

10 years ago
Be my guest.
bug taken. I'll work on this after I finish bug 370439.
Assignee: mark → cbarrett
Duplicate of this bug: 377227
Moving this to a relevant component.  This is now a regression for scrollbars (from bug 370439).
Component: Widget: Mac → Widget: Cocoa
Flags: blocking1.9?
Keywords: regression
QA Contact: jrgmorrison → cocoa

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Target Milestone: Future → mozilla1.9alpha6

Comment 33

10 years ago
Colin - ping..

Updated

10 years ago
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta2
This is being moved to b2 per Mac Status meeting target milestone triage.
Assignee: cbarrett → joshmoz

Comment 35

10 years ago
Created attachment 277577 [details] [diff] [review]
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

10 years ago
Created attachment 277597 [details] [diff] [review]
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).
Attachment #277577 - Attachment is obsolete: true

Comment 37

10 years ago
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?
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?
> 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.
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

10 years ago
Created attachment 277629 [details] [diff] [review]
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.
Attachment #277597 - Attachment is obsolete: true
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

10 years ago
Created attachment 277940 [details] [diff] [review]
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.
Attachment #277629 - Attachment is obsolete: true
Attachment #277940 - Flags: review?(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 :)
Attachment #277940 - Flags: review?(cbarrett) → review+
(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.
Attachment #277940 - Flags: superreview?(roc)
What about performance --- do you have any idea what this does to Tp?

Comment 47

10 years ago
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.
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.
Attachment #277940 - Flags: superreview?(roc) → superreview+

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 49

10 years ago
landed on trunk

Comment 50

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 394092

Updated

10 years ago
Target Milestone: mozilla1.9 M9 → ---
Targeting M9 as this bug will block beta.
Target Milestone: --- → mozilla1.9 M9

Comment 52

10 years ago
Moving this to M10 due for resource reasons.
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Duplicate of this bug: 394092
Depends on: 399030

Updated

10 years ago
No longer depends on: 399030
Depends on: 394892

Updated

10 years ago
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11

Updated

10 years ago
Priority: P3 → P4
Target Milestone: mozilla1.9 M11 → ---

Comment 54

10 years ago
Given that this pre-dates 1.9 and hasn't gotten wrapped up moving to wanted list..
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
(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

10 years ago
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.
(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."
Flags: blocking1.9- → blocking1.9?

Comment 58

10 years ago
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.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9+

Comment 59

10 years ago
(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..

Updated

10 years ago
Priority: P4 → P3

Updated

10 years ago
Depends on: 399030

Comment 60

9 years ago
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.
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
(Assignee)

Comment 61

9 years ago
Created attachment 307554 [details] [diff] [review]
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.
Attachment #277940 - Attachment is obsolete: true
(Assignee)

Comment 62

9 years ago
Created attachment 307573 [details] [diff] [review]
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.
Attachment #307554 - Attachment is obsolete: true
Attachment #307573 - Flags: review?(joshmoz)

Comment 63

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

Comment 64

9 years ago
Created attachment 307844 [details]
Screenshot of bugzilla search form in an inactive firefox window

Selects look good to me. Maybe the issue is gone.
(Assignee)

Comment 65

9 years ago
Created attachment 307957 [details]
Comparison of inactive selects in Safari and Firefox

I think I now know what you mean, but I don't think it looks awful.
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.
(Assignee)

Comment 67

9 years ago
Created attachment 308973 [details] [diff] [review]
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].
Attachment #307573 - Attachment is obsolete: true
Attachment #308973 - Flags: review?(joshmoz)
Attachment #307573 - Flags: review?(joshmoz)

Updated

9 years ago
Duplicate of this bug: 424147
Josh, what's the status of your reviewing this (or is there someone else who can look at it)?

Comment 70

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

Updated

9 years ago
Duplicate of this bug: 427678

Comment 72

9 years ago
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?
Attachment #308973 - Flags: review?(joshmoz)
(Assignee)

Comment 73

9 years ago
Created attachment 314414 [details] [diff] [review]
fix v1.2, unbitrotted
Attachment #308973 - Attachment is obsolete: true
Attachment #314414 - Flags: review?(joshmoz)

Comment 74

9 years ago
+  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.

(Assignee)

Comment 75

9 years ago
Thanks for the review, I'll post an updated patch as soon as bug 406730 lands, since it modifies nsNativeThemeCocoa.mm, too.
Status: REOPENED → ASSIGNED
(Assignee)

Updated

9 years ago
Assignee: joshmoz → mstange
Status: ASSIGNED → NEW
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 76

9 years ago
Created attachment 319024 [details] [diff] [review]
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?
Attachment #314414 - Attachment is obsolete: true
Attachment #319024 - Flags: superreview?(roc)
Attachment #319024 - Flags: review?(joshmoz)
Attachment #314414 - Flags: review?(joshmoz)
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.
Attachment #319024 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 432023

Updated

9 years ago
Attachment #319024 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 79

9 years ago
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.
Attachment #319024 - Flags: approval1.9?

Comment 80

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

a+ schrep
Attachment #319024 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed

Comment 81

9 years ago
landed
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED

Comment 82

9 years ago
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.
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
(Assignee)

Comment 83

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

Updated

9 years ago
Depends on: 432817
Depends on: 432950
(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

9 years ago
Backed out due to Ts regression, we'll probably have to fix the problems and take this for 1.9.0.x.
Status: RESOLVED → REOPENED
Flags: wanted1.9.0.x+
Resolution: FIXED → ---
Blocks: 432182
Depends on: 432866
Whiteboard: [missed 1.9 checkin]

Updated

9 years ago
Whiteboard: [missed 1.9 checkin] → [not needed for 1.9]

Comment 86

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

Comment 87

9 years ago
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).
Status: REOPENED → ASSIGNED
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.
(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.
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
Hardware: Macintosh → All
Target Milestone: mozilla1.9 → ---
(Assignee)

Comment 91

9 years ago
Created attachment 324206 [details]
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.

Updated

9 years ago
Duplicate of this bug: 439104

Updated

9 years ago
Duplicate of this bug: 439353
Duplicate of this bug: 442184
Duplicate of this bug: 443641

Updated

9 years ago
Duplicate of this bug: 443681
(Assignee)

Comment 97

9 years ago
Created attachment 333269 [details] [diff] [review]
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.
Attachment #319024 - Attachment is obsolete: true
Attachment #333269 - Flags: superreview?(roc)
Attachment #333269 - Flags: superreview?(roc) → superreview+
Duplicate of this bug: 455566
(Assignee)

Comment 99

9 years ago
Created attachment 339651 [details] [diff] [review]
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);
 }
Attachment #333269 - Attachment is obsolete: true
Attachment #339651 - Flags: superreview?(roc)
Attachment #339651 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 100

9 years ago
pushed: http://hg.mozilla.org/mozilla-central/rev/3a4db7f8bd83
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
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.
(Assignee)

Comment 102

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

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

Comment 104

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

Updated

9 years ago
Depends on: 458297

Comment 105

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

Comment 107

9 years ago
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.
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
(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.
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

8 years ago
Duplicate of this bug: 492926

Updated

7 years ago
Duplicate of this bug: 568301
You need to log in before you can comment on or make changes to this bug.