Bug 66068 - Web Inspector: console loses focus upon reloading the page from the inspector.
Summary: Web Inspector: console loses focus upon reloading the page from the inspector.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-11 09:41 PDT by Pavel Feldman
Modified: 2011-08-12 05:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.96 KB, patch)
2011-08-11 10:27 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (33.56 KB, patch)
2011-08-12 04:39 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-08-11 09:41:19 PDT
Open Scripts tab
Open console
Type something
Press Ctrl+R (Cmd+R) to reload the page

Console no longer has focus.
Comment 1 Pavel Feldman 2011-08-11 10:27:08 PDT
Created attachment 103643 [details]
Patch
Comment 2 Yury Semikhatsky 2011-08-11 23:44:22 PDT
Comment on attachment 103643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103643&action=review

> Source/WebCore/inspector/front-end/ElementsPanel.js:224
> +            WebInspector.console.focusIfVisible();

It is somewhat controversial: this way user will need to return focus back to the DOM tree in order to use keyboard to navigate/edit DOM tree. Is there a user requests for  this feature?
Comment 3 Yury Semikhatsky 2011-08-11 23:47:12 PDT
Comment on attachment 103643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103643&action=review

> Source/WebCore/inspector/front-end/ConsoleView.js:302
> +        if (this.toggleConsoleButton.toggled)

Can we have ConsoleView listening to the navigation event and restore focus on it only if it was in the console before the navigation?
Comment 4 Pavel Feldman 2011-08-12 00:00:31 PDT
> Can we have ConsoleView listening to the navigation event and restore focus on it only if it was in the console before the navigation?

This is my big request since I tried to use console and reloaded a page several times yesterday. I suggest that you try following the steps in the bug.

Ideally, we should not manually manage focus at all. Today, upon page load (no matter whether user reloaded it from inspector or the page did it), we restore various selections in the tree outlines. These selections steal focus from the console should it be there. So I decided to restore it to console in case it was opened. (Opened console is a somewhat strong indication that it has user's focus). I can see the downside of it, but it is easily worked around via closing the console.

Alternatively, I could make TreeElement's select not request focus. But in that case, with console closed, reloading the page on elements panel would remote focus from the tree. Remembering the focus owner view and restoring it after its re-population sounds like the right, but fairly long way to go. I can try doing that, but if I fail, I'm suggesting that we land this.
Comment 5 Yury Semikhatsky 2011-08-12 00:26:12 PDT
(In reply to comment #4)
> > Can we have ConsoleView listening to the navigation event and restore focus on it only if it was in the console before the navigation?
> 
> This is my big request since I tried to use console and reloaded a page several times yesterday. I suggest that you try following the steps in the bug.
> 
> Ideally, we should not manually manage focus at all. Today, upon page load (no matter whether user reloaded it from inspector or the page did it), we restore various selections in the tree outlines. These selections steal focus from the console should it be there. So I decided to restore it to console in case it was opened. (Opened console is a somewhat strong indication that it has user's focus). I can see the downside of it, but it is easily worked around via closing the console.
> 
> Alternatively, I could make TreeElement's select not request focus. But in that case, with console closed, reloading the page on elements panel would remote focus from the tree. Remembering the focus owner view and restoring it after its re-population sounds like the right, but fairly long way to go. I can try doing that, but if I fail, I'm suggesting that we land this.

I think we can file a bug on preserving focused view and fix it in another change.
Comment 6 Pavel Feldman 2011-08-12 04:39:38 PDT
Created attachment 103760 [details]
Patch
Comment 7 Yury Semikhatsky 2011-08-12 04:49:57 PDT
Comment on attachment 103760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103760&action=review

Please make sure all usages and implementations of revealAndSelect has consistent signatures.

> Source/WebCore/inspector/front-end/ElementsPanel.js:183
> +        this.selectDOMNode(null);

this.selectDOMNode(null, false); ?

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:80
> +    selectedDOMNode: function()

Why is it not a getter?

> Source/WebCore/inspector/front-end/ProfilesPanel.js:393
> +        profile._profilesTreeElement.revealAndSelect(false);

Remove the false parameter?

> Source/WebCore/inspector/front-end/treeoutline.js:439
> +TreeOutline.prototype.revealAndSelect = function()

Add omitFocus parameter?
Comment 8 Pavel Feldman 2011-08-12 05:04:36 PDT
(In reply to comment #7)
> (From update of attachment 103760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103760&action=review
> 
> Please make sure all usages and implementations of revealAndSelect has consistent signatures.
> 
> > Source/WebCore/inspector/front-end/ElementsPanel.js:183
> > +        this.selectDOMNode(null);
> 
> this.selectDOMNode(null, false); ?
> 

Done.

> > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:80
> > +    selectedDOMNode: function()
> 
> Why is it not a getter?
> 

It used to be setSelectedDOMNode and I want to maintain their consistency.

> > Source/WebCore/inspector/front-end/ProfilesPanel.js:393
> > +        profile._profilesTreeElement.revealAndSelect(false);
> 
> Remove the false parameter?
> 

Done.

> > Source/WebCore/inspector/front-end/treeoutline.js:439
> > +TreeOutline.prototype.revealAndSelect = function()
> 
> Add omitFocus parameter?

Done.
Comment 9 Pavel Feldman 2011-08-12 05:56:46 PDT
Committed r92956: <http://trac.webkit.org/changeset/92956>