406730 - Correctly style unfocused window on OS X

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

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

We need to change the title bar color, unified gradient, and sidebar color.  Should be possible due to bug 404096.  Bug 397073 allows us to tell the difference between 10.4 and 10.5.

Comment 1

[Mike Connor [:mconnor]]

That bug doesn't help us, it helps people not using titlebarcolor.  We need a way to set a different color when the app is not focused, pretty sure cbarrett was looking into this already.

Comment 5

[Chris Walter]

Is this the same bug as 417175? 

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #5)
> Is this the same bug as 417175? 
> 

yes

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: changes titlebarcolor and background image of nav-bar on focus and blur

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 303108 [details] [diff] [review]
changes titlebarcolor and background image of nav-bar on focus and blur

Josh: we can haz review?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Or Dao?

Comment 11

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

Comment on attachment 303108 [details] [diff] [review]
changes titlebarcolor and background image of nav-bar on focus and blur

Mano already r-ed this in the other bug. The same comments still apply.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

according to mconnor in response to mano:

I think we're in the process of uncrippling themes, we made an explicit choice
to stop trying to preserve some sort of "safe" theme framework a while back,
but I don't know if we've explicitly declared as such, but this patch should be
acceptable now.

Comment 13

[Asaf Romano (gone)]

According to myself: it's still possible to simply move this binding over to browser/ and don't do dirty hacks like this, <handler>s still don't in themes, whenever they do, onevent attributes would be acceptable too.

Comment 14

[Asaf Romano (gone)]

And in that handler, we should set an attribute or class, and per that set the image in the css file. You shouldn't change style rules from js code if you don't have to.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

I tried setting main-window:focus and nav-bar:focus in browser.css, neither worked.  I would agree that it would be much cleaner, any suggestions on how to make that work?

Comment 16

[Asaf Romano (gone)]

Set an attribute on document.documentElement and then use an attribute selector, e.g. document.documentElement.setAttibribute("focused", "true")/removeAttribute("focused"); in the binding, and window[focused] { .. } block in the css file.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

I am unassigning this bug because XBL is not my fortay and I cannot resolve mano's objections.

Comment 19

[Harish Inamdar]

Works for me. 

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: this uses attributes and selectors, but still sets the attributes from onblur and onfocus

hopefully someone who knows xbl better can move this js code to handlers

Comment 21

[Dão Gottwald [:dao]]

|if(toolbar.setAttribute)|? That's one reason to avoid |for each| for arrays and node lists.
I think this approach isn't good anyway. We can set the attribute on the root element and use descendant selectors, right?

Comment 22

[Dão Gottwald [:dao]]

As per comment 13 and comment 14, this shouldn't be fixed only within the theme.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao: at this point I think we should entertain hacks for the default theme if they get the job done, and if moving the binding over is more complex. If not, then by all means, let's do it that way.

From a usability perspective, this *does* have an effect. Knowing which window has focus matters for a lot of operations.

Comment 24

[Asaf Romano (gone)]

Beltzner: when did anyone say it's more complex?

Comment 25

[Mike Connor [:mconnor]]

-> Mano, since he seems to have an idea of how this should work. :)

Comment 26

[Markus Stange [:mstange]]

Attachment: Set focused attribute only on window, use descendant selectors

This patch addresses some of the shortcomings of Brad's patch. The binding is still located in /toolkit/themes/pinstripe.

I've got some questions.
 1. When I focus the content view of the window (by clicking into it) after
    having focused a tab(button), the window's onblur handler fires. Clicking
    on the active tab fires the window's onfocus handler. Is that the intended
    behavior of these handlers?
    It makes the patch quite useless.
 2. In what file in /browser/base/content/ should the binding be located?
 3. How can we avoid duplicating the code for window, dialog, wizard and
    prefwindow? The inheritance tree doesn't seem to like us.
 4. What about themes that would like to use the unified style with
    different colors? The titlebarcolors are hardcoded into these bindings.

And FWIW, this is the way I think the problem would ideally be solved:
 - Transform the "titlebarcolor" attribute into a -moz-titlebar-color CSS
   property and
 - make :focus work for windows, so that we don't need any bindings.
But I don't think that's going to make it.

Comment 29

[Markus Stange [:mstange]]

I've given up on this bug. Even using <handler>s, I couldn't reliably test whether a window is focused or not. Sometimes the focus / blur events don't fire until the window has been clicked, sometimes blur doesn't fire at all, sometimes a blur fires after a focus while the window is still focused...

Maybe at some point in the future we can introduce a :-moz-in-active-window pseudoclass and reuse some of the code of bug 54488.

Comment 31

[Markus Stange [:mstange]]

Attachment: Fix v4.0: Move the binding to toolkit/content/widgets

Let's try again. This patch mostly works.

I still don't know how to share the code for window, dialog, prefwindow and wizard. This patch only contains the functionality for <window>s.
(The only difference between the bindings would be the <binding extends="..."> attribute.)

Comment 32

[Mike Connor [:mconnor]]

We have updates from Stephen for images.

Comment 33

[Markus Stange [:mstange]]

Attachment: Fix v4.1: Introduce DOMWindowActivate/-Deactivate event

Using "focus" and "blur" events didn't work reliably enough, see comment 49 for examples. So I added "DOMWindowActivate" and "DOMWindowDeactivate" as DOM events corresponding to the already existing Gecko events NS_ACTIVATE and NS_DEACTIVATE.

For unknown reasons, sometimes these events aren't dispatched by the <window>'s <handler>s, but only by the <window>'s window object. That's why I chose window.addEventListener over using handlers.

In an effort to share the focusing code between <window>, <prefwindow>, <dialog> and <wizard>, I extracted it into a JavaScript file (toolkit/content/focusHandling.js), but that hasn't made it much shorter...

This patch does not contain changes to the theme's CSS, I suppose that's Kevin Gerich's part. For example, for the main window's navbar, the CSS would look like this:

#nav-bar {
  /* appearance of inactive window */
}
window[focused] #nav-bar{
  /* differences for active window */
}

The titlebarcolors can be set in themes/pinstripe/global/globalBindings.xml using the attributes "activetitlebarcolor" and "inactivetitlebarcolor".

Comment 34

[Markus Stange [:mstange]]

(In reply to comment #33)
> see comment 49 for examples.

That's comment 29, of course:

> Sometimes the focus / blur events don't
> fire until the window has been clicked, sometimes blur doesn't fire at all,
> sometimes a blur fires after a focus while the window is still focused...

Comment 35

[Stefan [:stefanh]]

+# The Original Code is mozilla.org code.
+#
+# The Initial Developer of the Original Code is
+# Netscape Communications Corporation.
+# Portions created by the Initial Developer are Copyright (C) 2008
+# the Initial Developer. All Rights Reserved.
+#
+# Contributor(s):
+#   Markus Stange <[e-mail]>
+#

If it's your code, you're the Initial Developer and the copyright is also yours and then you can leave the contributors section blank. I doubt Netscape wrote this... :-)

Comment 36

[Neil Deakin]

(In reply to comment #33)
> Created an attachment (id=313348) [details]
> Fix v4.1: Introduce DOMWindowActivate/-Deactivate event

Why isn't this just a matter of just switching the colour in nsCocoaWindow.mm? Why are any toolkit changes required? The widget should know if it is focused or not, and we just should flip the titlebar colour there.

There doesn't appear to be any code in this patch which fires the activate/deactive events. That said, I don't like the addition of these events. The distinction between activate and focus is unclear and I think is mostly there due to a Windowsism. The code for focus/activate is a huge mess and I don't think we want to expose additional events which further propagate the mess.

Also, do not use the subscript loader in toolkit code. It doesn't cache scripts.

Comment 37

[Markus Stange [:mstange]]

Thanks for the review.

(In reply to comment #36)
> (In reply to comment #33)
> > Created an attachment (id=313348) [details] [details]
> > Fix v4.1: Introduce DOMWindowActivate/-Deactivate event
> 
> Why isn't this just a matter of just switching the colour in nsCocoaWindow.mm?
> Why are any toolkit changes required? The widget should know if it is focused
> or not, and we just should flip the titlebar colour there.

That would work for changing the titlebarcolor, but not for the rest of the window. The main purpose of the toolkit changes is to set a "focused" attribute on the <window>, so that the theme's CSS can style toolbars, sidebar and statusbar correctly. These parts aren't drawn by widget code.

I don't like to mess with toolkit either, but I don't know a better way to tackle the problem. If you know how to do it, please tell.

> There doesn't appear to be any code in this patch which fires the
> activate/deactive events. 

The NS_ACTIVATE and NS_DEACTIVATE events are already fired by existing code, for example in [nsChildView viewsWindowDidBecomeKey]. Up to now, they just weren't converted into DOM events.

> That said, I don't like the addition of these events.

I don't like it either, but as I said, I don't know a better way to achieve the desired effect.

> The distinction between activate and focus is unclear and I think is mostly
> there due to a Windowsism. The code for focus/activate is a huge mess and I
> don't think we want to expose additional events which further propagate the
> mess.

Agreed.

> Also, do not use the subscript loader in toolkit code. It doesn't cache
> scripts.

OK, but what should I use instead?
The problem is that the focus/activation handling code is the same for window, dialog, prefwindow and wizard. Is there another way of sharing the code between them? As I see it, inheritance doesn't help here.

Comment 38

[Markus Stange [:mstange]]

(In reply to comment #37)
> > There doesn't appear to be any code in this patch which fires the
> > activate/deactive events. 
> 
> The NS_ACTIVATE and NS_DEACTIVATE events are already fired by existing code,
> for example in [nsChildView viewsWindowDidBecomeKey]. Up to now, they just
> weren't converted into DOM events.

And I can confirm that these DOM events are fired; the window changes its color as intended.

Comment 39

[Shawn Wilsher :sdwilsh]

Just to clarify - this bug is only for browser chrome, and not widgets in content, correct?

Comment 40

[Dave Miller [:justdave]]

Hmm, both problems are happening, actually.  But I would assume they're probably separate bugs, but perhaps not.  The most noticeable is the select boxes, which have the blue are where the arrow is on the right... the blue color is supposed to go away when the window is in the background, and it doesn't.  Unless the two issues are closely related that might be better for an additional bug.

Comment 41

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

This bug is just chrome. Widgets in content are bug 54488 (which is actually a regression on trunk according to comment 58).

Comment 43

[Neil Deakin]

> I don't like it either, but as I said, I don't know a better way to achieve the
> desired effect.
> 

I think it would be better to just set the attribute when the NS_ACTIVATE/NS_DEACTIVATE events get received in either GlobalWindow, the nsEventStateManager, or somewhere like that. smaug should know a suitable place. That way, you wouldn't need to add extra events or any of the toolkit hacks.

Comment 44

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

(In reply to comment #33)
> For unknown reasons, sometimes these events aren't dispatched by the <window>'s
> <handler>s, but only by the <window>'s window object.
|window| is global object, <window> is the document.documentElement.
So different objects. <handler>s are registered to an element.(In reply to comment #43)

> I think it would be better to just set the attribute when the
> NS_ACTIVATE/NS_DEACTIVATE events get received in either GlobalWindow, the
> nsEventStateManager, or somewhere like that.
GlobalWindow already uses those events in PostHandleEvent
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&rev=1.1009&mark=2218-2231#2207
That could be perhaps extended so that if mDocument is a XUL document,
some attribute is set to the root element when window is active, or something like that. 

Comment 45

[Markus Stange [:mstange]]

Attachment: Fix v5.0: nsGlobalWindow sets the focused attribute, nsCocoaWindow decides what titlebarcolor to use

Like this?

Comment 46

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

Comment on attachment 314680 [details] [diff] [review]
Fix v5.0: nsGlobalWindow sets the focused attribute, nsCocoaWindow decides what titlebarcolor to use

Using -p with cvs diff would be great,
>   if (outer && outer->mFullScreen) {
>     if (aVisitor.mEvent->message == NS_DEACTIVATE ||
>         aVisitor.mEvent->message == NS_ACTIVATE) {
>       nsCOMPtr<nsIFullScreen> fullScreen =
>-        do_GetService("@mozilla.org/browser/fullscreen;1");
>+      do_GetService("@mozilla.org/browser/fullscreen;1");
Why you remove those 2 spaces. Put them back.


>+      
>+  // Set / unset the "focused" attribute on the documentElement
>+  nsCOMPtr<nsIContent> rootElem = mDoc->GetRootContent();
>+  if (aVisitor.mEvent->message == NS_DEACTIVATE)
>+    rootElem->UnsetAttr(kNameSpaceID_None, nsGkAtoms::focused, PR_TRUE);
>+  else
>+    rootElem->SetAttr(kNameSpaceID_None, nsGkAtoms::focused,
>+                      NS_LITERAL_STRING("true"), PR_TRUE);

You could change code a bit so that there would be only one
if (aVisitor.mEvent->message == NS_DEACTIVATE ||
    aVisitor.mEvent->message == NS_ACTIVATE)
and then inside that block do whatever is needed.
Also, I think you should check that mDoc is really a XUL document.
And I'm not sure if "focused" is the right name for the attribute.
perhaps "active" or "activated" or "activatedWindow" or "focusedWindow"?

>-    <content titlebarcolor="#C9C9C9"/>
>+    <content activetitlebarcolor="#C9C9C9" inactivetitlebarcolor="#E4E4E4"/>
I have no idea about these colors. Josh have to review this part.

>+++ widget/public/nsIWidget.h	9 Apr 2008 20:24:59 -0000
...
>-    NS_IMETHOD SetWindowTitlebarColor(nscolor aColor) = 0;
>+    NS_IMETHOD SetWindowTitlebarColor(nscolor aColor, PRBool aActive) = 0;

You're changing the interface, please update the IID.


>Index: widget/src/cocoa/nsCocoaWindow.h
Can't really review this.

Comment 47

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

Is this bug really 10.5 only like the summary describes?

Comment 48

[Markus Stange [:mstange]]

(In reply to comment #47)
> Is this bug really 10.5 only like the summary describes?

No, it's not. (Otherwise I wouldn't be able to test it ;)
And I don't think there is a reason to make it 10.5 only.

Comment 49

[Neil Deakin]

> and then inside that block do whatever is needed.
> Also, I think you should check that mDoc is really a XUL document.

Yes, you should only do this for XUL documents, and you should probably check for those that are nsIDOMChromeWindows as well.

> And I'm not sure if "focused" is the right name for the attribute.
> perhaps "active" or "activated" or "activatedWindow" or "focusedWindow"?

I think active="true" would be best here.

Comment 50

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

(In reply to comment #48)
> No, it's not. (Otherwise I wouldn't be able to test it ;)
> And I don't think there is a reason to make it 10.5 only.

Thanks. Updating summary accordingly.

Comment 51

[Markus Stange [:mstange]]

Attachment: Fix v5.1: better

(In reply to comment #46)
> (From update of attachment 314680 [details] [diff] [review])
> Using -p with cvs diff would be great,

Uh. I'd never have realized.

> Why you remove those 2 spaces. Put them back.

Oops, not intended.

> You could change code a bit so that there would be only one
> if (aVisitor.mEvent->message == NS_DEACTIVATE ||
>     aVisitor.mEvent->message == NS_ACTIVATE)
> and then inside that block do whatever is needed.

I think what I did there was rubbish anyway; when shuffling the code I seem to have lost such a check.

> Also, I think you should check that mDoc is really a XUL document.

Done.

> >-    <content titlebarcolor="#C9C9C9"/>
> >+    <content activetitlebarcolor="#C9C9C9" inactivetitlebarcolor="#E4E4E4"/>
> I have no idea about these colors. Josh have to review this part.

I hope Kevin Gerich will customize these colors in the next version of the theme. Right now they're only placeholders.

> >+++ widget/public/nsIWidget.h	9 Apr 2008 20:24:59 -0000
> ...
> >-    NS_IMETHOD SetWindowTitlebarColor(nscolor aColor) = 0;
> >+    NS_IMETHOD SetWindowTitlebarColor(nscolor aColor, PRBool aActive) = 0;
> 
> You're changing the interface, please update the IID.

Done.

(In reply to comment #49)
> Yes, you should only do this for XUL documents, and you should probably check
> for those that are nsIDOMChromeWindows as well.

How do I do that?
nsCOMPtr<nsIDOMChromeWindows> chromeWindow(do_QueryInterface(this));
resulted in "error: 'nsISupports' is an ambiguous base of 'nsGlobalWindow'".

> 
> > And I'm not sure if "focused" is the right name for the attribute.
> > perhaps "active" or "activated" or "activatedWindow" or "focusedWindow"?
> 
> I think active="true" would be best here.

Done.

(And where is the "additional review" box?)

Comment 52

[Markus Stange [:mstange]]

Comment on attachment 314733 [details] [diff] [review]
Fix v5.1: better

> (And where is the "additional review" box?)
There it is. Strange.

Comment 53

[Neil Deakin]

> > Yes, you should only do this for XUL documents, and you should probably check
> > for those that are nsIDOMChromeWindows as well.
> 
> How do I do that?

You need to static_cast this first, like http://lxr.mozilla.org/mozilla/source/dom/src/base/nsGlobalWindow.cpp#8190

I'm not sure how important it is but we reason for doing this is make sure that the window is a top level window and not a frame.

Comment 54

[Markus Stange [:mstange]]

Attachment: Fix v5.2: Check for nsIDOMChromeWindow as well

Comment 55

[Håkan Waara]

Comment on attachment 314854 [details] [diff] [review]
Fix v5.2: Check for nsIDOMChromeWindow as well


>+    nsCOMPtr<nsIDOMChromeWindow> chromeWin =
>+      do_QueryInterface(static_cast<nsIDOMWindow *>(this));
>+    if (doc && chromeWin) {

This cast is wrong and unnecessary AFAIK. 

The QueryInterface call is right, but you don't need to cast |this|, since what you pass to QI is just anything that supports nsISupports, which |this| already does (and if it didn't, you'd crash here I think).

Comment 56

[Neil Deakin]

(In reply to comment #55)
nnecessary AFAIK. 
> 
> The QueryInterface call is right, but you don't need to cast |this|, since what
> you pass to QI is just anything that supports nsISupports, which |this| already
> does (and if it didn't, you'd crash here I think).
> 

A cast is needed because 'this' inherits from nsISupports more than once due to multiple inheritance.

Comment 57

[Neil Deakin]

Comment on attachment 314854 [details] [diff] [review]
Fix v5.2: Check for nsIDOMChromeWindow as well

Index: dom/src/base/nsGlobalWindow.cpp
+      
+      // We need to ask the theme if the window is active.
+      // NS_DEACTIVATE doesn't always mean that the window loses "main" state

When would this situation occur?

+      
+      nsIDocShell *docShell = GetDocShell();
+      if (docShell) {

You get the docshell and compare to null multiple times. Just get it once and rely on the NS_ENSURE_TRUE to null check it.

+  virtual PRBool WindowIsActive(nsIWidget* aWindow) { return PR_FALSE; }

Should this be true by default for other platforms? Or is the idea that 'active' would never be set for these?
Or would it be better to rely on the activate/deactivate events for those platforms?

+PRBool 
+nsNativeThemeCocoa::WindowIsActive(nsIWidget* aWindow)
+{
+  PRBool isActive = PR_TRUE;
+  NSWindow* nativeWindow = (NSWindow*)aWindow->GetNativeData(NS_NATIVE_WINDOW);
+
+  if (nativeWindow) {
+    nsWindowType windowType;
+    aWindow->GetWindowType(windowType);
+    isActive = [nativeWindow isMainWindow] || (windowType == eWindowType_popup);
+  }

Is there something special that requires popups to be specified here? Popups are not actually ever activated.

Comment 58

[Håkan Waara]

(In reply to comment #56)
> (In reply to comment #55)
> nnecessary AFAIK. 
> > 
> > The QueryInterface call is right, but you don't need to cast |this|, since what
> > you pass to QI is just anything that supports nsISupports, which |this| already
> > does (and if it didn't, you'd crash here I think).
> > 
> 
> A cast is needed because 'this' inherits from nsISupports more than once due to
> multiple inheritance.
> 

Is that what NS_INTERFACE_MAP_ENTRY_AMBIGUOUS is for? For examples, see elsewhere in our tree.

Comment 59

[Neil Deakin]

> Is that what NS_INTERFACE_MAP_ENTRY_AMBIGUOUS is for? For examples, see
> elsewhere in our tree.

No, all those macros are there only to implement QueryInterface. That macro in particular also does a static_cast as with here.

Comment 60

[Markus Stange [:mstange]]

(In reply to comment #57)
> (From update of attachment 314854 [details] [diff] [review])
> Index: dom/src/base/nsGlobalWindow.cpp
> +      
> +      // We need to ask the theme if the window is active.
> +      // NS_DEACTIVATE doesn't always mean that the window loses "main" state
> 
> When would this situation occur?

When opening a sheet. (javascript:alert("sheet"))
 
> +      nsIDocShell *docShell = GetDocShell();
> +      if (docShell) {
> 
> You get the docshell and compare to null multiple times. Just get it once and
> rely on the NS_ENSURE_TRUE to null check it.

Oh, right.

> +  virtual PRBool WindowIsActive(nsIWidget* aWindow) { return PR_FALSE; }
> 
> Should this be true by default for other platforms? Or is the idea that
> 'active' would never be set for these?

The idea was to return true, I messed up.

> Or would it be better to rely on the activate/deactivate events for those
> platforms?

That's a good idea. Are there other platforms that have different-looking background windows?

> +PRBool 
> +nsNativeThemeCocoa::WindowIsActive(nsIWidget* aWindow)
> +{
> +  PRBool isActive = PR_TRUE;
> +  NSWindow* nativeWindow =
> (NSWindow*)aWindow->GetNativeData(NS_NATIVE_WINDOW);
> +
> +  if (nativeWindow) {
> +    nsWindowType windowType;
> +    aWindow->GetWindowType(windowType);
> +    isActive = [nativeWindow isMainWindow] || (windowType ==
> eWindowType_popup);
> +  }
> 
> Is there something special that requires popups to be specified here? Popups
> are not actually ever activated.

Actually, I don't know. That part is from Josh's original patch for bug 54488. Josh, do you remember why you did that?

Comment 61

[Markus Stange [:mstange]]

Attachment: Fix v5.3: Use fallback for other platforms, remove popup special case

(In reply to comment #57)
> Or would it be better to rely on the activate/deactivate events for those
> platforms?

Done.

(In reply to comment #60)
> > Is there something special that requires popups to be specified here? Popups
> > are not actually ever activated.
> 
> Actually, I don't know. That part is from Josh's original patch for bug 54488.

I guess we'll only need the special case for bug 54488, so I'm leaving it out for now.

I've contacted Kevin Gerich about the CSS changes; he said he has "all of the pieces ready for the inactive window styles" and will create a patch soon.

Comment 62

[Kevin Gerich]

I applied the latest patch and I'm experimenting with inactive window styles. I tried this in browser.css:

#main-window:not([active="true"]) > #navigator-toolbox > #nav-bar {
  opacity: 0.5;
} 

This works as expected when switching from a browser window to the window of another application, but if I switch between browser windows, the inactive style is applied to the active window and vice versa. Also, when I open Minefield, the initial browser window takes on the inactive style.

Markus, could you please test out that CSS rule and let me know if you see what I'm seeing or let me know what I'm doing wrong? Thanks!

Comment 63

[Markus Stange [:mstange]]

Attachment: Screenshot of two open browser windows with CSS rule applied

(In reply to comment #62)
> I tried this in browser.css:
> 
> #main-window:not([active="true"]) > #navigator-toolbox > #nav-bar {
>   opacity: 0.5;
> }
(The ="true" isn't necessary.)

> This works as expected when switching from a browser window to the window of
> another application, but if I switch between browser windows, the inactive
> style is applied to the active window and vice versa.

That's funny. I can't reproduce the problem, see screenshot. What color is the titlebar of the active window? Are the close/min/max buttons colored? What does the DOM Inspector say about the presence of the active attribute?
How do you switch between the windows? By clicking or with a keyboard command?

> Also, when I open
> Minefield, the initial browser window takes on the inactive style.

Might that be the case because the window *is* inactive when it opens? (see if the close button is colored)

Can somebody else try to reproduce the problem? (Maybe it only happens on 10.5?)

Comment 64

[Kevin Gerich]

Attachment: Window appearance after applying patch 5.3 on Leopard and CSS rule in comment #62

In the screenshot you can see that the DOM Inspector shows that the active attribute is present even when the window isn't active. I don't have a Tiger machine to test.

Comment 65

[Kevin Gerich]

Forgot to add that I switch between windows by clicking. If I create a new window or summon the pref window with the keyboard, I get the same appearance as my screenshot.

If I switch between apps with command-tab, the styles work correctly. 

Comment 66

[Stefan [:stefanh]]

10.5.2: I see the same things as kevin does. The titlebarcolors switch correct, though.

Comment 68

[Markus Stange [:mstange]]

Whoops, I've found the culprit. You have to attach the patch for bug 428071 as well.

Comment 69

[Kevin Gerich]

Attachment: Style changes after applying patch 5.3 and patch from bug 428071

Comment 70

[Josh Aas]

Comment on attachment 315211 [details] [diff] [review]
Fix v5.3: Use fallback for other platforms, remove popup special case

+- (NSColor*)titlebarColorActive;
+- (NSColor*)titlebarColorInactive;

A more natural way to name these would be "activeTitlebarColor" and "inactiveTitlebarColor". Active/inactive are adjectives, see the names of other colors in the NSColor API.

+                             forActiveWindow:(BOOL)aActive];

This is one character off from being correctly aligned with the prior argument.

-- (id)initWithTitlebarColor:(NSColor*)aTitlebarColor 
+- (id)initWithTitlebarColorActive:(NSColor*)aTitlebarColorActive
+         andTitlebarColorInactive:(NSColor*)aTitlebarColorInactive

Generally obj-c naming convention does not include "and" for every argument in a list of args. There is already one arg named that way (before your patch), please fix that too. Please also put active/inactive first in the names of the arguments.

+  NSColor *mTitlebarColorActive;
+  NSColor *mTitlebarColorInactive;

Same issue with these variable names.

Looks good to me other than this naming stuff. Please post a new patch as the naming fixes will touch a lot of the patch.

Comment 71

[Markus Stange [:mstange]]

Attachment: Fix v5.4: address review comments

Comment 72

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

Why can't we track NS_ACTIVATE/NS_DEACTIVATE events and keep track of whether the window is active ourselves, instead of adding a new widget API? It's not good to add a new API like that and only implement it on one platform, even though it only matters on that platform, because someone might try to use it in another context.

Comment 73

[Markus Stange [:mstange]]

(In reply to comment #72)
> Why can't we track NS_ACTIVATE/NS_DEACTIVATE events and keep track of whether
> the window is active ourselves, instead of adding a new widget API?

I don't think that's possible.

On Mac OS X, a window can have "main" state and "key" state. Most of the time, a focused window is both main and key. However, when sheets are involved, this is no longer the case.
When you open a sheet, e.g. using alert(), the containing window loses key state and the sheet becomes key; the containing window is still main. So the containing window is main, but not key, and the sheet is key, but not main.
The opening sheet causes NS_DEACTIVATE to be fired on the main window and NS_ACTIVATE on the sheet. This is correct; Gecko needs to know which window takes keyboard input, for example.

The active/inactive appearance of a window depends on its main state.
So in case of an opening sheet, the NS_DEACTIVATE event should be ignored because the window's main state hasn't changed.
I don't know how to detect this case other than asking the theme. The NS_DEACTIVATE event doesn't contain any information that could be useful, does it?

Maybe introducing different NS_ACTIVATE events for key and main state might make sense at some point in the future; for this bug it certainly doesn't.

Comment 74

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

OK.

What is the "active" attribute on the root element used for?

Comment 75

[Kevin Gerich]

Attachment: Style changes for inactive window revised

Comment 76

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

I think it would make more sense to define a new CSS pseudo-class for active windows instead of hacking the element like this.

But let's leave that aside for now, because even if we did that we'd still need a cross-platform IsActive method. So let's carry on with this approach, but make IsActive a member of nsIWidget. The base class implementation should at least do NS_ERROR to let the developer know that the method is not implemented.

Comment 77

[Markus Stange [:mstange]]

Attachment: Fix v5.5: Move isActive from nsITheme to nsIWidget

(In reply to comment #76)
> I think it would make more sense to define a new CSS pseudo-class for active
> windows instead of hacking the element like this.

I agree.

> But let's leave that aside for now, because even if we did that we'd still need
> a cross-platform IsActive method. So let's carry on with this approach, but
> make IsActive a member of nsIWidget. The base class implementation should at
> least do NS_ERROR to let the developer know that the method is not implemented.

Done.

Comment 78

[Markus Stange [:mstange]]

(In reply to comment #75)
> Created an attachment (id=316755) [details]
> Style changes for inactive window revised

Thanks Kevin, that's better.
I think you forgot the download manager's status bar.

Comment 79

[Josh Aas]

Comment on attachment 316803 [details] [diff] [review]
Fix v5.5: Move isActive from nsITheme to nsIWidget

We really need to sort out this active/focused/key/main status business in Moz 2. Somebody who knows the Mac requirements needs to figure out how things work on Win/GTK and create a well-documented system that works for everyone.

Comment 80

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

This is the explanation from Apple:

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

Comment 81

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

> The base class implementation should at
> least do NS_ERROR to let the developer know that the method is not implemented.

I think the patch still needs this? Other than that, the patch looks good.

nsIWidget needs some serious deCOMtamination, wonder if I can find a volunteer for that. Markus, are you interested in replacing this attribute with a content state bit and CSS pseudo-class selector, post-1.9?

Comment 82

[Markus Stange [:mstange]]

(In reply to comment #81)
> > The base class implementation should at
> > least do NS_ERROR to let the developer know that the method is not implemented.
> 
> I think the patch still needs this? Other than that, the patch looks good.

It's in nsBaseWidget.cpp:

>  //-------------------------------------------------------------------------
>  //
> +// Is the window active?
> +//
> +//-------------------------------------------------------------------------
> +NS_IMETHODIMP nsBaseWidget::IsActive(PRBool* aState)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}

> Markus, are you interested in replacing this attribute with a content
> state bit and CSS pseudo-class selector, post-1.9?

Yes. :)

Comment 83

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

Sorry I meant that actually NS_ERROR("We're doomed!") macro.

Comment 84

[Markus Stange [:mstange]]

Ah, sure. But then IsActive really must not be called on these platforms, right? The current strategy is to call it on all platforms and fall back to the event type if it fails (as suggested by Neil in comment 57):

>+      PRBool isActive = PR_TRUE;
>+      nsresult rv = mainWidget->IsActive(&isActive);
>+      if (NS_FAILED(rv))
>+        isActive = (aVisitor.mEvent->message == NS_ACTIVATE);

Setting the "active" attribute cross-platform helps the portability of Mac themes.

Comment 85

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

OK then, don't worry about it.

Comment 86

[Mike Connor [:mconnor]]

Comment on attachment 316803 [details] [diff] [review]
Fix v5.5: Move isActive from nsITheme to nsIWidget

a=mconnor on behalf of 1.9 drivers

Comment 87

[Markus Stange [:mstange]]

Note that the patch shouldn't be landed as long as bug 428071 isn't solved (because of the bug mentioned by Kevin in comment 62).

Comment 88

[Markus Stange [:mstange]]

Attachment: Fix v5.6: bug 428071 requires this

Changes from the Fix v5.5:
- nsGlobalWindow now looks for the top level window so that NS_DEACTIVATE
  events fired on the sheet can deactivate its containing window
- added nsCocoaWindow::GetParent
- modified nsXULWindow to return nsIDOMWindowInternal when requested
  to allow for the clientData hack

I manually removed the bug 428071 changes from the nsCocoaWindow.mm diff, but kept them in the nsCocoaWindow.h diff because they're overlapping there.

Do you think this is ok, Steven?

Comment 89

[Steven Michaud [:smichaud] (Retired)]

I'll get to this tomorrow.

Comment 90

[Steven Michaud [:smichaud] (Retired)]

I notice that your current patch changes the behavior of
nsCocoaWidget::GetParent().  I'm not sure this is safe (Josh may have
more to say on this subject).

Previously nsCocoaWindow inherited this method from nsBaseWidget,
where it returns NULL.  The idea was (I think) that _every_
nsCocoaWindow object (even a sheet) is a top-level widget.  The only
widgets (in Cocoa widgets) that aren't top level widgets are
nsChildView objects.

Now you've changed the code so that, when an nsCocoaWindow object does
have a parent (when StandardCreate() is called with aParent not NULL),
nsCocoaWindow::GetParent() will return that object.  So the code which
assumes that even nsCocoaWindow object is a top-level widget may no
longer work properly.

I now think I gave you wrong advice when (in bug 428071 comment #19) I
suggested you use nsIWidget::GetParent() to find a sheet's "parent".
I think you'll need to use some other call -- possibly one you add to
a private interface.

(I'll post additional comments over the course of the day.)

Comment 91

[Steven Michaud [:smichaud] (Retired)]

+NS_IMETHODIMP nsCocoaWindow::IsActive(PRBool *aState)
+{
+  if (aState)
+    *aState = [mWindow isMainWindow];
+  return NS_OK;
+}

This only gives a correct result if the nsCocoaWindow isn't a sheet
(since sheets never become main).

How about changing it as follows:

NS_IMETHODIMP nsCocoaWindow::IsActive(PRBool *aState)
{
  if (aState) {
    if (mWindowType == eWindowType_sheet) {
      *aState = [mWindow isKeyWindow];
    } else {
      *aState = [mWindow isMainWindow];
    }
  }
  return NS_OK;
}

(The reason I don't suggest using [mWindow isSheet] is that's false
when the current window isn't showing -- even if it _is_ a sheet.)

Comment 92

[Markus Stange [:mstange]]

(In reply to comment #90)
> I think you'll need to use some other call -- possibly one you add to
> a private interface.

Yeah, that's probably better, nsCocoaWindow::GetParent() does get called quite frequently (e.g. when calculating relative mouse positions in chromeless windows).

How should I implement the "private interface" thing? I'm not sure I know what you mean. I only have a pointer to an nsIWidget, so I'll have to add another method there, won't I?

(In reply to comment #91)
> +NS_IMETHODIMP nsCocoaWindow::IsActive(PRBool *aState)
> +{
> +  if (aState)
> +    *aState = [mWindow isMainWindow];
> +  return NS_OK;
> +}
> 
> This only gives a correct result if the nsCocoaWindow isn't a sheet
> (since sheets never become main).

Which was kind of fine since I never ask a sheet for IsActive, only the containing top-level window.

> How about changing it as follows:
> 
> NS_IMETHODIMP nsCocoaWindow::IsActive(PRBool *aState)
> {
>   if (aState) {
>     if (mWindowType == eWindowType_sheet) {
>       *aState = [mWindow isKeyWindow];
>     } else {
>       *aState = [mWindow isMainWindow];
>     }
>   }
>   return NS_OK;
> }

I'll do that (and call widget->IsActive instead of topLevelWidget->IsActive). In combination with bug 428071 comment 32 it really makes sense. :)

> (The reason I don't suggest using [mWindow isSheet] is that's false
> when the current window isn't showing -- even if it _is_ a sheet.)

Heh.

Comment 93

[Steven Michaud [:smichaud] (Retired)]

> How should I implement the "private interface" thing?

Even I'm not too sure (yet) ... so maybe you should forget this
suggestion :-)

If the powers that be will allow it, it's probably best to add another
new method to nsIWidget.  And if they don't like this, they're in a
better position than I am to suggest alternatives.

Comment 94

[Neil Deakin]

If it's Mac specific, you could also instead add it to nsPIWidgetCocoa.idl

Comment 95

[Steven Michaud [:smichaud] (Retired)]

> nsPIWidgetCocoa.idl

Can this be used from outside the widget module?  I notice that it currently isn't.

Comment 96

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix v 6.0: Additional changes for bug 428071

This patch avoids the potentially dangerous change to
nsCocoaWindow::GetParent(), and instead adds another new method to the
nsIWidget interface (GetSheetWindowParent()).

I've also found that we don't really need nsIWidget::IsActive(), so
I've gotten rid of it.

Otherwise this patch is the same as Markus's v5.6 patch.

This patch must be tested together with the patches for bug 428071 and
bug 431429 (the latter fixes an unrelated bug that would make testing
extremely painful).

In an hour or so I'll post another patch that combines the patches for
all three bugs, along with a tryserver build made with it.

Comment 97

[Steven Michaud [:smichaud] (Retired)]

Attachment: Combo patch for bugs 406730, 428071 and 431429 (for testing)

Here's the combo patch I promised above.  I'm not asking for review on
it specifically -- only on its component parts (in the relevant bugs).

And here's a tryserver Mac build:

https://build.mozilla.org/tryserver-builds/2008-04-30_13:[e-mail]-bugzilla406730-428071-431429/[e-mail]

Tryserver builds for Linux and Windows will be available in a while at
the same location:

https://build.mozilla.org/tryserver-builds/2008-04-30_13:[e-mail]-bugzilla406730-428071-431429/

Comment 98

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

This depends on 428071, which is too risky for 1.9. We'll just have to live with the bug for now.

Comment 99

[Mike Beltzner [:beltzner, not reading bugmail]]

Renominating for blocking. This is a significant usability issue on OSX, and I would like to have a full discussion (not an email thread that spans 10 minutes of clock-time and has no representation from the front end driver community) about possible mitigation and full impact before we decide to not block on it.

Comment 100

[Mike Beltzner [:beltzner, not reading bugmail]]

Is there any way we can resolve this issue without 428071? I'm thinking mitigate, mitigate, mitigate!

Comment 101

[Steven Michaud [:smichaud] (Retired)]

> Is there any way we can resolve this issue without 428071?

The two people who've done the work on this bug should have some credence here.

I'd say "no", and I'm pretty sure Markus would say the same.

Comment 102

[Steven Michaud [:smichaud] (Retired)]

By the way, for whoever's interested in this issue I strongly suggest the following:

Test the tryserver build from comment #97, and compare its behavior with a current Minefield nightly.  Ask yourselves how significant the difference is.

Comment 103

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

+        // Ask for nsIDOMWindowInternal and pray that it's an nsGlobalWindow
+        req->GetInterface(NS_GET_IID(nsIDOMWindowInternal), (void**)&topLevelWindow);

This is really fragile. It not only depends on nsGlobalWindow implementing nsIDOMWindowInternal, it depends on that being the first interface in its interface list. Why don't you just make topLevelWindow an nsCOMPtr<nsIDOMWindow> and get the document via nsIDOMWindow::GetDocument?

+    [mActiveTitlebarColor autorelease];

How come we use 'release' in some places and 'autorelease' here? I don't really know what the difference is.

Seems like we really need some kind of RetainPtr<T> though.

Comment 104

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix v 7.0: Let's try something different

This should shake things up a bit :-)

> +    [mActiveTitlebarColor autorelease];
>
> How come we use 'release' in some places and 'autorelease' here? I
> don't really know what the difference is.

My own personal preference is to always use 'release' in cases like
this.  But the original (pre-patch) code uses 'autorelease'.  And if
we're concerned about introducing bugs at the last minute, we should
leave things the way they were.

> Why don't you just make topLevelWindow an nsCOMPtr<nsIDOMWindow> and
> get the document via nsIDOMWindow::GetDocument?

This doesn't work.

But along the way to finding this out I discovered something very
interesting:  The active/inactive window restyling still works even
when I get rid of all the code that the original patches added to
nsGlobalWindow::PostHandleEvent()!!

So here's a new patch that drops a lot of the code from previous
patches, and (as an added bonus) doesn't require the patch for bug
428071.

A tryserver build will follow shortly.

I'm not familiar with the non-Cocoa-widgets code that this patch
touches, so I frankly don't know _why_ it works.  I'm not going to ask
for reviews until someone can give me a convincing answer :-)

Comment 105

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

Well sure. It means we don't update the 'active' attribute on the XUL document so none of the style rules work, so the window titlebar will change color but none of the rest of the chrome will. This is what Vlad suggested trying. It means, though, that an inactive window looks bad because the titlebar has changed color but the toolbar that's supposed to be unified with the titlebar hasn't changed --- right?

Comment 106

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

(In reply to comment #104)
> My own personal preference is to always use 'release' in cases like
> this.  But the original (pre-patch) code uses 'autorelease'.  And if
> we're concerned about introducing bugs at the last minute, we should
> leave things the way they were.

Sure OK.

> > Why don't you just make topLevelWindow an nsCOMPtr<nsIDOMWindow> and
> > get the document via nsIDOMWindow::GetDocument?
> 
> This doesn't work.

Why not, BTW?

Comment 107

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my v7.0 patch:

https://build.mozilla.org/tryserver-builds/2008-05-01_18:[e-mail]-bugzilla406730-v70/[e-mail]

Comment 108

[Steven Michaud [:smichaud] (Retired)]

> Well sure. It means we don't update the 'active' attribute on the
> XUL document so none of the style rules work, so the window titlebar
> will change color but none of the rest of the chrome will. This is
> what Vlad suggested trying. It means, though, that an inactive
> window looks bad because the titlebar has changed color but the
> toolbar that's supposed to be unified with the titlebar hasn't
> changed --- right?

Compare the results you get with the tryserver build from comment #97
with the one from comment #107.  To me they look the same.

Comment 109

[Shawn Wilsher :sdwilsh]

Both appear to do the same thing - does either build include attachment 316755 [details] [diff] [review] (the style changes)?

Comment 110

[Mike Beltzner [:beltzner, not reading bugmail]]

smichaud: can you post a screenshot of what this looks like? I left my mac at the office :(

Comment 111

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: screenshot of v7

Comment 112

[Mike Connor [:mconnor]]

Comment on attachment 318663 [details] [diff] [review]
Fix v 6.0: Additional changes for bug 428071

let's get this landed, we can always back out at the first sign of trouble

Comment 113

[Markus Stange [:mstange]]

Attachment: Screenshot compilation

I don't think it should be landed without the "active" attribute functionality and the theme changes. I'm working on it as fast as I can.

Comment 114

[Markus Stange [:mstange]]

Attachment: Fix v8.0: Return of the "active" attribute

(In reply to comment #106)
> > > Why don't you just make topLevelWindow an nsCOMPtr<nsIDOMWindow> and
> > > get the document via nsIDOMWindow::GetDocument?
> > 
> > This doesn't work.
> 
> Why not, BTW?

I'd like to know that, too... It works for me :)

Comment 115

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

+    nsCOMPtr<nsIDOMWindow> topLevelWindow;
+    if (topLevelWidget == mainWidget) {
+      topLevelWindow = do_QueryInterface(static_cast<nsIDOMWindow *>(this));

Why do you need the static cast here? I'm not even sure why you need the do_QueryInterface.

+      void* clientData;
+      topLevelWidget->GetClientData(clientData); // clientData is nsXULWindow
+      nsISupports* data = (nsISupports*)clientData;

static_cast

+        // Ask for nsIDOMWindowInternal and pray that it's an nsGlobalWindow

This comment is no longer relevant.

+        req->GetInterface(NS_GET_IID(nsIDOMWindowInternal), (void**)&topLevelWindow);

Can't you use do_GetInterface, and topLevelWindow actually an nsCOMPtr<nsIDOMWindowInternal>?

Comment 116

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

+  if (aIID.Equals(NS_GET_IID(nsIDOMWindowInternal))) {
+    nsCOMPtr<nsIDOMWindowInternal> window;
+    nsresult rv = GetWindowDOMWindow(getter_AddRefs(window));
+    if (NS_FAILED(rv)) return rv;
+    return window->QueryInterface(aIID, aSink);

Can't this just be "return GetWindowDOMWindow(aSink);"?

Comment 117

[Markus Stange [:mstange]]

Attachment: Fix v8.1: address review comments

(In reply to comment #115)
> +    nsCOMPtr<nsIDOMWindow> topLevelWindow;
> +    if (topLevelWidget == mainWidget) {
> +      topLevelWindow = do_QueryInterface(static_cast<nsIDOMWindow *>(this));
> 
> Why do you need the static cast here?

Because "'nsISupports' is an ambiguous base of 'nsGlobalWindow'", see comment 53 to 56.

> I'm not even sure why you need the
> do_QueryInterface.

Err, yes, I didn't need that. But now I do (nsIDOMWindow -> nsIDOMWindowInternal).

> +        req->GetInterface(NS_GET_IID(nsIDOMWindowInternal),
> (void**)&topLevelWindow);
> 
> Can't you use do_GetInterface, and topLevelWindow actually an
> nsCOMPtr<nsIDOMWindowInternal>?

Nice.

(In reply to comment #116)
> +  if (aIID.Equals(NS_GET_IID(nsIDOMWindowInternal))) {
> +    nsCOMPtr<nsIDOMWindowInternal> window;
> +    nsresult rv = GetWindowDOMWindow(getter_AddRefs(window));
> +    if (NS_FAILED(rv)) return rv;
> +    return window->QueryInterface(aIID, aSink);
> 
> Can't this just be "return GetWindowDOMWindow(aSink);"?

I made it return GetWindowDOMWindow(reinterpret_cast<nsIDOMWindowInternal**>(aSink));
Am I using reinterpret_cast the right way here?

Comment 118

[Markus Stange [:mstange]]

Attachment: CSS changes

I added a download manager search bar style. Other than that, it's the same as Kevin's attachment 316755 [details] [diff] [review].

Comment 119

[Markus Stange [:mstange]]

Attachment: Update combo patch for bugs 406730 and 428071 with CSS changes (for testing)

It would be cool if someone made a tryserver build using this patch.

Comment 120

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

(In reply to comment #117)
> Err, yes, I didn't need that. But now I do (nsIDOMWindow ->
> nsIDOMWindowInternal).

No, you can static_cast to nsIDOMWindowInternal.

> (In reply to comment #116)
> > +  if (aIID.Equals(NS_GET_IID(nsIDOMWindowInternal))) {
> > +    nsCOMPtr<nsIDOMWindowInternal> window;
> > +    nsresult rv = GetWindowDOMWindow(getter_AddRefs(window));
> > +    if (NS_FAILED(rv)) return rv;
> > +    return window->QueryInterface(aIID, aSink);
> > 
> > Can't this just be "return GetWindowDOMWindow(aSink);"?
> 
> I made it return
> GetWindowDOMWindow(reinterpret_cast<nsIDOMWindowInternal**>(aSink));
> Am I using reinterpret_cast the right way here?

Yeah, I think so.

Comment 121

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

> No, you can static_cast to nsIDOMWindowInternal.

I fixed that and checked that patch in.

I think to keep things simple, the theme changes should move to their own bug.

Comment 122

[Markus Stange [:mstange]]

Great, thanks!
I'll file the bug.

Comment 123

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #114)

> I'd like to know that, too... It works for me :)

I did it the wrong way (in the wrong place).

Comment 124

[Asa Dotzler [:asa]]

theme bug is bug 431831

Comment 125

[Stuart Morgan]

We haven't done a local backout to be sure yet, but there is strong evidence in bug 431910 that this patch was not embedding-friendly, and is causing crashes in Camino due a null mainWidget (see bug 431910 for the line). I haven't had time to look too closely at this patch, but depending on the code path GetMainWidget cannot be relied on to be non-null for embeddors (see for example bug 404290).

Comment 126

[Markus Stange [:mstange]]

Attachment: Fix embedding bustage [relanded]

Comment 127

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319140 [details] [diff] [review]
Fix embedding bustage [relanded]

a=beltzner

Comment 128

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

I've taken a look at this with a fresh debug build. What I can see is that switching between the different states is a bit slow and you see it flicker. Does it happen due to the debug build or is an optimization needed? IMO it doesn't look well at the moment.

Comment 129

[Nick Fagerlund]

No, I'm seeing that in the regular nightlies as well -- it just doesn't transition as snappily as other apps. 

Granted, "slow" is (IMO) a big step up from "broken." 

Comment 130

[Asa Dotzler [:asa]]

It's pretty instantaneous on a contemporary MBP running 10.5.2. I really cannot see the difference between a Firefox window and a Finder window in how long it takes to transition from light to dark an back.

Comment 131

[Nick Fagerlund]

I'm using a previous-generation MacBook (Core Duo 2ghz, 2GB RAM), and while the lag is noticeable, it's not a big deal.

(G4s in the house?)

Comment 132

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

Comment on attachment 319140 [details] [diff] [review]
Fix embedding bustage [relanded]

I checked this in and will monitor boxset; hope this fixes it :)

Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.1014; previous revision: 1.1013
done

Comment 133

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

Comment on attachment 319140 [details] [diff] [review]
Fix embedding bustage [relanded]

I backed this out because qm-centos5-03 went orange after this checkin and no-one around thought the failing mochitests were among the "usual suspects".

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1209867207.1209870295.28903.gz

	FAIL - Checking 'BrowserTab.uri' after opening - Got about:blank, expected chrome://mochikit/content/browser/browser/fuel/test/ContentA.html - chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js
	FAIL - Checking 'BrowserTab.uri' after opening - Got about:blank, expected chrome://mochikit/content/browser/browser/fuel/test/ContentB.html - chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js
	FAIL - Checking existence of element in content DOM - chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js
	FAIL - Timed out - chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js

It had been an hour and I also hadn't gotten any positive response from boxset yet, and I can't stay up much later, so backing out was the best course of action. :(

Comment 134

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

Comment on attachment 319140 [details] [diff] [review]
Fix embedding bustage [relanded]

Sorry for all the bugspam here, but I relanded this since the Linux orange went green on its own before it picked up my backout and since Phil volunteered to baby-sit the tree after I go to sleep.

This does fix the warning about nasty stuff in the embedding case even though it doesn't appear to fix boxset (we'll have to keep investigating bug 431910), so it seems like we want this fix on that basis alone.

Comment 135

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

(In reply to comment #130)
> It's pretty instantaneous on a contemporary MBP running 10.5.2. I really cannot
> see the difference between a Firefox window and a Finder window in how long it
> takes to transition from light to dark an back.

Asa, which build are you using? The latest nightly doesn't have the patch on bug 431831 included, so only the title bar and the shadow is changing. That is what I can see with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050209 Minefield/3.0pre ID:2008050209. You should get an hourly build to run the test.

Shall I file a new bug for the slowness?

Comment 136

[Asa Dotzler [:asa]]

> Asa, which build are you using? 

I picked up the first tinderbox build from http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/bm-xserve08-trunk/ after the changes landed.

I see the full theme change in my build and like I said, it's plenty fast on my MBP on 10.5.2

Comment 137

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

Ok, I grabbed that build and run a test. The transition for the Navigation Toolbar with large icons is still behind the title bar, which causes that flicker. So you don't see it with a MBP? Perhaps it's an issue with the slower graphic card in a MB?

Comment 138

[Asa Dotzler [:asa]]

I'm on large icons with the default set and there's absolutely no discernible difference between the transition time for the titlebar and the toolbar.  

My MBP does have the pretty decent GeForce 8600M GT with 512 MB VRAM which I'd wager is a step above what's available in a MB. 

Comment 139

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

Ok, I did further investigation and I'm not able to see this behavior with a brand-fresh debug build Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050410 Minefield/3.0pre on my Mac Mini which also only have a GMA 950 graphic card.

Marcia or Stephen, is there any chance to run tests on the same machine on Tiger and Leopard? It could be that the issue I mentioned in comment 137 only happens with Tiger.

Comment 140

[Stephen Horlander [:shorlander]]

I see a slight delay the first time I switch open windows. Probably as it loads the image for the first time. After that I get no noticeable difference. I am on a MBP.

Comment 141

[Markus Stange [:mstange]]

I have filed bug 432131 for the flash.

Comment 142

[cris]

does this color change should apply to firefox in tiger at all?

Comment 143

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

In general it is working fine. Remaining issues should all be handled on their own bugs.

Marking Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008051304 Minefield/3.0pre ID:2008051304

Looks like a good candidate for litmus. Users should run tests in switching applications and using different windows of Firefox, e.g. browser windows, options, download manager, and others.

Comment 144

[Yehuda]

Attachment: Active tab is broken in RTL when window is unfocused

Looks like this fix breaks some of the fixes for bug 364536: When the window is unfocused, the active tab looks "broken". 

Sorry for spotting this after RC1 is out.

Comment 145

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

(In reply to comment #144)
> Created an attachment (id=321454) [details]
> Active tab is broken in RTL when window is unfocused

Could you please file a new bug on this?

Comment 146

[Yehuda]

(In reply to comment #145)
> 
> Could you please file a new bug on this?
> 

Added bug 434360

Comment 147

[Marek Stępień [:marcoos, inactive]]

This looks great now! 

There's one really minor issue - when the window is minimized to the dock and then you click to restore it, it's light-colored during the geenie effect and only gets dark when it's restored. Other apps are dark even during the geenie effect.

Comment 148

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

(In reply to comment #147)
> There's one really minor issue - when the window is minimized to the dock and
> then you click to restore it, it's light-colored during the geenie effect and
> only gets dark when it's restored. Other apps are dark even during the geenie
> effect.

Marek, same applies to you. Please also file a new bug on this.

Comment 149

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #148)
> Marek, same applies to you. Please also file a new bug on this.

Filed bug 434378.

Comment 150

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

Thinking about the in-litmus flag makes me wonder if it is really needed. This implementation is very obvious and will be definitely identified by nightly testers.