DevTools: Remember the last focused widget.

DevTools: Remember the last focused widget.
When focusing a widget, try to focus the last focused child widget instead of the first child widget.

BUG=646046
Committed: https://crrev.com/1817d1b7a6958902adb487eac198c6d13d96a423
Cr-Commit-Position: refs/heads/master@{#418121}

[einbinder, 2016-09-07 18:30:36]

einbinder@chromium.org changed reviewers:
+ dgozman@chromium.org, luoe@chromium.org, pfeldman@chromium.org

[einbinder, 2016-09-07 18:30:37]

Ptal. It works, there are a few issues but I'd like some feedback before I keep
going.

I'm not quite happy with the wasFocused method, it's too easy to forget. Maybe
you don't override focus and instead an event gets fired or an onFocus method
gets called?

Right now a root widget doesn't have an event listener if it has a
defaultFocusedElement. This is a pretty small edge case, but I need to think
more about the memory dependencies.

[einbinder, 2016-09-07 23:57:58]

Instead of listening for focus on the defaultFocusedElement, I listen on the
document and walk up the element tree looking for widgets. This made the code a
lot nicer.

[dgozman, 2016-09-08 00:26:09]

I propose this:
- keep wasFocused and _lastFocusedChild;
- public restoreFocusedChild() returns boolean;
- focus() goes into element, then into restoreFocusedChild(), then into
children[0].

[pfeldman, 2016-09-08 00:53:50]

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(left):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:128:
this._sourcesView.focus();
Did we only have one?

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:873:
WebInspector.Widget.focusWidgetForNode(x);
I'd listen to focus event independently to decouple new nice code from this old
ugly code.

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:436:
makeDefaultFocused: function()
I'd rather have it on the parent.

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:439:
this._parentWidget._lastFocusedChild = this;
You should now call it defaultFocusChild...

[luoe, 2016-09-08 20:48:56]

https://codereview.chromium.org/2319523004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:451:
this._defaultFocusedElement.removeEventListener("focus", this._boundWasFocused);
Should this and L437 also get useCapture == true, same as L449?

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:858: x.focus();
I'm a little confused, if we have this x.focus(), why do we need the extra call
on 873?  Doesn't this focus() trigger the listener?

[einbinder, 2016-09-09 01:32:16]

I started doing dgozman's restoreFocus proposal, but it got messy because
implementing a defaultFocusedChild by overriding Widget.focus required
intimidate knowledge of how Widget.focus used restoreFocus.

I moved the makeDefaultFocused I had in SourcesPanel to the SplitWidget itself.
It makes sense for all of the mainWidgets to be focused by default.

@luoe, I moved the focus listener to the _focusChanged instead of
setCurrentFocusElement

https://codereview.chromium.org/2319523004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:451:
this._defaultFocusedElement.removeEventListener("focus", this._boundWasFocused);
On 2016/09/08 at 20:48:56, luoe wrote:
> Should this and L437 also get useCapture == true, same as L449?

This was removed in the next patch.

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(left):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:128:
this._sourcesView.focus();
On 2016/09/08 at 00:53:50, pfeldman wrote:
> Did we only have one?

For now. There are some places where we don't set focus at all and we should,
and some places where we set focus explicitly and we should do this instead. I
haven't tracked them all down yet.

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2319523004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:873:
WebInspector.Widget.focusWidgetForNode(x);
On 2016/09/08 at 00:53:50, pfeldman wrote:
> I'd listen to focus event independently to decouple new nice code from this
old ugly code.

Listening for focus is a bit complicated because of ShadowRoots, but I moved it
to WebInspector._focusChanged instead of setCurrentFocusElement.

[dgozman, 2016-09-12 16:50:22]

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:874: 
stray blank line

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:442:
WebInspector.Widget.__assert(childIndex >= 0, "Attempt to set non-child widget
as default focused.");
Let's instead check child._parentWidget === this.

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:454: if
(this._defaultFocusedChild)
Let's go to the first visible children if this is not visible.

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:458: _wasFocused:
function()
Let's inline this into focusWidgetForNode.

[einbinder, 2016-09-12 18:09:11]

Description was changed from

==========
DevTools: Remember the last focused widget.
When focusing a widget, try to focus the last focused child widget instead of
the first child widget.

BUG=none
==========

to

==========
DevTools: Remember the last focused widget.
When focusing a widget, try to focus the last focused child widget instead of
the first child widget.

BUG=646046
==========

[einbinder, 2016-09-12 18:18:54]

ptal

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:874: 
On 2016/09/12 at 16:50:21, dgozman wrote:
> stray blank line

Done.

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:442:
WebInspector.Widget.__assert(childIndex >= 0, "Attempt to set non-child widget
as default focused.");
On 2016/09/12 at 16:50:22, dgozman wrote:
> Let's instead check child._parentWidget === this.

Done.

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:454: if
(this._defaultFocusedChild)
On 2016/09/12 at 16:50:22, dgozman wrote:
> Let's go to the first visible children if this is not visible.

Done.

https://codereview.chromium.org/2319523004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:458: _wasFocused:
function()
On 2016/09/12 at 16:50:22, dgozman wrote:
> Let's inline this into focusWidgetForNode.

Done.

[dgozman, 2016-09-12 18:24:54]

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:242: if
(!this._parentWidget._defaultFocusedChild)
I think we can now remove this one, as we iterate over children in focus()
anyway.

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:339:
this._parentWidget._defaultFocusedChild = this._parentWidget._children[0] ||
null;
Just set to null here?

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:456: else {
style: else on the same line as }

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:627: while (node) {
nit: I'd split this into two loops:

while (node && !node.__widget) node = node.parentNodeOrShadowHost();
if (!node) return;
while (widget._parentWidget) { ... }

[einbinder, 2016-09-12 18:37:12]

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Widget.js (right):

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:242: if
(!this._parentWidget._defaultFocusedChild)
On 2016/09/12 at 18:24:54, dgozman wrote:
> I think we can now remove this one, as we iterate over children in focus()
anyway.

Done.

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:339:
this._parentWidget._defaultFocusedChild = this._parentWidget._children[0] ||
null;
On 2016/09/12 at 18:24:54, dgozman wrote:
> Just set to null here?

Done.

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:456: else {
On 2016/09/12 at 18:24:54, dgozman wrote:
> style: else on the same line as }

Done.

https://codereview.chromium.org/2319523004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Widget.js:627: while (node) {
On 2016/09/12 at 18:24:54, dgozman wrote:
> nit: I'd split this into two loops:
> 
> while (node && !node.__widget) node = node.parentNodeOrShadowHost();
> if (!node) return;
> while (widget._parentWidget) { ... }

Done.

[dgozman, 2016-09-12 18:44:48]

Code looks good, let's add a unit test. I think calling focusWidgetFromNode to
emulate focus and logging calls to focus would be a good coverage.

[einbinder, 2016-09-12 19:29:00]

Added a test.

[dgozman, 2016-09-12 21:21:14]

lgtm

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/widget-focus.html (right):

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:1: <html>
Let's place it under inspector/components.

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:10: var mainWidget =
new WebInspector.Widget();
Let's add SplitWidget into a mix as well to exercise it's
setDefaultFocusedChild.

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:49: function
dumpFocus(){
{ on next line

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:50: var focused =
WebInspector.currentFocusElement();
Should we check document.deepActiveElement to not rely on our own code?

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:58: else {
} else on the same line

[einbinder, 2016-09-12 22:19:05]

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/widget-focus.html (right):

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:1: <html>
On 2016/09/12 at 21:21:14, dgozman wrote:
> Let's place it under inspector/components.

Done.

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:10: var mainWidget =
new WebInspector.Widget();
On 2016/09/12 at 21:21:14, dgozman wrote:
> Let's add SplitWidget into a mix as well to exercise it's
setDefaultFocusedChild.

Done.

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:49: function
dumpFocus(){
On 2016/09/12 at 21:21:14, dgozman wrote:
> { on next line

Done.

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:50: var focused =
WebInspector.currentFocusElement();
On 2016/09/12 at 21:21:14, dgozman wrote:
> Should we check document.deepActiveElement to not rely on our own code?

document.deepActiveElement unfortunately isn't a thing (yet)

https://codereview.chromium.org/2319523004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/widget-focus.html:58: else {
On 2016/09/12 at 21:21:14, dgozman wrote:
> } else on the same line

Done.

[einbinder, 2016-09-12 22:48:00]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2016-09-12 22:48:00]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2319523004/#ps160001
(title: "Fix test")

[commit-bot: I haz the power, 2016-09-12 22:48:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 00:27:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-13 00:30:00]

Description was changed from

==========
DevTools: Remember the last focused widget.
When focusing a widget, try to focus the last focused child widget instead of
the first child widget.

BUG=646046
==========

to

==========
DevTools: Remember the last focused widget.
When focusing a widget, try to focus the last focused child widget instead of
the first child widget.

BUG=646046

Committed: https://crrev.com/1817d1b7a6958902adb487eac198c6d13d96a423
Cr-Commit-Position: refs/heads/master@{#418121}
==========

[commit-bot: I haz the power, 2016-09-13 00:30:01]

Patchset 9 (id:??) landed as
https://crrev.com/1817d1b7a6958902adb487eac198c6d13d96a423
Cr-Commit-Position: refs/heads/master@{#418121}