313403 - nsWindow::GetAttention should obey the system set FOREGROUNDFLASHCOUNT if aCycleCount = -1 (background dialogs flash in the taskbar forever, should only flash <user-specified> times)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Shayne Jewers]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5

When FF, TB or XR wants to get my attention with window.getAttention(), it
flashes on the windows taskbar instead of my specified 1 flash

Reproducible: Always

Steps to Reproduce:
1.Using TweakUI, in Generak>focus, set "Flash Taskbar button" to 1
2.get FF, TB or XR to get your attention (eg. "Do you want to set FF as your
default broswer?" box
3.Not the taskbar

Actual Results:  
The Taskbar flashes forever

Expected Results:  
It should have flashed once, like I set in TweakUI

Comment 1

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

Even better would be to use FlashWindowEx for the win32 impl of getAttention
instead of FlashWindow, eliminating the need to do timer stuff ourselves.
According to
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/debug/base/flashwindowex.asp,
 FlashWindowEx is supported in everything >= Windows 98, so unless Windows 95
support is a problem this could simplify code quite a bit (eliminate the need
for nsAttentionTimerMonitor).

Using the user-specified pref would also be easier, but I haven't looked into
how to get that value.

Comment 2

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

Looks like you can get the appropriate blink time using
SystemParametersInfo(SPI_GETFOREGROUNDFLASHCOUNT), but not in Win95 or WinNt
(dwMajorVersion >= 5 || dwMinorVersion >= 10).

Comment 3

[Darin Fisher]

Not going to block 1.8.1 for this.

Comment 4

[Ere Maijala (slow)]

Attachment: Patch v1

Comment 5

[Benjamin Smedberg]

pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/7a55d41f8ca2

Comment 6

[Benjamin Smedberg]

Backed out due to build bustage:
c:/builds/slave/trunk_win2k3_hw/build/widget/src/windows/nsWindow.cpp(7353) : error C2446: '==' : no conversion from 'HWND' to 'HWND (__stdcall *)(void)'
        There is no context in which this conversion is possible
c:/builds/slave/trunk_win2k3_hw/build/widget/src/windows/nsWindow.cpp(7353) : error C2040: '==' : 'HWND (__stdcall *)(void)' differs in levels of indirection from 'HWND'

Comment 7

[Ere Maijala (slow)]

Attachment: Patch v1.1

Fixed typo in the first patch. Transferring reviews.

Comment 8

[Reed Loden [:reed] (use needinfo?)]

Pushed as 17104:4460de8869d8.

Comment 9

[7842.9]

In the latest nightly build, with this patch, alert boxes now flash the firefox taskbar item even if it is in the foreground.

In the previous nightly this doesn't happen.

Comment 10

[Ere Maijala (slow)]

Indeed, sucky.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Backed out to avoid getting it into 1.9.1a2

Comment 12

[Ere Maijala (slow)]

Comment on attachment 334547 [details] [diff] [review]
Patch v1.1

Of course, to make this work properly the flashing needs to be canceled when focus is obtained. To do that we need to fix how dialogs are displayed because they think they have focus right away (with flashing disabled they even look like that).

Comment 13

[Ere Maijala (slow)]

Attachment: Patch v1.2

A new try. Adding a check so we don't flash if the owner is the foreground window and stop flashing upon activation. Plus don't try to activate dialogs if that's not allowed.

Comment 14

[[e-mail]]

Comment on attachment 335935 [details] [diff] [review]
Patch v1.2

Does the GetAttention(2); call inside nsWindow::Show need to be changed to -1?

>-        if (!sIsInEndSession)
>-          ::ShowWindow(mWnd, SW_HIDE);
>+        if (!sIsInEndSession)
>+          ::ShowWindow(mWnd, SW_HIDE);
Not sure what happened here?

>+          StopFlashing();
Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it will unfortunately still try to flash the active window once.)

>+  FLASHWINFO flashInfo;
>+  ZeroMemory(&flashInfo, sizeof(FLASHWINFO));
>+  flashInfo.cbSize = sizeof(FLASHWINFO);
>+  flashInfo.hwnd = flashWnd;
>+  flashInfo.dwFlags = FLASHW_ALL;
>+  flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount;
Since you're setting nearly all the members anyway wouldn't an initialiser be more efficient?

Comment 15

[Ere Maijala (slow)]

(In reply to comment #14)
> (From update of attachment 335935 [details] [diff] [review])
> Does the GetAttention(2); call inside nsWindow::Show need to be changed to -1?

I'd say no. We just want a quick note that the window didn't come to foreground and if the system parameter is long it would be quite annoying.
 
> >-        if (!sIsInEndSession)
> >-          ::ShowWindow(mWnd, SW_HIDE);
> >+        if (!sIsInEndSession)
> >+          ::ShowWindow(mWnd, SW_HIDE);
> Not sure what happened here?

Leftover from another patch though I don't know how that got there with no real change..

> >+          StopFlashing();
> Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it
> will unfortunately still try to flash the active window once.)

That would have been nice, but as we flash the owner, that wouldn't be the foreground window. 

> >+  FLASHWINFO flashInfo;
> >+  ZeroMemory(&flashInfo, sizeof(FLASHWINFO));
> >+  flashInfo.cbSize = sizeof(FLASHWINFO);
> >+  flashInfo.hwnd = flashWnd;
> >+  flashInfo.dwFlags = FLASHW_ALL;
> >+  flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount;
> Since you're setting nearly all the members anyway wouldn't an initialiser be
> more efficient?

Well, I believe in ZeroMemory just to make sure the struct is properly initialized even if it gets new members in a future version.

Comment 16

[[e-mail]]

(In reply to comment #15)
>(In reply to comment #14)
>>>+          StopFlashing();
>>Can you not use FLASHW_TIMERNOFG to avoid this? (It's not a panacea because it
>> will unfortunately still try to flash the active window once.)
>That would have been nice, but as we flash the owner, that wouldn't be the
>foreground window.
I guess it must be an undocumented feature of FLASHW_TIMERNOFG that it works even when the flashed window is the owner of the foreground window.

>>>+  FLASHWINFO flashInfo;
>>>+  ZeroMemory(&flashInfo, sizeof(FLASHWINFO));
>>>+  flashInfo.cbSize = sizeof(FLASHWINFO);
>>>+  flashInfo.hwnd = flashWnd;
>>>+  flashInfo.dwFlags = FLASHW_ALL;
>>>+  flashInfo.uCount = aCycleCount > 0 ? aCycleCount : defaultCycleCount;
>>Since you're setting nearly all the members anyway wouldn't an initialiser be
>>more efficient?
>Well, I believe in ZeroMemory just to make sure the struct is properly
>initialized even if it gets new members in a future version.
And I believe in C initialiser semantics to make sure the struct is properly initialised even if it gets new members in a future version.

Comment 17

[Ere Maijala (slow)]

(In reply to comment #16)
> I guess it must be an undocumented feature of FLASHW_TIMERNOFG that it works
> even when the flashed window is the owner of the foreground window.

Could be that I confused it with the forementioned single flash when testing. Will try again.

> And I believe in C initialiser semantics to make sure the struct is properly
> initialised even if it gets new members in a future version.

So I failed the semantics awareness test...

Comment 18

[Ere Maijala (slow)]

FLASHW_TIMERNOFG seems to work fine. One remaining problem is that the first time an alert is displayed it will bring the window to foreground no matter what. Still need to find out the reason for this.

Comment 19

[Ere Maijala (slow)]

Attachment: Patch v1.3

Finally... The foreground change first time a dialog was displayed seems to be some kind of a debugger interaction, as it doesn't happen outside Visual Studio. And even there it doesn't steal focus.

Comment 20

[[e-mail]]

Comment on attachment 368772 [details] [diff] [review]
Patch v1.3

Sorry to mess you around like that, but it seems that TIMERNOFG refers to the application, not the window :-(

Comment 21

[Ere Maijala (slow)]

Comment on attachment 335935 [details] [diff] [review]
Patch v1.2

Oh, no worries. I didn't notice that either. Annoying, but I'll update the previous patch to actually apply and change the struct initialization while at it.

Comment 22

[Ere Maijala (slow)]

Attachment: Patch v1.2 renewed

Ok, this is the previous patch renewed.

Comment 23

[Ere Maijala (slow)]

Attachment: Patch v1.2 renewed

Ok, this is the previous patch renewed.

Comment 24

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

Comment on attachment 369339 [details] [diff] [review]
Patch v1.2 renewed

+  while (HWND ownerWnd = ::GetWindow(flashWnd, GW_OWNER))
+    flashWnd = ownerWnd;

{} around the statement

Comment 25

[Ere Maijala (slow)]

Pushed e783d41a4e4a to mozilla-central with roc's comment noted.

Comment 26

[Ere Maijala (slow)]

I ifndef'd the flashwindow stuff from WinCE to fix bustage.

Comment 27

[[e-mail]]

Comment on attachment 369339 [details] [diff] [review]
Patch v1.2 renewed

>+  HWND flashWnd = mWnd;
>+  while (HWND ownerWnd = ::GetWindow(flashWnd, GW_OWNER))
>+    flashWnd = ownerWnd;
>+
>+  // Don't flash if the owner window is active either.
>+  if (fgWnd == flashWnd)
>+    return NS_OK;
The old code may have had this bug too, but it's easier to see like this ;-)

If the flash window is the parent of the foreground window which is the parent of the attention window, then I don't think we want to flash. I'm not so sure about the case where the attention window and the foreground window have the same owner.