Closed Bug 313403 Opened 15 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: shayne.anthony.jewers, Assigned: emaijala+moz)

References

()

Details

Attachments

(1 file, 5 obsolete files)

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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looks like you can get the appropriate blink time using
SystemParametersInfo(SPI_GETFOREGROUNDFLASHCOUNT), but not in Win95 or WinNt
(dwMajorVersion >= 5 || dwMinorVersion >= 10).
Summary: Windows Flash Forever instead of The User-Specified Amount of times → nsWindow::GetAttention should obey the system set FOREGROUNDFLASHCOUNT if aCycleCount = -1 (background dialogs flash in the taskbar forever, should only flash <user-specified> times)
Flags: blocking1.8.1?
Version: Trunk → 1.8 Branch
Not going to block 1.8.1 for this.
Flags: blocking1.8.1? → blocking1.8.1-
Component: Widget: Win32 → XP Toolkit/Widgets: Menus
Version: 1.8 Branch → Trunk
Assignee: win32 → emaijala
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #332086 - Flags: review?(timeless)
Status: NEW → ASSIGNED
Attachment #332086 - Flags: review?(timeless) → review?(timeless)
Attachment #332086 - Flags: review?(timeless) → review+
Attachment #332086 - Flags: superreview?(roc)
Attachment #332086 - Flags: superreview?(roc) → superreview+
pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/7a55d41f8ca2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v1.1 (obsolete) — Splinter Review
Fixed typo in the first patch. Transferring reviews.
Attachment #332086 - Attachment is obsolete: true
Attachment #334547 - Flags: superreview+
Attachment #334547 - Flags: review+
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Whiteboard: Checkin needed after alpha2
Pushed as 17104:4460de8869d8.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: Checkin needed after alpha2
Target Milestone: --- → mozilla1.9.1a2
Component: XP Toolkit/Widgets: Menus → Widget: Win32
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.
Indeed, sucky.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out to avoid getting it into 1.9.1a2
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).
Attachment #334547 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #335935 - Flags: review?(neil)
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?
(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.
Status: REOPENED → ASSIGNED
(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.
(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...
Attachment #335935 - Attachment is obsolete: true
Attachment #335935 - Flags: review?(neil)
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.
Attached patch Patch v1.3 (obsolete) — Splinter Review
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.
Attachment #368772 - Flags: review?(neil)
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 :-(
Attachment #368772 - Flags: review?(neil) → review-
Attachment #335935 - Attachment is obsolete: false
Attachment #335935 - Flags: review+
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.
Attachment #335935 - Attachment is obsolete: true
Attachment #335935 - Flags: review+
Attached patch Patch v1.2 renewed (obsolete) — Splinter Review
Ok, this is the previous patch renewed.
Attachment #368772 - Attachment is obsolete: true
Attachment #369331 - Flags: review?(neil)
Ok, this is the previous patch renewed.
Attachment #369331 - Attachment is obsolete: true
Attachment #369339 - Flags: review?(neil)
Attachment #369331 - Flags: review?(neil)
Attachment #369339 - Flags: review?(neil) → review+
Attachment #369339 - Flags: superreview?(roc)
Attachment #369339 - Flags: superreview?(roc) → superreview+
Comment on attachment 369339 [details] [diff] [review]
Patch v1.2 renewed

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

{} around the statement
Pushed e783d41a4e4a to mozilla-central with roc's comment noted.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
I ifndef'd the flashwindow stuff from WinCE to fix bustage.
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.
You need to log in before you can comment on or make changes to this bug.