1174798 - [e10s] nsGlobalWindow::SetKeyboardIndicators should propagate value to child process windows

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Neil Deakin]

On Windows, this will get called when the indicator of whether focus rings should be rendered changes in response to the WM_UPDATEUISTATE message. It can change when someone updates the control panel setting, but it is also needed to set the initial state when a window first opens.

Should be fairly simple to fix:

- Within nsGlobalWindow::SetKeyboardIndicators, as well as looping over the child docshells, call nsContentUtils::CallOnAllRemoteChildren to iterate over the remote process children, passing the two flags.
- Add a function in the child process (in TabChild.cpp) which calls nsGlobalWindow::SetKeyboardIndicators with the same two flags.


Very minor polish though.

Comment 1

[Neil Deakin]

This is causing erroneous focus indicators to appear in content pages on Windows and bug 1263330 made this more noticeable. The issue is that the content pages get to see the WM_UPDATEUISTATE message which Windows sends to a window when it opens to disable focus rings by default. Let's fix this.

Comment 2

[Neil Deakin]

Attachment: Initialize keyboard indicators in child processes

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey Neil, did you mean to set a reviewer for your patch, or were you still working on it?

Comment 6

[Neil Deakin]

Attachment: Initialize keyboard indicators in child processes, v2

This patch initializes the focus rings in child process windows, and propagates the keyboard state flags when they change.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757299 [details] [diff] [review]
Initialize keyboard indicators in child processes, v2


>@@ -550,16 +551,19 @@ parent:
>     async InvokeDragSession(IPCDataTransfer[] transfers, uint32_t action,
>                             nsCString visualData, uint32_t width, uint32_t height,
>                             uint32_t stride, uint8_t format,
>                             int32_t dragAreaX, int32_t dragAreaY);
> 
>     async AudioChannelActivityNotification(uint32_t aAudioChannel,
>                                            bool aActive);
> 
>+    sync GetKeyboardIndicators() returns (bool showAccelerators,
>+                                          bool showFocusRings);
>+
We really should be able to pass this kind of data to child when creating PBrowser. Sync messaging during tab creation is bad for ux.
Why not send that initial data in TabContext or somewhere?

Comment 8

[Neil Deakin]

Attachment: Initialize keyboard indicators in child processes, v3

This version uses the TabContext.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8760296 [details] [diff] [review]
Initialize keyboard indicators in child processes, v3

+  virtual bool ShowAccelerators() override {
nit, { goes to its own line. Same also with other new methods in nsWindowRoot.h

 
>+UIStateChangeType
>+TabContext::ShowAccelerators() const{
nit, { goes to its own line

>+UIStateChangeType
>+TabContext::ShowFocusRings() const{
ditto

>+++ b/widget/windows/nsWindow.cpp
>@@ -5583,17 +5583,21 @@ nsWindow::ProcessMessage(UINT msg, WPARA
>     if (action == UIS_SET || action == UIS_CLEAR) {
>       int32_t flags = HIWORD(wParam);
>       UIStateChangeType showAccelerators = UIStateChangeType_NoChange;
>       UIStateChangeType showFocusRings = UIStateChangeType_NoChange;
>       if (flags & UISF_HIDEACCEL)
>         showAccelerators = (action == UIS_SET) ? UIStateChangeType_Clear : UIStateChangeType_Set;
>       if (flags & UISF_HIDEFOCUS)
>         showFocusRings = (action == UIS_SET) ? UIStateChangeType_Clear : UIStateChangeType_Set;
>-      NotifyUIStateChanged(showAccelerators, showFocusRings);
>+
>+      if (mWindowType == eWindowType_toplevel ||
>+          mWindowType == eWindowType_dialog) {
>+        NotifyUIStateChanged(showAccelerators, showFocusRings);
>+      }
This looks a bit weird. Why are we even calculating the values for showAccelerators and showFocusRings if we aren't going to use them?
So, move the check higher up to be in 'if (action == UIS_SET || action == UIS_CLEAR) {'
if ((action == UIS_SET || action == UIS_CLEAR) && 
    (mWindowType == eWindowType_toplevel || mWindowType == eWindowType_dialog) {


The added mWindowType check does look somewhat unrelated to this bug, but ok.

Comment 10

[Neil Deakin]

> if ((action == UIS_SET || action == UIS_CLEAR) && 
>     (mWindowType == eWindowType_toplevel || mWindowType ==
> eWindowType_dialog) {
> 
> 
> The added mWindowType check does look somewhat unrelated to this bug, but ok.

There's a test where Windows sends this message to a plugin window, so this is the easiest way to filter that out.

Comment 11

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/435c691340a05280302417b3ba150a317cc16c08
Bug 1174798, propagate keyboard indicator state down to child processes, r=smaug

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/435c691340a0