Closed Bug 1174798 Opened 4 years ago Closed 3 years ago

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

Categories

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

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox50 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin, Mentored)

References

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 file, 2 obsolete files)

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.
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.
tracking-e10s: --- → ?
Blocks: 1272550
Summary: [e10] nsGlobalWindow::SetKeyboardIndicators should propagate value to child process windows → [e10s] nsGlobalWindow::SetKeyboardIndicators should propagate value to child process windows
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Priority: -- → P2
Hey Neil, did you mean to set a reviewer for your patch, or were you still working on it?
Whiteboard: [fct-active]
Whiteboard: [fct-active] → [fce-active]
Duplicate of this bug: 1047716
Duplicate of this bug: 1109474
This patch initializes the focus rings in child process windows, and propagates the keyboard state flags when they change.
Attachment #8752971 - Attachment is obsolete: true
Flags: needinfo?(enndeakin)
Attachment #8757299 - Flags: review?(bugs)
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?
Attachment #8757299 - Flags: review?(bugs) → review-
Depends on: 1253610
This version uses the TabContext.
Attachment #8757299 - Attachment is obsolete: true
Attachment #8760296 - Flags: review?(bugs)
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.
Attachment #8760296 - Flags: review?(bugs) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/435c691340a05280302417b3ba150a317cc16c08
Bug 1174798, propagate keyboard indicator state down to child processes, r=smaug
https://hg.mozilla.org/mozilla-central/rev/435c691340a0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1291651
Depends on: 1301631
No longer depends on: 1301631
Whiteboard: [fce-active] → [fce-active-legacy]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.