User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130618035212 Steps to reproduce: This only happens in Firefox 22 on a Mac, as far as I know. Go to a page that has the "Are you sure you want to leave, you have unsaved data" dialog. One example is here: 1. Go to http://en.wikipedia.org/w/index.php?title=Example&action=edit&section=0 2. Type something in the text area. 3. Click on any of the links to leave the page. Actual results: None of the two buttons available are selected, which is normally indicated with a blue background color to the button. Expected results: The "Leave Page" button should be selected, indicated by a blue background color. Once I hover my mouse over the button, or select it with the Tab key, then the button is selected (indicated by the blue background color).

Confirming. nsDocumentViewer correctly sets a default button [1], so moving to Core::Widget: Cocoa for further investigation. [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1181

Are you saying this doesn't happen in Firefox 21?

Correct, it definitely does not happen in Firefox 21. I noticed it immediately after upgrading to Firefox 22.

I can confirm this happens in FF 22 but not FF 21. I've also seen it myself, with a different STR (more about that later). This bug *doesn't* involve modal windows. Rather it involves "sheets" (native objects that are window-modal).

Here's alternate STR, which work when you enable the "Quit Firefox" dialog (also a sheet). To enable the quit warning, enable browser.showQuitWarning in about:config. 1) Run FF. 2) Open at least one additional tab (so that at least two are open). 3) Press Cmd-q or choose Firefox : Quit Firefox from the menu. The "Quit Firefox" dialog will appear, which has the same problem.

The first mozilla-central nightly with this bug is: firefox-2013-03-0803-10-05-mozilla-central Which implies the following regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ee4879719f78&tochange=cb432984d5ce

> firefox-2013-03-0803-10-05-mozilla-central firefox-2013-03-08-03-10-05-mozilla-central

I bisected by hand, and found that this patch caused/triggered this bug: http://hg.mozilla.org/mozilla-central/rev/b1557651f08a Bug 839029 - Don't repaint default button if disabled or inactive. r=mstange, r=josh author Andre Reinald <areinald@mozilla.com> Thu Mar 07 17:26:39 2013 -0500 (at Thu Mar 07 17:26:39 2013 -0500) André, can I assign this bug to you? :-)

The above patch is just about repainting: the default button still behaves like expected. Just press enter to check. When reproducing the bug, I can see that *before* the sheet appears on screen nsNativeThemeCocoa::DrawWidgetBackground is called for the default button, at which point FrameIsInActiveWindow returns false (which is correct), and no animation is started. I think the best way to fix this bug, and probably other similar bugs, would be to have nsNativeThemeCocoa::DrawWidgetBackground called when a window/frame is shown, hidden, activated, deactivated, enabled or disabled. But I don't know where that should happen. The easy workaround is, instead of not calling QueueAnimatedContentForRefresh when we don't need an animation, call it anyway but with a much higher value (let's say 1 second). This would allow the button to blink after a short initial delay, without noticeably impacting performance (see but 839029), and without requiring other changes.

> The easy workaround is, instead of not calling > QueueAnimatedContentForRefresh when we don't need an animation, call > it anyway but with a much higher value (let's say 1 second). I'd prefer not to do this except as a last resort. > I think the best way to fix this bug, and probably other similar > bugs, would be to have nsNativeThemeCocoa::DrawWidgetBackground > called when a window/frame is shown, hidden, activated, deactivated, > enabled or disabled. But I don't know where that should happen. Sounds reasonable to me. Markus, what do you think? Is this something that could be done relatively easily (as far as you know)? Should we also ask someone else? > When reproducing the bug, I can see that *before* the sheet appears > on screen nsNativeThemeCocoa::DrawWidgetBackground is called for the > default button, at which point FrameIsInActiveWindow returns false > (which is correct), and no animation is started. Markus, do you think it'd be worthwhile trying to fix this problem directly?

Markus: ping

FWIW, another easy way to reproduce is by loading a site that requires HTTP auth (eg http://intranet.mozilla.org), which is why this has been bugging me. :)

Looks like Markus isn't going to respond to my questions. At some point in the not-too-distant future I hope to re-examine bug 839029, to see if I can find a way to fix it that doesn't cause/trigger this bug. I'll start by seeing where the "extra" paints mostly come from. With luck I'll be able to get to this next week. If it turns out I don't have the time, we can do some kind of workaround here.

Sorry, I was going to answer, but I wanted to do some tests first. Repainting things that change their appearance when window focus changes is enforced by a full-window invalidation in PresShell::DocumentStatesChanged for the flag NS_DOCUMENT_STATE_WINDOW_INACTIVE: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#3996 This is called by nsGlobalWindow::SetActive which is called (through an event, probably) from a Cocoa window delegate function, if I recall correctly. What we need to find out here is which window state change events Cocoa fires at which point during sheet opening (didBecomeKey and didBecomeMain are the interesting ones), and which of those we translate to NS_ACTIVATE events, and if not, why not. The ideal state would be to have the sheet window in "deactivated" state (from a Gecko perspective) on creation, and only "activate" it at the point where the sheet is fully open and the button should become blue. Then the window state change will repaint the button (by invalidating everything).

Thanks, Markus. This should help me in my search.

I finally have time to start looking at this. So far I've discovered that the single call to nsNativeThemeCocoa::DrawWidgetBackground(NS_THEME_BUTTON) that takes place while the Quit dialog sheet is being displayed is for the window underneath it -- not for the sheet (as it should be). I'll keep digging.

(Following up comment #17) Actually that's not true. The problem's that NativeWindowForFrame() returns the wrong window -- not the sheet (as it should), but the window above the sheet. Sounds like this is the key to this bug. With luck I'll have a patch tomorrow.

> The problem's that NativeWindowForFrame() returns the wrong window This isn't true, either :-( But I'm slowly making progress. Now I've found something I think really is true, and which I suspect to be the cause of part of this bug: When you activate and deactivate a window, nsGlobalWindow::SetActive() gets called on the same window (an nsGlobalChromeWindow object), regardless of whether or not a sheet is present. This is presumably the sheet's "parent". This happens even though the parent window and its sheet (correctly) have different "listeners" (nsWebShellWindow objects), and WindowActivated() and WindowDeactivated() are correctly called on those listeners. So presumably there are bugs in nsWebShellWindow::WindowActivated() and WindowDeactivated(), or in code called by those methods. I'll keep digging.

Here's a patch that fixes this bug in my tests. (In reply to comment #9) > When reproducing the bug, I can see that *before* the sheet appears > on screen nsNativeThemeCocoa::DrawWidgetBackground is called for the > default button, at which point FrameIsInActiveWindow returns false > (which is correct), and no animation is started. It took me a while, but I also noticed this, André. I didn't fix it, but I found what I think is a bullet-proof workaround (though I had to use an undocumented technique to accomplish it). My changes to nsNativeThemeCocoa.mm "fixed" (or rather worked around) the bug as originally reported. But then I noticed that sheet buttons still didn't behave properly: When I clicked on another window (Firefox's or another app's), or on the desktop, and then back again on the original window, I noticed that nsNativeThemeCocoa::DrawWidgetBackground() didn't get called, and the active button didn't get updated. After much digging around I discovered the reason -- a workaround for a different bug in nsGlobalWindow::ActivateOrDeactivate(). This caused the sheet not to receive NS_ACTIVATE or NS_DEACTIVATE events -- only the window above it. So the sheet wasn't getting restyled when it should have been. I've started an all-platform set of tryserver runs. The tests won't complete for many hours. But a build with this patch should be available in another hour or two. I'll post it when that happens.

Here's the tryserver build made with my patch from comment #20: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-b3868bf34f69/try-macosx64/firefox-26.0a1.en-US.mac.dmg

Comment on attachment 800976 [details] [diff] [review] Fix Neil, please review this patch's changes to nsGlobalWindow::ActivateOrDeactivate(). The code I'm changing was originally written by you. Both the sheet and its parent window need to be restyled. But if you can think of better ways to accomplish this, please let me know.

Comment on attachment 800976 [details] [diff] [review] Fix Markus, please review my patch's changes to nsNativeThemeCocoa.mm.

Comment on attachment 800976 [details] [diff] [review] Fix Is the nsNativeThemeCocoa.mm change still necessary, given the nsGlobalWindow.cpp change? Why? When is the NS_ACTIVATE event sent to the sheet window, with your change? Before the sheet opening animation or after?

Comment on attachment 800976 [details] [diff] [review] Fix >- // When a window with an open sheet loses focus, only the sheet window >- // receives the NS_DEACTIVATE event. However, it's not the sheet that >- // should lose the active styling, but the containing top level window. >+ // When a window with an open sheet gains or loses focus, only the sheet >+ // window receives the NS_ACTIVATE/NS_DEACTIVATE event. However the >+ // styling of the containing top level window also needs to change. We >+ // get around this by sending the event to both windows. This patch doesn't cause any changes to what events are sent, so that comment doesn't seem right. This patch makes the change such that :-moz-window-inactive now applies to neither the top level window nor the sheet, effectively making both windows active from the perspective of styling. It doesn't change any actual focus behaviour though that I can see. Can you confirm that is the intent here?

(In reply to comment #25) How about if I change the last sentence in my comment to: "We get around this by calling nsPIDOMWindow::SetActive() on both windows." And also change the comment at the top of nsGlobalWindow::ActivateOrDeactivate() to: // Set / unset mIsActive on the top level window, which is used for the // :-moz-window-inactive pseudoclass, and its sheet (if any). No, I didn't (and don't) intend to change focus behavior. I also hope and assume my patch doesn't break the :-moz-window-inactive pseudoclass ... though I don't really understand how that works.

(In reply to comment #24) > Is the nsNativeThemeCocoa.mm change still necessary, given the > nsGlobalWindow.cpp change? It's not *absolutely* necessary. But without it the animation starts a fraction of a second late. (From comment #15) > The ideal state would be to have the sheet window in "deactivated" > state (from a Gecko perspective) on creation, and only "activate" it > at the point where the sheet is fully open and the button should > become blue. Then the window state change will repaint the button > (by invalidating everything). This is what's currently happening (even with the sheet), and is the source of the trouble. The first call to nsNativeThemeCocoa::DrawWidgetBackground() (and FrameIsInActiveWindow()) on the sheet comes in as the result of a call to nsCocoaWindow::Show() (and to nsXULWindow::ShowModal()). At this point the sheet is still in its initial, deactivated state. Without my changes to nsGlobalWindow::ActivateOrDeactivate(), this is the only call to nsNativeThemeCocoa::DrawWidgetBackground() on the sheet (as it's first being shown). But with my changes there's also a second call to nsNativeThemeCocoa::DrawWidgetBackground(), made (indirectly) as the result of a call to nsGlobalWindow::ActivateOrDeactivate(true), which is in turn triggered by the OS calling -[NSWindow becomeKeyWindow:] on the sheet.

(In reply to Steven Michaud from comment #27) > (In reply to comment #24) > > > Is the nsNativeThemeCocoa.mm change still necessary, given the > > nsGlobalWindow.cpp change? > > It's not *absolutely* necessary. But without it the animation starts > a fraction of a second late. Is it different than what native apps do? In my tests, native apps have clear default buttons during the sheet window opening animation, and blue pulsating buttons as soon as the opening animation completes. > (From comment #15) > > > The ideal state would be to have the sheet window in "deactivated" > > state (from a Gecko perspective) on creation, and only "activate" it > > at the point where the sheet is fully open and the button should > > become blue. Then the window state change will repaint the button > > (by invalidating everything). > > This is what's currently happening (even with the sheet), and is the > source of the trouble. Well, I'd say the source of the trouble is that the key state change doesn't invalidate the window. > The first call to nsNativeThemeCocoa::DrawWidgetBackground() (and > FrameIsInActiveWindow()) on the sheet comes in as the result of a call > to nsCocoaWindow::Show() (and to nsXULWindow::ShowModal()). At this > point the sheet is still in its initial, deactivated state. Good. > Without my changes to nsGlobalWindow::ActivateOrDeactivate(), this is > the only call to nsNativeThemeCocoa::DrawWidgetBackground() on the > sheet (as it's first being shown). But with my changes there's also a > second call to nsNativeThemeCocoa::DrawWidgetBackground(), made > (indirectly) as the result of a call to > nsGlobalWindow::ActivateOrDeactivate(true), which is in turn triggered > by the OS calling -[NSWindow becomeKeyWindow:] on the sheet. This sounds like exactly what we want.

(In reply to Neil Deakin from comment #25) > This patch makes the change such that :-moz-window-inactive now applies to > neither the top level window nor the sheet, effectively making both windows > active from the perspective of styling. It doesn't change any actual focus > behaviour though that I can see. > > Can you confirm that is the intent here? The actual intent is to get PresShell::DocumentStatesChanged to call FrameLayerBuilder::InvalidateAllLayersForFrame for the sheet window. The change to :-moz-window-inactive is a somewhat unintended side-effect, which I admit I hadn't thought about, but I think it's actually a positive change.

>>> Is the nsNativeThemeCocoa.mm change still necessary, given the >>> nsGlobalWindow.cpp change? >> >> It's not *absolutely* necessary. But without it the animation >> starts a fraction of a second late. > > Is it different than what native apps do? In my tests, native apps > have clear default buttons during the sheet window opening > animation, and blue pulsating buttons as soon as the opening > animation completes. OK, fair enough. I see that this happens with the Print dialog (a native sheet) in both Safari and the Contacts app. I'll drop my changes to nsNativeThemeCocoa.mm. I'll post a new patch once it's clear what Neil Deakin wants.

Neil, do you agree with my comment changes from comment #26?

They are ok.

What I'll land, once mozilla-inbound reopens. Carrying forward Neil's r+.

Comment on attachment 802565 [details] [diff] [review] Fix rev1 Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/98a74c28ee4d