Closed
Bug 54488
Opened 24 years ago
Closed 16 years ago
[Mac] Non-draggable widgets in background windows should look disabled
Categories
(Core :: Widget: Cocoa, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b1
People
(Reporter: mpt, Assigned: mstange)
References
(Blocks 1 open bug)
Details
(Keywords: platform-parity, polish, regression, Whiteboard: [not needed for 1.9])
Attachments
(7 files, 11 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mark
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
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•24 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•24 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•24 years ago
|
||
The click is currently used by the widget, at least on Win32.
Comment 4•24 years ago
|
||
->pinkerton, leaving future target.
Assignee: trudelle → pinkerton
Status: ASSIGNED → NEW
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•24 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•23 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•23 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•23 years ago
|
||
Reporter | ||
Comment 11•23 years ago
|
||
*** Bug 76214 has been marked as a duplicate of this bug. ***
Comment 12•23 years ago
|
||
*** Bug 124736 has been marked as a duplicate of this bug. ***
Comment 13•23 years ago
|
||
*** Bug 141478 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Keywords: mozilla1.2,
polish
Keywords: mozilla1.2 → mozilla1.3
Comment 14•21 years ago
|
||
See also bug 38646, "mousedown on something draggable shouldn't give focus to
mozilla".
Comment 15•21 years ago
|
||
*** Bug 217489 has been marked as a duplicate of this bug. ***
Comment 16•21 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•21 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•18 years ago
|
Assignee: mikepinkerton → mark
Component: XP Toolkit/Widgets → Widget: Mac
Comment 18•18 years ago
|
||
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)
Attachment #223227 -
Flags: review?(joshmoz) → review+
Reporter | ||
Comment 19•18 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•18 years ago
|
||
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•18 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.
Attachment #223329 -
Flags: review?(joshmoz) → review+
Comment 22•18 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)
Comment 23•18 years ago
|
||
Could this have caused bug 339482?
Comment 24•18 years ago
|
||
No, regression range is wrong.
Comment 25•18 years ago
|
||
Checked in on MOZILLA_1_8_BRANCH.
Attachment #223824 -
Flags: approval-branch-1.8.1+
Updated•18 years ago
|
Attachment #223329 -
Flags: approval-branch-1.8.1?(mark)
Comment 26•18 years ago
|
||
*** Bug 338482 has been marked as a duplicate of this bug. ***
Comment 27•18 years ago
|
||
*** Bug 242678 has been marked as a duplicate of this bug. ***
Comment 28•18 years ago
|
||
I'd be interested in working on this bug. Do you mind if I take it?
Comment 29•18 years ago
|
||
Be my guest.
Comment 30•18 years ago
|
||
bug taken. I'll work on this after I finish bug 370439.
Assignee: mark → cbarrett
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
Comment 33•17 years ago
|
||
Colin - ping..
Comment 34•17 years ago
|
||
This is being moved to b2 per Mac Status meeting target milestone triage.
Updated•17 years ago
|
Assignee: cbarrett → joshmoz
Comment 35•17 years ago
|
||
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•17 years ago
|
||
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•17 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?
Comment 38•17 years ago
|
||
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.
Comment 40•17 years ago
|
||
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•17 years ago
|
||
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
Comment 42•17 years ago
|
||
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•17 years ago
|
||
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 44•17 years ago
|
||
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+
Comment 45•17 years ago
|
||
(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.
Updated•17 years ago
|
Attachment #277940 -
Flags: superreview?(roc)
What about performance --- do you have any idea what this does to Tp?
Comment 47•17 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+
Comment 49•17 years ago
|
||
landed on trunk
Comment 50•17 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 → ---
Updated•17 years ago
|
Target Milestone: mozilla1.9 M9 → ---
Comment 51•17 years ago
|
||
Targeting M9 as this bug will block beta.
Target Milestone: --- → mozilla1.9 M9
Comment 52•17 years ago
|
||
Moving this to M10 due for resource reasons.
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Comment 54•17 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•17 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•17 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•17 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..
Comment 60•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 years ago
|
||
Selects look good to me. Maybe the issue is gone.
Assignee | ||
Comment 65•17 years ago
|
||
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•17 years ago
|
||
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)
Josh, what's the status of your reviewing this (or is there someone else who can look at it)?
Comment 70•17 years ago
|
||
Could someone add "ue" to the keywords? It will make it easier to find.
Comment 72•17 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•17 years ago
|
||
Attachment #308973 -
Attachment is obsolete: true
Attachment #314414 -
Flags: review?(joshmoz)
Comment 74•17 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•17 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•17 years ago
|
Assignee: joshmoz → mstange
Status: ASSIGNED → NEW
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 76•17 years ago
|
||
(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+
Attachment #319024 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 79•17 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•17 years ago
|
||
Comment on attachment 319024 [details] [diff] [review]
fix v1.3: address review comments
a+ schrep
Attachment #319024 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 81•17 years ago
|
||
landed
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 82•17 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.
Updated•17 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
Assignee | ||
Comment 83•17 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.
(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•17 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
Updated•17 years ago
|
Whiteboard: [missed 1.9 checkin]
Updated•17 years ago
|
Whiteboard: [missed 1.9 checkin] → [not needed for 1.9]
Comment 86•17 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•16 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.
Comment 90•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 97•16 years ago
|
||
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+
Assignee | ||
Comment 99•16 years ago
|
||
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•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Comment 101•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
I've noticed that, too - can you file a bug, mws, and mark it blocking this one? Thanks.
Comment 105•16 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.
Comment 106•16 years ago
|
||
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•16 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.
Updated•16 years ago
|
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
Comment 108•16 years ago
|
||
(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•16 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•