Last Comment Bug 635691 - GetAttention() flashes the grandparent window although the parent window is in the forground
: GetAttention() flashes the grandparent window although the parent window is i...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla10
Assigned To: Brian R. Bondy [:bbondy]
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks: 693163
  Show dependency treegraph
 
Reported: 2011-02-21 04:23 PST by neil@parkwaycc.co.uk
Modified: 2011-10-10 11:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for GetAttention() flashing when window is in foreground. (1.40 KB, patch)
2011-09-05 12:23 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Alternate Window flashing when it shouldn't fix (4.20 KB, patch)
2011-09-07 09:02 PDT, Brian R. Bondy [:bbondy]
neil: review+
jmathies: feedback+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-02-21 04:23:39 PST
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 neil@parkwaycc.co.uk 2011-02-23 06:38:52 PST
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] 2011-09-04 12:21:38 PDT
Is this still a problem? I can't reproduce on Windows XP SP2. Win 7 as you also mentioned also works.
Comment 3 neil@parkwaycc.co.uk 2011-09-05 02:12:07 PDT
(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] 2011-09-05 04:24:24 PDT
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] 2011-09-05 05:18:52 PDT
Confirmed I can reproduce with the debugger so I'll fix that way.
Comment 6 Brian R. Bondy [:bbondy] 2011-09-05 12:23:38 PDT
Created attachment 558326 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-09-07 03:18:42 PDT
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] 2011-09-07 05:36:44 PDT
> 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 neil@parkwaycc.co.uk 2011-09-07 06:59:27 PDT
(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] 2011-09-07 09:02:10 PDT
Created attachment 558838 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-09-07 13:24:12 PDT
(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] 2011-09-07 13:58:28 PDT
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] 2011-09-29 07:42:29 PDT
Feedback ping
Comment 14 Jim Mathies [:jimm] 2011-10-06 13:28:15 PDT
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] 2011-10-06 13:30:05 PDT
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] 2011-10-06 13:52:48 PDT
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] 2011-10-07 19:00:34 PDT
Try results look good.
Comment 18 neil@parkwaycc.co.uk 2011-10-08 16:38:05 PDT
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] 2011-10-08 20:52:47 PDT
Sounds good, please post I can take it eventually.
Comment 20 Brian R. Bondy [:bbondy] 2011-10-09 07:21:11 PDT
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) 2011-10-10 11:10:34 PDT
https://hg.mozilla.org/mozilla-central/rev/51ef48b74453

Note You need to log in before you can comment on or make changes to this bug.