Closed Bug 953146 Opened 11 years ago Closed 10 years ago

Shouldn't allow other applications to set focus to popup window

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(8 files, 5 obsolete files)

part.1 Clean up nsWindow::DealWithPopups()
11.50 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.2 Remove unnecessary code from nsWindow::DealWithPopups()
1.61 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.3 nsWindow::DealWithPopups() should decide if popup should be rolled up in each case of messages
5.63 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.4 Separate counting pupups to rollup from nsWindow::DealWithPopups()
6.50 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.5 The switch statement of nsWindow::DealWithPopups() should decide if the popup should be rolled up completely
5.42 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.1 Clean up nsWindow::DealWithPopups() (-w)
10.19 KB, patch
Details | Diff | Splinter Review
part.6 Clean up WM_MOUSEACTIVE case in nsWindow::ProcessMessage()
3.32 KB, patch
jimm
: review+
Details | Diff | Splinter Review
part.7 Don't allow other application to activate non-focusable popup
7.07 KB, patch
jimm
: review+
Details | Diff | Splinter Review 
This must be the cause of closing dropdown of combo box with touchpad of Elantech and Synaptics.

Such buggy utilities try to set focus under mouse cursor at two finger swipe. However, this causes deactivate event on the logical parent window and also closing the popup.

I'm thinking that nsWindow shouldn't dispatch deactivate event on the window and restore the focus.
Will Mozilla internal focus state be inconsistent with the system state?
I'm not sure what we can do, what we cannot do. I'm trying to look for safer way as far as possible. If you have some idea, let me know.
testcase for checking the DOM focus/blur events.
URL: data:text/html,<select><option>1<opti...
What other browsers are doing? Or are those broken drivers sniffing our window and setting focus only on our window?
Before fixing this bug, let's sort out nsWindow::DealWithPopup() because it's very complicated and looks like a lot of small bugs.

This patch doesn't change any behavior. Just cleans up the code.
Attachment #8351501 - Flags: review?(jmathies)
There is some code which checks if the message is WM_MOUSEACTIVATE in the else block of |if (nativeMessage == WM_MOUSEACTIVATE)|. So, we can just drop them.
Attachment #8351502 - Flags: review?(jmathies)
Currently, all messages which may cause rolling up popup are handled in same block. This is the worst point of this complicated code. Let's decide |rollup| value in each case block. This helps later patches.
Attachment #8351503 - Flags: review?(jmathies)
This patch separates the block for nested XUL popups. This change is necessary for next patch.
Attachment #8351504 - Flags: review?(jmathies)
This patch get rid of |rollup|. That means that below of the switch statement just executes to roll up popups. In other words, switch statement quits immediately if it believes that any popups shouldn't be rolled up.
Attachment #8351505 - Flags: review?(jmathies)
In nsWindow::ProcessMessage(), WM_MOUSEACTIVATE case handles only a case while there are popups. This code should be in DealWithPopups().
Attachment #8351506 - Flags: review?(jmathies)
This is the actual patch of this bug. This works well but the non-client area flicks by swapping focus between normal window and popup window. I'm still researching a way to prevent it.

If somebody has idea for this, let me know.
oops, forgot to qrefresh.
Attachment #8351507 - Attachment is obsolete: true
FYI: This is a diff file with -w of part.1. Use this if you need.
FYI: I'm thinking that part.1 - part.6 should be landed first for easier to check regressions by the clean up. After that, part.7 should be landed.
(In reply to Masatoshi Kimura [:emk] from comment #4)
> What other browsers are doing? Or are those broken drivers sniffing our
> window and setting focus only on our window?

On my ZENBOOK whose touchpad is Elantech, only IE doesn't close the popup of dropdown. However, the parent window loses focus (non-client area becomes inactive). However, Chrome also closes the popup.

On my LIFEBOOK whose touchpad is Synaptics, only Firefox has this bug. In the registry, there is only custom keys for firefox.exe :-(
Doesn't SetWindowsHookEx/WH_CBT/HCBT_ACTIVATE work?
This bug is not reproducible on my dynabook KIRA in spite of the Synaptics touchpad driver, so I couldn't test it myself :(
(In reply to Masatoshi Kimura [:emk] from comment #16)
> Doesn't SetWindowsHookEx/WH_CBT/HCBT_ACTIVATE work?
> This bug is not reproducible on my dynabook KIRA in spite of the Synaptics
> touchpad driver, so I couldn't test it myself :(

Thank you for the information. But it's odd, I don't see the notifications for popup windows even if I don't call unhook API...
How about WS_EX_NOACTIVATE?
This patch works perfectly on my LIFEBOOK (Synaptics, Win8.1). I'll test this on my ZENBOOK (Elantech, Win8). And I'll confirm on WinXP, Vista and 7 without touchpad.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1c95e758a10b
Attachment #8351509 - Attachment is obsolete: true
(In reply to Masatoshi Kimura [:emk] from comment #18)
> How about WS_EX_NOACTIVATE?

Although, I don't test it, according to the MSDN, it can be activated with SetActiveWindow() and SetForegroundWindow(). So, the approach depends on the mouse utils.
And I found a crash bug related to such mouse driver.

XUL's panel isn't closed by scroll operation over it. However, the focus is confused. Then, clicking on tab bar of another window, firefox crashes.

https://crash-stats.mozilla.com/report/index/bp-d739fed7-819c-4900-bee3-ab8e12131227
https://crash-stats.mozilla.com/report/index/bp-02f1c6ba-6114-4fd8-84b6-d8e552131227

It seems that this has gone with the part.7. Perhaps, the reason is that it fixes the focus confusion.
Comment on attachment 8351545 [details] [diff] [review]
part.7 Don't allow other application to activate popup panel

I don't see any problems on WinXP, Vista, 7, 8, and 8.1.

I only tested the drivers which have the problem only on Win 8 and 8.1 because it's cannot be tested on VM.

This patch makes that when popup gets focus except by mouse operation, restores focus to previous window (the first block of WM_ACTIVATE case).

However, it causes flickering nonclient area. Therefore, this patch handles WM_NCACTIVATE for ignoring it. If focus is moved to non-popup window and window is deactivaging (else if block of WM_ACTIVATE case), the ignored WM_NCACTIVATE is set manually. This fixes the nonclient area state with actual state.

Additionally, SetForegroundWidnow() always fails to change foreground window at receiving WM_ACTIVATE. Therefore, this uses a hack with new message MOZ_WM_REACTIVATE.

This patch works with Elantech's touchpad and Synaptics touchpad which have this bug.
Attachment #8351545 - Flags: review?(jmathies)
These patches break any panel with noautohide="true". Mouse clicks are ignored on them. For example:

<button label="Open" type="panel">
  <panel id="panelnormal" noautohide="true" titlebar="normal">
    <button label="Normal Close" oncommand="this.parentNode.hidePopup()"/>
    <textbox/>
    <iframe type="content" src="http://www.mozilla.org" width="300" height="300"/>
  </panel>
</button>
Comment on attachment 8351545 [details] [diff] [review]
part.7 Don't allow other application to activate popup panel

Sure.
Attachment #8351545 - Flags: review?(jmathies)
I think that IsFocusablePopup() in this patch isn't correct for your example, isn't it?

Do you have any idea for fixing this in nsWindow level? Or do I need to add new method to nsIRollupListener such as |IsFocusablePopup()|?
Attachment #8351545 - Attachment is obsolete: true
Attachment #8351590 - Flags: feedback?(enndeakin)
Flags: needinfo?(enndeakin)
Oops, in WA_INACTIVE, it needs to check if the window is popup.
Oh, is the focusable popup always owned window?
Comment on attachment 8351506 [details] [diff] [review]
part.6 WM_MOUSEACTIVE should be handled in nsWindow::DealWithPopups() completely

Hmm, this patch breaks the focusable panel behavior. part.1 - part.5 are fine.
Attachment #8351506 - Flags: review?(jmathies) → review-
Flags: needinfo?(enndeakin)
Comment on attachment 8351506 [details] [diff] [review]
part.6 WM_MOUSEACTIVE should be handled in nsWindow::DealWithPopups() completely

Ah, I misunderstood. The part.6 isn't necessary. The message should be handled even if DealWithPopup() returns before the switch statement.
Attachment #8351506 - Attachment is obsolete: true
Comment on attachment 8351599 [details] [diff] [review]
part.6 Clean up WM_MOUSEACTIVE case in nsWindow::ProcessMessage()

This patch makes part.7 easier to read.
Attachment #8351599 - Flags: review?(jmathies)
Comment on attachment 8351590 [details] [diff] [review]
part.7 Don't allow other application to activate popup panel (WIP)

Okay, I misunderstood. part.7 doesn't have the problem. The bug was caused by the old part.6 patch.

I'll attach new part.7 too.
Attachment #8351590 - Flags: feedback?(enndeakin)
This patch cannot fix one issue, that is, nonclient area of owner window of non-auto-hide panel is flickered at trying to scroll on panel with problematic mouse drivers. However, there is no such UI on our products and this is really the mouse drivers' issue, not ours. So, we don't need to spend long time for fixing such edge issue.
Attachment #8351590 - Attachment is obsolete: true
Attachment #8351636 - Flags: review?(jmathies)
Do you have a log of the windows messages that are sent in a build without these patches when this bug occurs?

If this a bug with a specific mouse driver, can we contact its manufacturer?
Attachment #8351501 - Flags: review?(jmathies) → review+
(In reply to Neil Deakin from comment #35)
> Do you have a log of the windows messages that are sent in a build without these patches when this bug occurs?

With Synaptics driver on my LIFEBOOK:

Click the window for activating.
> <00001> 001310EE S WM_MOUSEACTIVATE hwndTopLevel:001310EE nHittest:HTCLIENT uMsg:WM_LBUTTONDOWN
> <00002> 001310EE R WM_MOUSEACTIVATE fuActivate:MA_ACTIVATE
> <00003> 001310EE S WM_NCACTIVATE fActive:True
> <00004> 001310EE R WM_NCACTIVATE
> <00005> 001310EE S WM_ACTIVATE fActive:WA_CLICKACTIVE fMinimized:False hwndPrevious:(null)
> <00006> 001310EE S WM_NCACTIVATE fActive:True
> <00007> 001310EE R WM_NCACTIVATE
> <00008> 001310EE S WM_SETFOCUS hwndLoseFocus:(null)
> <00009> 001310EE R WM_SETFOCUS
> <00010> 001310EE R WM_ACTIVATE

Open dropdown by click.
> <00011> 0062111C S WM_CREATE lpcs:008FDE80
> <00012> 0062111C R WM_CREATE fContinue:0 (continue creation)

Try to scroll by 2 finger swipe.
> <00013> 0062111C S WM_MOUSEACTIVATE hwndTopLevel:0062111C nHittest:HTCLIENT uMsg:WM_NULL
> <00014> 0062111C R WM_MOUSEACTIVATE fuActivate:MA_NOACTIVATE
> <00015> 001310EE S WM_NCACTIVATE fActive:False
> <00016> 001310EE R WM_NCACTIVATE fDeactivateOK:True
> <00017> 001310EE S WM_ACTIVATE fActive:WA_INACTIVE fMinimized:False hwndPrevious:0062111C
> <00018> 001310EE R WM_ACTIVATE
> <00019> 0062111C S WM_NCACTIVATE fActive:True
> <00020> 0062111C R WM_NCACTIVATE
> <00021> 0062111C S WM_ACTIVATE fActive:WA_ACTIVE fMinimized:False hwndPrevious:001310EE
> <00022> 0062111C S WM_NCACTIVATE fActive:True
> <00023> 0062111C R WM_NCACTIVATE
> <00024> 001310EE S WM_KILLFOCUS hwndGetFocus:0062111C
> <00025> 0062111C S WM_SETFOCUS hwndLoseFocus:001310EE
> <00026> 0062111C P WM_MOUSEWHEEL fwKeys:0000 zDelta:-72 xPos:1121 yPos:497
> <00027> 0062111C P WM_MOUSEWHEEL fwKeys:0000 zDelta:-168 xPos:1121 yPos:497
> <00028> 0062111C S WM_NCACTIVATE fActive:False
> <00029> 0062111C R WM_NCACTIVATE fDeactivateOK:True
> <00030> 0062111C S WM_ACTIVATE fActive:WA_INACTIVE fMinimized:False hwndPrevious:001310EE
> <00031> 0062111C R WM_ACTIVATE
> <00032> 001310EE S WM_NCACTIVATE fActive:True
> <00033> 001310EE R WM_NCACTIVATE
> <00034> 001310EE S WM_ACTIVATE fActive:WA_ACTIVE fMinimized:False hwndPrevious:0062111C
> <00035> 001310EE S WM_NCACTIVATE fActive:True
> <00036> 001310EE R WM_NCACTIVATE
> <00037> 0062111C S WM_KILLFOCUS hwndGetFocus:001310EE
> <00038> 0062111C R WM_KILLFOCUS
> <00039> 001310EE S WM_SETFOCUS hwndLoseFocus:0062111C
> <00040> 001310EE R WM_SETFOCUS
> <00041> 001310EE R WM_ACTIVATE
> <00042> 0062111C S WM_DESTROY
> <00043> 0062111C R WM_DESTROY
> <00044> 001310EE P WM_MOUSEWHEEL fwKeys:0000 zDelta:-24 xPos:1121 yPos:497
> <00045> 001310EE P WM_MOUSEWHEEL fwKeys:0000 zDelta:-24 xPos:1121 yPos:497

> If this a bug with a specific mouse driver, can we contact its manufacturer?

I'm not sure. Both Elantech and Synaptcis are OEM vendor. So, their site only tell us business contact.

Additionally, there is no guarantee the fix of their drivers are updated by all PC vendor. And probably, it's not. So, for our users, we should avoid this issue if we can do it even if we could contact the vendors and they'd fix this.
Attachment #8351502 - Flags: review?(jmathies) → review+
Comment on attachment 8351503 [details] [diff] [review]
part.3 nsWindow::DealWithPopups() should decide if popup should be rolled up in each case of messages

Review of attachment 8351503 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +7327,2 @@
>      case WM_MOUSEACTIVATE:
> +      // XXX Why do we need to check the message pos?

You've added these comments in three places within the break. Is this an open ended question or can we remove the check?
Attachment #8351503 - Flags: review?(jmathies) → review+
Attachment #8351504 - Flags: review?(jmathies) → review+
Attachment #8351505 - Flags: review?(jmathies) → review+
Comment on attachment 8351599 [details] [diff] [review]
part.6 Clean up WM_MOUSEACTIVE case in nsWindow::ProcessMessage()

Review of attachment 8351599 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +5180,5 @@
>          }
>        }
>        break;
>        
>      case WM_MOUSEACTIVATE:

nit - please clear up the white space above this while we're in here.

::: widget/windows/nsWindow.h
@@ +288,5 @@
>    void                    PickerClosed();
>  
>    bool                    const DestroyCalled() { return mDestroyCalled; }
>  
> +  HWND                    GetOwnerWnd() const

Do these need to be public? (There's a protected section farther down for 'Window utilities'.)
Attachment #8351599 - Flags: review?(jmathies) → review+
Comment on attachment 8351636 [details] [diff] [review]
part.7 Don't allow other application to activate non-focusable popup

Review of attachment 8351636 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindowDefs.h
@@ +39,4 @@
>  
>  // Internal message for ensuring the file picker is visible on multi monitor
>  // systems, and when the screen resolution changes.
> +#define MOZ_WM_ENSUREVISIBLE              (WM_APP+0x374F)

I think we can clean this up a bit by making these successive values from the wm_app values above (0x0314 and 0x0315)
Attachment #8351636 - Flags: review?(jmathies) → review+
Neil, would you like to do an sr on this? I haven't worked with the popup rollup code that much. Generally logic looks sound.
Flags: needinfo?(enndeakin)
(In reply to Jim Mathies [:jimm] from comment #37)
> Comment on attachment 8351503 [details] [diff] [review]
> part.3 nsWindow::DealWithPopups() should decide if popup should be rolled up
> in each case of messages
> 
> Review of attachment 8351503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +7327,2 @@
> >      case WM_MOUSEACTIVATE:
> > +      // XXX Why do we need to check the message pos?
> 
> You've added these comments in three places within the break. Is this an
> open ended question or can we remove the check?

I believe that they are not necessary. So, yes, I think that we should remove them. However, I don't like to do it here. I'd like to do it in follow up bug for making regression-window clear.
(In reply to Jim Mathies [:jimm] from comment #40)
> Neil, would you like to do an sr on this? I haven't worked with the popup
> rollup code that much. Generally logic looks sound.

Hmm, I'm still waiting the reply... but if Enn won't reply for this, I'll land it for Nightly testers. Then, Enn will complain about something, I'll fix them in follow up bugs.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c354b68a0ca

landed the patch.

Enn, please file followup bugs if you find some problems in the patch.
Whiteboard: [leave open]
(In reply to Jim Mathies [:jimm] from comment #39)
> Comment on attachment 8351636 [details] [diff] [review]
> part.7 Don't allow other application to activate non-focusable popup
> 
> Review of attachment 8351636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindowDefs.h
> @@ +39,4 @@
> >  
> >  // Internal message for ensuring the file picker is visible on multi monitor
> >  // systems, and when the screen resolution changes.
> > +#define MOZ_WM_ENSUREVISIBLE              (WM_APP+0x374F)
> 
> I think we can clean this up a bit by making these successive values from
> the wm_app values above (0x0314 and 0x0315)

I only changed the new message. I'm not sure why MOZ_WM_ENSUREVISIBLE uses such unique value. We should investigate the reason if we'd change it.
https://hg.mozilla.org/mozilla-central/rev/3c354b68a0ca
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Was it intentional that the response to WM_ACTIVATEAPP now no longer calls the code in GetPopupsToRollup?
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #48)
> Was it intentional that the response to WM_ACTIVATEAPP now no longer calls
> the code in GetPopupsToRollup?

Yes. See part.4. The old code checks if the message is NOT WM_ACTIVATEAPP.

7356  uint32_t popupsToRollup = UINT32_MAX;
7357  if (rollup && nativeMessage != WM_ACTIVATEAPP) {
7358    nsAutoTArray<nsIWidget*, 5> widgetChain;
7359    uint32_t sameTypeCount =
7360      rollupListener->GetSubmenuWidgetChain(&widgetChain);
Yes, but you added that yourself in part 3. The original code doesn't have that check.

http://hg.mozilla.org/mozilla-central/annotate/20bc4777d26a/widget/windows/nsWindow.cpp
(In reply to Neil Deakin from comment #50)
> Yes, but you added that yourself in part 3. The original code doesn't have
> that check.
> 
> http://hg.mozilla.org/mozilla-central/annotate/20bc4777d26a/widget/windows/
> nsWindow.cpp

Ah, I forgot adding the shortcut. The reason is that EventIsInsideWindow() always returns false at handling WM_ACTIVATEAPP.
http://hg.mozilla.org/mozilla-central/annotate/20bc4777d26a/widget/windows/nsWindow.cpp#l7267

Therefore, the loop always does nothing at handling WM_ACTIVATEAPP.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: