1278889 - Focus via Toolbox is lost when docking

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Ada [:adalucinet]]

Attachment: Screen recording

[Affected versions]:
- Firefox 48 beta 1
- latest Aurora 49.0a2 e10s on/off
- latest Nightly 50.0a1 e10s on/off

[Affected platforms]:
- Windows 10 64-bit
- Mac OS X 10.10.5
- Ubuntu 16.04 64-bit

[Steps to reproduce]:
1. Launch Firefox
2. Open Web console
3. Click to dock to side/bottom/separate window

[Expected result]:
- The focus is maintained in the Web console.

[Actual result]:
- The focus is lost.

[Regression range]:
- Not reproducible with Firefox 47.0RC; will investigate further and update asap.

[Additional notes]:
- Note that this is applicable for the rest of the toolbox-hosted tools.
- Screen recording attached.

Comment 1

[Marco Mucci [:MarcoM]]

Hi Alex, which bug should this defect block?  Thanks.

Comment 2

[Ada [:adalucinet]]

Here is the regression range:
Last good revision: 3435dd7ad71fe9003bdeee18fd38d815e033beef
First bad revision: dc4d7f68030e9edd22ad0bb0bd2244d047dd767d
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3435dd7ad71fe9003bdeee18fd38d815e033beef&tochange=dc4d7f68030e9edd22ad0bb0bd2244d047dd767d

Based on the pushlog, this is a 49 regression; which is odd because it's reproducible with 48 beta 1 too (as mentioned in comment 0). Note that the regression range was performed twice for this reason.

Comment 3

[Brian Grinstead [:bgrins]]

Blocking on Bug 1266419 since it seems like it could be related to the toolbar button change and might just be a need to preventDefault, but the fact that it's happening in 48 is suspicious since it didn't land there.

Comment 4

[Joe Walker [:jwalker]]

This bug has a priority set that is not P1, so it's not an urgent fix.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

This was caused by the button's click event making the button steal focus and then the frameSwitcher getting the focus wrong.

We now:
1. On mousedown we save the currently focused element.
2. At the end of the click event we focus the correct element again.

A simple test:
1. Type something into a filter field.
2. Dock the toolbox to the side or detach it.

If you continue typing the text will be appended to the end of the text field you had focused.

It works just as well with other elements.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8767220 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

The frameloader appears to make the browser think that the window is focused although it isn't. This means that if the window is focused rather than an HTML node keyboard navigation still becomes broken.

We should still be able to focus the first HTML node in this situation though.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

I am much happier with this fix.

Focusing the window is not enough... we just need to focus the iframe.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8767687 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

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

This seems to work pretty well.

I was however able to loose focus a few times, but I don't have exact steps to reproduce unfortunately. It almost looks like a race condition that's sometime causing the focus to be lost.
It happened to me most often in the netmonitor search field.
But also in the console in fact.
I was just testing this several times, switching to different panels, focusing different fields, and then switching hosts many times, and again and again. And at some point, focus would be lost.

Brian mentioned that bug 1266419 could be the reason for this not to work anymore. Maybe you should look into the changes made in that bug, maybe that could give insight into why the code that was there before stopped working at all.

Also, if I understand correctly, this stopped working at some stage and we didn't notice, meaning that we either don't have a test for this, or it doesn't work correctly.
After looking, I think browser_toolbox_toggle.js should be responsible for this.

Can you check why it didn't fail without your patch?

::: devtools/client/framework/toolbox.js
@@ +1814,5 @@
>  
>        this._buildDockButtons();
>        this._addKeysToWindow();
>  
> +      this.win.document.querySelector("iframe").focus();

Maybe keep the comment before this line?

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

(In reply to Patrick Brosset <:pbro> from comment #8)
> Comment on attachment 8767687 [details] [diff] [review]
> 1278889-stop-toolbox-losing-focus-when-docking.diff
> 
> Review of attachment 8767687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Brian mentioned that bug 1266419 could be the reason for this not to work
> anymore. Maybe you should look into the changes made in that bug, maybe that
> could give insight into why the code that was there before stopped working
> at all.
> 

None of those code changes appear to be related. I tried removing that patch piece by piece and it made no difference.

As far as I can tell, this is an issue with swapDocShells()... the platform "thinks" that the window is focused when it isn't. I suspect that the event fails to propagate properly... maybe a message manager issue?

> Also, if I understand correctly, this stopped working at some stage and we
> didn't notice, meaning that we either don't have a test for this, or it
> doesn't work correctly.
> After looking, I think browser_toolbox_toggle.js should be responsible for
> this.
> 

That test opens and closes a docked toolbox and then opens and closes a detached toolbox so it is not related. I will add a test though.

> Can you check why it didn't fail without your patch?
> 

See above... the test is unrelated to this issue.

> ::: devtools/client/framework/toolbox.js
> @@ +1814,5 @@
> >  
> >        this._buildDockButtons();
> >        this._addKeysToWindow();
> >  
> > +      this.win.document.querySelector("iframe").focus();
> 
> Maybe keep the comment before this line?

Done

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

We decided not to add a test because any test relying on focus is going to be very vulnerable to intermittent oranges.

Patrick agreed that we should land this because, even though the focus doesn't work 100% of the time (you can apparently break it if you try hard) it is a great improvement over our current situation and works 99.9% of the time.

Patrick he is now on vacation so asking Honza for review.

@Lin: Patrick has agreed that we can land this so you don't need to spend too much time on the review.

Comment 11

[Lin Clark [:linclark]]

Comment on attachment 8775124 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

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

I'm not familiar enough with this code to know the implications of the change, so I'm afraid I can't r+.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@bgrins: It is a simple change that greatly improves this issue... rather than chase our tails here we are best landing this. I can always ask pbrosset for review if you don't have time.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8775124 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

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

Indeed, it seems much better with the patch applied. Works for me with a green try (maybe some retriggers to make sure we aren't going to start up an intermittent).  Please update the commit message to have r=me and also instead of 'losing focus when docking' something like 'losing focus when changing docking modes'

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac1be84eb7a

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I never noticed the  --rebuild 5 option... lets you run all tests N times.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

A few consistent failures here... let's try this on a green base.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016b921d21b5

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So the consistent failures will be focus issues... investigating.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Only one real failure:
124 INFO TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_keybindings_02.js | The zoom level was changed in the toolbox - Didn't expect 1, but got it

Investigating...

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Relevant part of the test log:
 15:12:54     INFO -  29 INFO Create a test tab and open the toolbox
 15:12:54     INFO -  30 INFO Adding a new tab with URL: data:text/html;charset=utf8,test page
 15:12:54     INFO -  31 INFO Waiting for event: 'load' on [object XULElement].
 15:12:54     INFO -  32 INFO Got event: 'load' on [object XULElement].
 15:12:54     INFO -  33 INFO Tab added and finished loading
 15:12:54     INFO -  34 INFO Switch to host type side
 15:12:54     INFO -  35 INFO Try to use the toolbox shortcuts
 15:12:54     INFO -  36 INFO Zooming with key: toolbox.zoomIn.key
 15:12:54     INFO -  37 INFO Synthesizing key shortcut: CmdOrCtrl+Plus
 15:12:54     INFO -  38 INFO TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_keybindings_02.js | The zoom level was changed in the toolbox - Didn't expect 1, but got it

So the toolbox is opened at the bottom and moved to the side. It could be that we need an event to say that the toolbox is focused.

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

I have logged bug 1291755 to fix the issue with browser_keybindings_02.js failing. It is caused by the web console still needing to be converted from XUL to HTML. I suspect that this issue is caused by it having a deep XUL structure and the focus is failing to pass through some boundary.

For the meantime I have disabled devtools/client/framework/test/browser_keybindings_02.js

Because this bug improves toolbox focus in general and the web console is due to be rewritten we should land this first and look at the issue after the rewrite.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be343fc637b8

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8777412 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

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

::: devtools/client/framework/test/browser.ini
@@ +31,5 @@
>  [browser_dynamic_tool_enabling.js]
>  [browser_ignore_toolbox_network_requests.js]
>  [browser_keybindings_01.js]
>  [browser_keybindings_02.js]
> +skip-if = true # Bug 1291755 - Focusing toolbox fails for this test.

I don't think we should skip the test just because it started failing.  Does this mean that key shortcuts / focus would no longer work when changing docking modes with the console selected?

Comment 25

[Brian Grinstead [:bgrins]]

Also the console output region is actively being converted to HTML, but the jsterm input isn't.  So if the focus issue has to do with the XUL textbox at the bottom of the console, it's not going to be fixed in that project.

Comment 26

[Brian Grinstead [:bgrins]]

Just confirmed with the patch applied that this always loses focus when changing between bottom/side docking mode in the console (either using the button or the keyboard shortcut).

There's some randomness involved here - for me with a fersh checkout of fx-team I'm able to sometimes switch docking modes in webconsole without losing focus.  Using the keyboard shortcut never seems to cause focus to be lost but if I click on the button it sometimes happens.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Comment on attachment 8777412 [details] [diff] [review]
> 1278889-stop-toolbox-losing-focus-when-docking.diff
> 
> Review of attachment 8777412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/test/browser.ini
> @@ +31,5 @@
> >  [browser_dynamic_tool_enabling.js]
> >  [browser_ignore_toolbox_network_requests.js]
> >  [browser_keybindings_01.js]
> >  [browser_keybindings_02.js]
> > +skip-if = true # Bug 1291755 - Focusing toolbox fails for this test.
> 
> I don't think we should skip the test just because it started failing.  Does
> this mean that key shortcuts / focus would no longer work when changing
> docking modes with the console selected?

I disabled the test temporarily because I have tried focusing all iframes, contentDocuments, contentWindows and documentElements and we still can't get the console focused. Considering this will be rewritten it doesn't make sense to spend an extended period of time on it.

I guess I can spend another day on it but otherwise I think disabling the test temporarily is a good decision.

(In reply to Brian Grinstead [:bgrins] from comment #26)
> Just confirmed with the patch applied that this always loses focus when
> changing between bottom/side docking mode in the console (either using the
> button or the keyboard shortcut).
> 
> There's some randomness involved here - for me with a fersh checkout of
> fx-team I'm able to sometimes switch docking modes in webconsole without
> losing focus.  Using the keyboard shortcut never seems to cause focus to be
> lost but if I click on the button it sometimes happens.

The focus manager actually *thinks* the toolbox has focus but in reality the content area does.

I should look into the platform side of swapDocShells and see if there is a race condition. Somehow the content area is half gaining focus, most likely when the docshells are swapped.

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: prevent-default-on-mousedown.patch

This fixes the issue for me locally

Comment 29

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I can debug focus using:
export A11YLOG=focus,tree,DOMEvents;

All flags:
  docload
  doccreate
  docdestroy
  doclifecycle
  events
  platforms
  stack
  text
  tree
  DOMEvents
  focus
  selection
  notifications

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> Created attachment 8777590 [details] [diff] [review]
> prevent-default-on-mousedown.patch
> 
> This fixes the issue for me locally

So your patch fixes the issue of focus sometimes getting lost when you click on the button? Thanks for that.

Comment 31

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I am now focusing using:

```
this.focusTool(this.currentToolId);
```

Which has left it clear that the tool panels *are* focused but [cmd][-] and [cmd][+] affect the content area. This is interesting because it seems that after selecting the tool through the toolbar the same keys appear to work.

I need to see how these shortcuts are implemented.

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Interestingly, applying bgrins patch:
      button.addEventListener("mousedown", e => {
        e.preventDefault();
      }, true);

Which simply prevents focus on mousedown on the toolbox dock button completely breaks the web console when opened in the sidebar... you cannot type in the web console's input box.

Comment 33

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Got to the bottom of it.

If a tool is focused when we call swapFrameLoaders() it turns out in a state where the browser *thinks* the toolbox is focused but it isn't... the content area is focused really or at least it receives all key events. In fact... the tool becomes completely unfocusable unless you click outside the tool to blur it and then click the tool to refocus it.

Comment 34

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1278889-stop-toolbox-losing-focus-when-docking.diff

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4712f36a9750

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7aeccc73f1d0
Stop the toolbox losing focus when changing docking modes. r=bgrins

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7aeccc73f1d0

Comment 38

[Tanvir Rahman]

I have reproduced this bug with Nightly 50.0a1(2016-06-08) on Windows 10, 64 bit!

The Bug's fix is now verified on Nightly 51.0a1

Build ID 	20160806030806
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160803]

Comment 39

[Farhadur Reja Fahim]

I have reproduced this bug with Nightly 50.0a1 (2016-06-08) on Ubuntu 14.04, 64 bit!
The bug's fix is now verified on latest Nightly 51.0a1

Nightly 51.0a1:
Build ID 	20160807030201
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160803]

Comment 40

[Gerry Chang [:gchang]]

Hi :miker,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50?

Comment 41

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8777849 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

(In reply to Gerry Chang [:gchang] from comment #40)
> Hi :miker,
> Since this bug is a regression and also affects 49/50, are you also
> considering to uplift this patch to 49/50?

We certainly should do.

Approval Request Comment
[Feature/regressing bug #]: Toolbar converted to html
[User impact if declined]: Focus issues when switching between devtools docking modes
[Describe test coverage new/current, TreeHerder]: Can't be tested properly due to platform bug that makes the browser believe that the toolbox is focused.
[Risks and why]: Low risk with large benefits
[String/UUID change made/needed]: None

Comment 42

[Gerry Chang [:gchang]]

Comment on attachment 8777849 [details] [diff] [review]
1278889-stop-toolbox-losing-focus-when-docking.diff

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

This patch fixes a regression. Let's take it in 49 beta and 50 aurora. Should be in 49 beta 4

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a8ba9487038

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/23af684ebd4c