DevTools: rework focus logic

DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
Committed: https://crrev.com/d436b82c55a2d1d5ad217b59afd2744e1aa6fa77
Cr-Commit-Position: refs/heads/master@{#396955}

[luoe, 2016-05-27 00:48:28]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[luoe, 2016-05-27 00:48:29]

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar (I can't find this), DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus


Manual Test Plan (all of these should pass in patch 2):

Sources, open close GP should return focus to source
Sources, open close GP should focus on newly switched source
DeviceMode focus on size input, resize splitter, should keep focus in DeviceMode
DeviceMode focus on size input, open close CMenu should keep focus in DeviceMode
DeviceMode focus on size input, use CMenu to open a file should focus on source
DeviceMode open, search drawer focused, resizing splitter should restore focus
to search
Open Colorpicker, drag to adjust color, move mouse out of window, release;
pressing Esc and Enter should work
Open unfocusable drawer (rendering), opening search drawer via CMenu should
focus it
Opening CMenu twice shouldn’t trigger another CMenu (no flicker)

[luoe, 2016-05-27 00:53:48]

Description was changed from

==========
DevTools: rework focus logic

BUG=604427, 609013, 612397
==========

to

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar (I can't find this), DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

[luoe, 2016-05-27 00:54:14]

Description was changed from

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar (I can't find this), DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

to

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

[dgozman, 2016-05-27 01:06:00]

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:79:
WebInspector.Dialog._previousFocusedElement = WebInspector.currentFocusElement()
|| WebInspector.inspectorView.defaultFocusedElement();
Use WebInspector.Dialog._modalHostView instead of inspectorView.

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:98:
WebInspector.Dialog._previousFocusedElement.focus();
delete WebInspector.Dialog._previousFocusedElement

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:325: var
shouldFocus = this.hasFocus() || !WebInspector.currentFocusElement();
This looks strange. The logic here was preserving focus. If you want to focus
specific tabbed pane on selection, should do it on the callsite.

[lushnikov, 2016-05-27 01:14:44]

overall, feels good!

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:325: var
shouldFocus = this.hasFocus() || !WebInspector.currentFocusElement();
maybe focusing iff userGesture would work as well

[luoe, 2016-05-27 23:20:40]

Please take a look at Drawer.js for the added logic in showView().

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:79:
WebInspector.Dialog._previousFocusedElement = WebInspector.currentFocusElement()
|| WebInspector.inspectorView.defaultFocusedElement();
On 2016/05/27 01:05:59, dgozman wrote:
> Use WebInspector.Dialog._modalHostView instead of inspectorView.

Done.

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:98:
WebInspector.Dialog._previousFocusedElement.focus();
On 2016/05/27 01:05:59, dgozman wrote:
> delete WebInspector.Dialog._previousFocusedElement

Done.

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:325: var
shouldFocus = this.hasFocus() || !WebInspector.currentFocusElement();
On 2016/05/27 01:14:44, lushnikov wrote:
> maybe focusing iff userGesture would work as well

Almost, but it didn't cover all the cases.

https://codereview.chromium.org/2016963002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:325: var
shouldFocus = this.hasFocus() || !WebInspector.currentFocusElement();
On 2016/05/27 01:05:59, dgozman wrote:
> This looks strange. The logic here was preserving focus. If you want to focus
> specific tabbed pane on selection, should do it on the callsite.

Yeah, it is strange.  The condition has been reverted.  Since calling focus() on
show() is specific to drawer tabs, I added a focus() call in Drawer.showView().

[luoe, 2016-05-31 17:06:55]

The CQ bit was checked by luoe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-31 17:07:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016963002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016963002/80001

[commit-bot: I haz the power, 2016-05-31 18:50:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 18:50:28]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-05-31 19:30:42]

lgtm

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

https://codereview.chromium.org/2016963002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Drawer.js:78: this.focus();
I think you can call |this.focus()| right after |_innerShow| due to the focus
preserving logic in TabbedPane.

[luoe, 2016-05-31 21:22:54]

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

https://codereview.chromium.org/2016963002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Drawer.js:78: this.focus();
On 2016/05/31 19:30:42, dgozman wrote:
> I think you can call |this.focus()| right after |_innerShow| due to the focus
> preserving logic in TabbedPane.

This would cover most cases, but there is one case when calling focus() right
after innerShow doesn't work.  If we did that, the TabbedPane logic relies on
the assumption that after calling Drawer.focus(), the Drawer.hasFocus() == true
before TabbedPane.showTab() is called.  However, certain tabs (e.g. Rendering)
do not have a default focus element, so calling Drawer.focus() doesn't actually
focus the Drawer (hasFocus == false).

[luoe, 2016-05-31 21:24:14]

Do you think we should add a comment, making it clear that after Drawer.focus(),
it might be the case that Drawer.hasFocus() == false?

[luoe, 2016-05-31 22:29:07]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2016-05-31 22:29:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016963002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016963002/80001

[commit-bot: I haz the power, 2016-05-31 22:35:25]

Description was changed from

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

to

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

[commit-bot: I haz the power, 2016-05-31 22:35:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-31 22:36:49]

Description was changed from

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397
==========

to

==========
DevTools: rework focus logic

Summary of changes:
- We actually have 3 places where GlassPanes are instantiated [Dialogs, Options
in Toolbar, DraggingElements (e.g. Colorpicker)]
- DefaultFocusedViewStack is not necessary and is removed.  Previously, it was
only used by Dialogs anyways.  Dialogs are the only GlassPanes that will save
the previously focused element and call .focus() when the dialog closes
- Showing a different tab (switching to a drawer panel) will now focus on the
new tab, if no previous focus was set
- DeviceMode's toolbar previously depended on .setSelectionRange() to keep focus
in its size input field while resizing.  Now, dragging the splitter won't steal
focus at all, so DeviceMode doesn't need to force focus on itself
- When using the Command Menu to open a panel/file, it will close the GlassPane
first (which may call .focus() on the previous element) before calling .show()
on the new panel/file, so that the new panel/file gets the final focus

BUG=604427, 609013, 612397

Committed: https://crrev.com/d436b82c55a2d1d5ad217b59afd2744e1aa6fa77
Cr-Commit-Position: refs/heads/master@{#396955}
==========

[commit-bot: I haz the power, 2016-05-31 22:36:50]

Patchset 5 (id:??) landed as
https://crrev.com/d436b82c55a2d1d5ad217b59afd2744e1aa6fa77
Cr-Commit-Position: refs/heads/master@{#396955}