Closed Bug 1241707 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(e10s+, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: bgrins, Assigned: linclark)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

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)
Can only re-enable browser_webconsole_bug_588342_document_focus.js after this is fixed
Blocks: 1112599
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
Summary: [e10s] Console should restore focus to proper place when closed → [e10s] Web Console should restore focus to proper place when closed
Flags: needinfo?(twalker)
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.
Flags: needinfo?(twalker)
ni'ing Jen Fong from devtools

Jen, e10s team may need help with this.
Flags: needinfo?(jfong)
Lin, do you think you could take a look at this with bgrins?
Flags: needinfo?(jfong) → needinfo?(lclark)
No longer blocks: 1112599
Blocks: 1112599
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?
Flags: needinfo?(lclark) → needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead) → needinfo?(jryans)
(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
Flags: needinfo?(jryans)
Attached patch Bug1241707.patch (obsolete) — Splinter Review
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.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attachment #8713798 - Flags: review?(bgrinstead)
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.
Attachment #8713798 - Flags: review?(bgrinstead) → feedback?(jryans)
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)...
Attachment #8713798 - Flags: feedback?(jryans) → feedback+
Attached patch Bug1241707.patch (obsolete) — Splinter Review
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?
Attachment #8713798 - Attachment is obsolete: true
Attachment #8715583 - Flags: review?(jryans)
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.
Attachment #8715583 - Flags: review?(jryans) → review+
Attached patch Bug1241707.patch (obsolete) — Splinter Review
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
Attachment #8715583 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch Bug1241707.patchSplinter Review
Rerolled.
Attachment #8718534 - Attachment is obsolete: true
(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
https://hg.mozilla.org/mozilla-central/rev/b8003b4c63c3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1251863
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.