635691 - GetAttention() flashes the grandparent window although the parent window is in the forground

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. On the search bar, drop down the list of search engines and select Manage Search Engines...
2. In the search engine dialog, change the keyword of one of the engines

Actual result: prompt flashes taskbar briefly

This is because the prompting code calls GetAttention, but the widget code doesn't realise that the prompting window's parent is in the foreground.

There are two approaches we can take here; the first is that when we locate the ancestor window to flash that we check it against the foreground window; the second is that we locate the ancestor window of the foreground window too.

Comment 1

[[e-mail]]

Confirmed that this is not a problem on Vista/7 because they delay the initial flash so that the window appears and cancels the flash before it can start.

Comment 2

[Brian R. Bondy [:bbondy]]

Is this still a problem? I can't reproduce on Windows XP SP2. Win 7 as you also mentioned also works.

Comment 3

[[e-mail]]

(In reply to Brian R. Bondy from comment #2)
> Is this still a problem? I can't reproduce on Windows XP SP2.
I said briefly, so you may have to watch the taskbar while clicking the button ;-)

Comment 4

[Brian R. Bondy [:bbondy]]

I guess you can see more frames per second than I can with my measly human eyes, or you can stop time :)

But I can stop time too with a debugger, so I'll check that way :)

Comment 5

[Brian R. Bondy [:bbondy]]

Confirmed I can reproduce with the debugger so I'll fix that way.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch for GetAttention() flashing when window is in foreground.

Before path: Check if the topmost owner is the foreground window.
After patch: Check if any owner is the foreground window.

Let me know if you think maybe using parent is a better way instead of owner. Or if you have a different idea.

Comment 7

[[e-mail]]

Comment on attachment 558326 [details] [diff] [review]
Patch for GetAttention() flashing when window is in foreground.

>   // Don't flash if the flash count is 0 or if the
>   // top level window is already active.
>   HWND fgWnd = ::GetForegroundWindow();
>   if (aCycleCount == 0 || fgWnd == GetTopLevelHWND(mWnd))
>     return NS_OK;

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

>+  HWND flashWnd;
>+  HWND ownerWnd = mWnd;
>+  // Don't flash if this window, or any owner is the foreground window.
>+  do {
>     flashWnd = ownerWnd;
>+    if (fgWnd == flashWnd)
>+      return NS_OK;
>+
>+  } while (ownerWnd = ::GetWindow(flashWnd, GW_OWNER));
I'm not sure why you changed the while loop into a do loop. In particular, this window was already checked along with aCycleCount. (I don't know whether it's within the scope of this bug, but maybe flashWnd should be GetTopLevelHWND(mWnd), but that would involve a bit of rewriting...)

Comment 8

[Brian R. Bondy [:bbondy]]

> I'm not sure why you changed the while loop into a do loop

The intent was to get rid of 2 if conditions instead of 1. The fgWnd == GetTopLevelHWND(mWnd) was meant to be removed since it was already checked below in the do/while loop.

I'll do an alternate patch.  I think maybe we can use just one check if we do as you said with flashWnd as GetTopLevelHWND(mWnd).  I'll try now.

Comment 9

[[e-mail]]

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> > I'm not sure why you changed the while loop into a do loop
> The intent was to get rid of 2 if conditions instead of 1. The fgWnd ==
> GetTopLevelHWND(mWnd) was meant to be removed since it was already checked
> below in the do/while loop.
Fair enough.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Alternate Window flashing when it shouldn't fix

It turns out that GetTopLevelHWND's aStopOnDialogOrPopup flag didn't actually do what it said. 
I renamed it to what it does instead. 

Our dialogs don't have the popup style so we were breaking out early when that flag was set. 
Our dialogs don't have icons so I think it is correct handling to have the flash windows as the parent owner that does have one.
If instead you flash a dialog it will make the dialog window flash but no taskbar icon flash.

Comment 11

[[e-mail]]

(In reply to Brian R. Bondy from comment #10)
> It turns out that GetTopLevelHWND's aStopOnDialogOrPopup flag didn't
> actually do what it said. 
> I renamed it to what it does instead.
I had a look at the other callers. Most already pass PR_TRUE and so definitely don't crawl owner windows. Some callers are just interested in knowing whether or not they are a child window. Other callers don't want an owner window and are relying on the bug. So, it might make more sense to change the default for the parameter to true, fix the code to properly crawl owner windows when the parameter is false, and explcitly pass false from the flash window code. Jimm?

Comment 12

[Brian R. Bondy [:bbondy]]

Ya there are a couple cases where we use the default of PR_FALSE that I am not sure of so I thought it was best to not cause any regressions and only change the one we know is a bug.   Will wait for feedback on if the others should be changed as well or not.

Comment 13

[Brian R. Bondy [:bbondy]]

Feedback ping

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 558838 [details] [diff] [review]
Alternate Window flashing when it shouldn't fix

Hmm, so I don't see anything obviously wrong with this. This is however very touchy code, so I would keep an eye out for regressions after it lands.

Comment 15

[Brian R. Bondy [:bbondy]]

Neil let me know if you want me to change it, I'd prefer to keep it as is in the patch.

Comment 16

[Brian R. Bondy [:bbondy]]

Rebased and pushed to try in case the patch as is gets a positive review from Neil. Jim mentioned it's touchy code so the way in the patch is safer.

Try results:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=62322be294ee

Comment 17

[Brian R. Bondy [:bbondy]]

Try results look good.

Comment 18

[[e-mail]]

Comment on attachment 558838 [details] [diff] [review]
Alternate Window flashing when it shouldn't fix

So, this works, but I feel I should file a bug to investigate what the callers of GetTopLevelHWND actually want.

Comment 19

[Brian R. Bondy [:bbondy]]

Sounds good, please post I can take it eventually.

Comment 20

[Brian R. Bondy [:bbondy]]

I posted the follow up task in bug 693163.

Pushed this task to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/51ef48b74453

Comment 21

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/51ef48b74453