892547 - Sheets on Mac don't have a default option selected, until the default option is moused over or selected using the the Tab key.

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Gary]

Attachment: Combined images.png

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).

Comment 1

[Drew Willcoxon :adw]

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

Comment 2

[Steven Michaud [:smichaud] (Retired)]

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

Comment 3

[Gary]

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

Comment 4

[Steven Michaud [:smichaud] (Retired)]

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).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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

Comment 7

[Steven Michaud [:smichaud] (Retired)]

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

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

Comment 8

[Steven Michaud [:smichaud] (Retired)]

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 <[e-mail]>
	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? :-)

Comment 9

[André Reinald]

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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> 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?

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Markus:  ping

Comment 13

[Justin Dolske [:Dolske]]

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. :)

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 15

[Markus Stange [:mstange]]

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).

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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

Comment 17

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

(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.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

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.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

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

Comment 22

[Steven Michaud [:smichaud] (Retired)]

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 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 800976 [details] [diff] [review]
Fix

Markus, please review my patch's changes to nsNativeThemeCocoa.mm.

Comment 24

[Markus Stange [:mstange]]

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 25

[Neil Deakin]

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?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

(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.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

(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.

Comment 28

[Markus Stange [:mstange]]

(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.

Comment 29

[Markus Stange [:mstange]]

(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.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

>>> 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.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

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

Comment 32

[Neil Deakin]

They are ok.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

What I'll land, once mozilla-inbound reopens.

Carrying forward Neil's r+.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 802565 [details] [diff] [review]
Fix rev1

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a74c28ee4d

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98a74c28ee4d