1241707 - [e10s] Web Console should restore focus to proper place when closed

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Brian Grinstead [:bgrins]]

STR:
* Open data:text/html;charset=utf-8,<style>:focus { background: red }</style><input autofocus='true' />
* Open console
* Close console

Expected
Input is re-focused (as it does in non-e10s after Bug 588342)

Comment 1

[Brian Grinstead [:bgrins]]

Can only re-enable browser_webconsole_bug_588342_document_focus.js after this is fixed

Comment 2

[Tracy Walker [:tracy]]

STR:

0) in e10s mode (this does not affect non-e10s mode/windows)
1) open the minimized test case: data:text/html;charset=utf-8,<style>:focus { background: red }</style><input autofocus='true' /> (note that focus is in the red text field)
2) Select Tools > Web Developer > Web Console
3) Close the Web Console with the [X] at far right of its toolbar.

Test result: Focus is lost, to return focus to the text entry field you must click there.

Expected result:  Focus is returned to the text entry field once the Web Console is closed.

Changed Summary to define which console is affected; Web Console displays this bug. The Browser Console is not affected.  

Mac 10.9.5 Nightly 47.0a1 20160125060632 (e10s) - FAIL
Windows 7 Nightly 47.0a1 20160125060632 (e10s) - FAIL
Ubuntu 14 Nightly 47.0a1 20160126030244 (e10s) - FAIL

Mac 10.9.5 Firefox Release 43.0.4 (non-e10s) - PASS
Mac 10.9.5 Nightly 47.0a1 20160125060632 (new non-e10s window) - PASS

Comment 3

[Tracy Walker [:tracy]]

Per STR's from comment #2, step 0, swapping in non-e10s window for e01s, the focus is correctly returned to the red text field after step 3.

Win 7 (VM)- Nightly 47.0a1, Build ID 20160127030236, non-e10s - PASS
Ubuntu (VM) - Nightly 47.0a1, Build ID 20160128030208, non-e10s - PASS
Mac 10.9.5 - Nightly 47.0a1, Build ID 20160128030208, non-e10s - PASS

Recap: This bug is 100% reproducible in e10s only.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

ni'ing Jen Fong from devtools

Jen, e10s team may need help with this.

Comment 5

[Jen Fong-Adwent [:ednapiranha]]

Lin, do you think you could take a look at this with bgrins?

Comment 6

[Lin Clark [:linclark]]

This is the block of code we need to change:

> let onDestroy = function WC_onDestroyUI() {
>   try {
>     let tabWindow = this.target.isLocalTab ? this.target.window : null;
>     tabWindow && tabWindow.focus();
>   }

The problem here is that e10s targets are remote. This means we can't directly access the window object on the target.

My understanding is that to interact with the window object, we'd need to add a method to one of the actors. I'm not sure which actor would be the best place for this. :bgrins, do you have any thoughts?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark [:linclark] from comment #6)
> This is the block of code we need to change:
> 
> > let onDestroy = function WC_onDestroyUI() {
> >   try {
> >     let tabWindow = this.target.isLocalTab ? this.target.window : null;
> >     tabWindow && tabWindow.focus();
> >   }
> 
> The problem here is that e10s targets are remote. This means we can't
> directly access the window object on the target.
> 
> My understanding is that to interact with the window object, we'd need to
> add a method to one of the actors. I'm not sure which actor would be the
> best place for this. :bgrins, do you have any thoughts?

Maybe the TabActor?  Forwarding request to jryans.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Lin Clark [:linclark] from comment #6)
> > This is the block of code we need to change:
> > 
> > > let onDestroy = function WC_onDestroyUI() {
> > >   try {
> > >     let tabWindow = this.target.isLocalTab ? this.target.window : null;
> > >     tabWindow && tabWindow.focus();
> > >   }
> > 
> > The problem here is that e10s targets are remote. This means we can't
> > directly access the window object on the target.
> > 
> > My understanding is that to interact with the window object, we'd need to
> > add a method to one of the actors. I'm not sure which actor would be the
> > best place for this. :bgrins, do you have any thoughts?
> 
> Maybe the TabActor?  Forwarding request to jryans.

Yes, I think adding a "focus" method to TabActor (in webbrowser.js) would make sense as it generally represents the overall window being targeted.

Lin, hopefully it's not too confusing given the massive webbrowser.js file and old-style actor there.  For reference, I would suggest reading actor-heirarchy.md[1] if you haven't see it yet.  It tries to explain the connections between the various different "TabActor" like things.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/actor-hierarchy.md

Comment 9

[Lin Clark [:linclark]]

Attachment: Bug1241707.patch

This patch:

- adds a focus method to TabActor
- if the target is remote, uses that focus method when the web console is destroyed

I didn't add any additional tests. I didn't see unit tests for other TabActor methods like navigateTo, but it's very possible that I missed them. Let me know if I should add one.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8713798 [details] [diff] [review]
Bug1241707.patch

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

Clearing review for now since I'm still seeing the issue in an e10s window but this looks like generally the right path.

> I didn't add any additional tests. I didn't see unit tests for other
> TabActor methods like navigateTo, but it's very possible that I missed them.
> Let me know if I should add one.

Forwarding a feedback request to jryans for this question, plus for the changes to webbrowser / main.  Seems like we could have at least a chrome mochitest for basic TabActor functionality but I don't see anything existing either.

::: devtools/client/webconsole/hudservice.js
@@ +588,5 @@
>      }
>  
>      let onDestroy = function WC_onDestroyUI() {
>        try {
> +        if (this.target.isLocalTab) {

I think we don't need the conditional and can just always call target.activeTab.focus, but I know you are debugging this further - especially with regard to Browser Console.  Although I don't think the current code would be running with BC.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8713798 [details] [diff] [review]
Bug1241707.patch

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

(In reply to Brian Grinstead [:bgrins] from comment #10)
> > I didn't add any additional tests. I didn't see unit tests for other
> > TabActor methods like navigateTo, but it's very possible that I missed them.
> > Let me know if I should add one.
> 
> Forwarding a feedback request to jryans for this question, plus for the
> changes to webbrowser / main.  Seems like we could have at least a chrome
> mochitest for basic TabActor functionality but I don't see anything existing
> either.

Overall, the TabActor changes seem good.

As for testing, I think we've mostly used browser mochitests as integration tests for TabActor features in past, so there are less server-side tests currently.

Perhaps some of the inspector server tests like test_inspector_getImageData.html could be used as a starting point? :ochameau may have more ideas on how to test as well.

::: devtools/client/webconsole/hudservice.js
@@ +588,5 @@
>      }
>  
>      let onDestroy = function WC_onDestroyUI() {
>        try {
> +        if (this.target.isLocalTab) {

I agree with :bgrins, you should be able to use `this.target.activeTab.focus` in all cases.

Also, it's now an async action since RDP is used.  The focus function returns a promise, in case you want to yield until it's done, though maybe that does not matter here.

::: devtools/server/actors/webbrowser.js
@@ +1401,5 @@
>  
>    /**
> +   * Bring the tab's window to front.
> +   */
> +  onFocus: function(aRequest) {

Remove unused aRequest.

@@ +1403,5 @@
> +   * Bring the tab's window to front.
> +   */
> +  onFocus: function(aRequest) {
> +    this.window.focus();
> +    return {};

Some day, we'll use protocol.js for this and remove strange oddities like this (which is only present to make the server send a reply)...

Comment 12

[Lin Clark [:linclark]]

Attachment: Bug1241707.patch

The conditional was definitely wrong. But there is something going on that does require extra logic. 

I had put the conditional in to address failing tests. You can see which tests failed here:

Failing try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b03cc701380

I managed to get the tests passing by doing a check for this.window before calling this.window.focus() in TabActor:focus().

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bcd0bed3909

> Remove unused aRequest.

I had thought about this but decided to make it consistent with other methods like onDetach. Should I change it or leave it consistent with the others?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8715583 [details] [diff] [review]
Bug1241707.patch

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

::: devtools/client/webconsole/hudservice.js
@@ +598,5 @@
>  
>        let id = WebConsoleUtils.supportsString(this.hudId);
>        Services.obs.notifyObservers(id, "web-console-destroyed", null);
>        this._destroyer.resolve(null);
> +    }.bind(this));

I don't think the `bind` call is needed when using `Task.async`, but re-test to make sure.

::: devtools/server/actors/webbrowser.js
@@ +1401,5 @@
>  
>    /**
> +   * Bring the tab's window to front.
> +   */
> +  onFocus: function(aRequest) {

I would still remove it... ESLint would mark an error for the unused arg.  I think (once people are familiar with this older actor design) it's relatively clear that all the handlers get the request, so shouldn't be too confusing if it was needed later for some reason.

Comment 14

[Lin Clark [:linclark]]

Attachment: Bug1241707.patch

Removing .bind() made treeherder a firey display of oranges and reds, so I added it back in.

For posterity...
Without .bind(): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fe5f598937e
With bind: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ac863a62e8

Comment 15

[Lin Clark [:linclark]]

Attachment: Bug1241707.patch

Rerolled.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cdbe72c64a60

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/d8f5ce400566

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Pulsebot from comment #17)
> Backout:
> https://hg.mozilla.org/integration/fx-team/rev/d8f5ce400566

It looks like this got backed out on accident due to it being pushed along with: https://bugzilla.mozilla.org/show_bug.cgi?id=1225743#c37.  Going to reland

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b8003b4c63c3

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8003b4c63c3