[DevTools] Rework some focus code.

[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.

BUG=none

Committed: https://crrev.com/c320a900cd8320048e1e1f553cdc01f2633c68b3
Cr-Commit-Position: refs/heads/master@{#424035}

[dgozman, 2016-09-30 02:40:54]

Description was changed from

==========
[DevTools] Remove WebInspector.setCurrentFocusElement.

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

to

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

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

[dgozman, 2016-09-30 02:42:01]

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

[dgozman, 2016-09-30 02:42:02]

Could you please take a look?

This passes all my manual tests. I will additionally test all the code I touched
before landing.

[dgozman, 2016-09-30 02:42:50]

Description was changed from

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

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

to

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.
Next step would be to disallow using activeElement in some way.

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

[dgozman, 2016-09-30 17:39:16]

Tested everything, a couple of fixes applied. Ready to land :-)

[luoe, 2016-09-30 18:25:36]

Neat!

One thing I didn't understand was where isConnected is being set.  How does it
get its value?

Other than that, lgtm

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

https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:833: * @return
{?Element}
Should this be @return {null}, since this fn() always returns null?

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

https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/View.js:458: }));
Can we separate out this function?

promises.push(this._view.widget().then(showAndFocus);
function showAndFocus(){
...
}

[einbinder, 2016-09-30 18:41:54]

On 2016/09/30 at 18:25:36, luoe wrote:
> Neat!
> 
> One thing I didn't understand was where isConnected is being set.  How does it
get its value?
> 
> Other than that, lgtm
> 
>
https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):
> 
>
https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:833: * @return
{?Element}
> Should this be @return {null}, since this fn() always returns null?
> 
>
https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/ui/View.js (right):
> 
>
https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/ui/View.js:458: }));
> Can we separate out this function?
> 
> promises.push(this._view.widget().then(showAndFocus);
> function showAndFocus(){
> ...
> }

Node.isConnected looks like some new Shadow DOM api:
https://github.com/w3c/webcomponents/issues/81

[einbinder, 2016-09-30 19:37:52]

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (left):

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:335:
this.focus();
Why does this get removed? Is the prompt still focused when the console is
shown?

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/FilterBar.js (left):

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/FilterBar.js:227:
this._filterInputElement.addEventListener("mousedown",
this._onFilterFieldManualFocus.bind(this), false); // when the search field is
manually selected
Why can this be removed? Or more precisely why was it here in the first place?

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:817: element = /**
@type {!Element} */ (toFocus);
Should we check if element has a widget and focus that?

[dgozman, 2016-09-30 20:27:19]

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

https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:833: * @return
{?Element}
On 2016/09/30 18:25:36, luoe wrote:
> Should this be @return {null}, since this fn() always returns null?

This will mess up with jscompiler, unfortunately.

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

https://codereview.chromium.org/2377193004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/View.js:458: }));
On 2016/09/30 18:25:36, luoe wrote:
> Can we separate out this function?
> 
> promises.push(this._view.widget().then(showAndFocus);
> function showAndFocus(){
> ...
> }

Good question. I feel like we are steering towards 8-10 lines arrow functions
being ok. Let's maybe discuss this with the team?

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (left):

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:335:
this.focus();
On 2016/09/30 19:37:52, einbinder wrote:
> Why does this get removed? Is the prompt still focused when the console is
> shown?

Yes, the proper way of hierarchical focus() calls does it.

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/FilterBar.js (left):

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/FilterBar.js:227:
this._filterInputElement.addEventListener("mousedown",
this._onFilterFieldManualFocus.bind(this), false); // when the search field is
manually selected
On 2016/09/30 19:37:52, einbinder wrote:
> Why can this be removed? Or more precisely why was it here in the first place?

This is just a strange piece of code. Things work manually.

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:817: element = /**
@type {!Element} */ (toFocus);
On 2016/09/30 19:37:52, einbinder wrote:
> Should we check if element has a widget and focus that? 

That would make things non-obvious. I'd rather keep it this way.

[pfeldman, 2016-09-30 22:29:03]

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/externs.js:46:
Node.prototype.isConnected;
Firefox:
No public signals
Edge:
No public signals
Safari:
No public signals

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/Main.js:670: var
searchableView =
WebInspector.SearchableView.fromElement(document.deepActiveElement()) ||
WebInspector.inspectorView.currentPanel().searchableView();
which document?

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:813: if (toFocus
instanceof WebInspector.Widget) {
Layering violation: UIUtils should not depend on Widgets.

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:831: * @param
{!Element|!WebInspector.Widget} fromFocused
Let's have two methods with different signatures.

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:835:
WebInspector.restoreFocusedElement = function(fromFocused, savedElement)
The order of attributes and the logic of the method is questionable as well. You
don't need the fromFocused, you need the frontFocued scope.

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/View.js:453: var shouldFocus =
this.element.hasFocus();
What is happening here?!?

[dgozman, 2016-09-30 23:28:25]

New stuff! PTAL

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/externs.js:46:
Node.prototype.isConnected;
On 2016/09/30 22:29:03, pfeldman wrote:
> Firefox:
> No public signals
> Edge:
> No public signals
> Safari:
> No public signals

Removed for now :-)

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

https://codereview.chromium.org/2377193004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/View.js:453: var shouldFocus =
this.element.hasFocus();
On 2016/09/30 22:29:03, pfeldman wrote:
> What is happening here?!?

Added a comment.

[dgozman, 2016-09-30 23:28:34]

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

[commit-bot: I haz the power, 2016-09-30 23:29:16]

Dry run: 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-10-01 00:38:58]

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

[commit-bot: I haz the power, 2016-10-01 00:38:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2016-10-03 18:30:00]

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

[commit-bot: I haz the power, 2016-10-03 18:31:06]

Dry run: 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-10-03 19:43:40]

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

[commit-bot: I haz the power, 2016-10-03 19:43:41]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-10-03 22:01:46]

lgtm

[dgozman, 2016-10-04 20:21:32]

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

[commit-bot: I haz the power, 2016-10-04 20:22:11]

Dry run: 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-10-04 21:27:27]

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

[commit-bot: I haz the power, 2016-10-04 21:27:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2016-10-05 02:33:53]

Description was changed from

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.
Next step would be to disallow using activeElement in some way.

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

to

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.

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

[dgozman, 2016-10-07 22:12:41]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-10-07 22:12:42]

The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org
Link to the patchset: https://codereview.chromium.org/2377193004/#ps40001
(title: "FocusRestorer")

[commit-bot: I haz the power, 2016-10-07 22:13:04]

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-10-08 01:33:15]

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

[commit-bot: I haz the power, 2016-10-08 01:33:17]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-10-08 01:33:57]

Description was changed from

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.

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

to

==========
[DevTools] Rework some focus code.

- Removed WI.currentFocusElement() and friends;
- Fixed TabbedLocation lazy loading focus;
- Focusing drawer tabs conditionally;
- Added primitives to DOMExtension.js.

This doesn't touch last focused widget logic.

BUG=none

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

[commit-bot: I haz the power, 2016-10-08 01:33:58]

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