Fix some lingering WebUI focus issues.

Fix some lingering WebUI focus issues.

1) make (x) dialog buttons update whether focus outline is visible
2) don't restore focus when keyboard triggered dialogs are closed by mouse
3) make chrome://extensions overlays act more sanely
4) remove odd code that required activeElement != body to set initial focus

R=dmazzoni@chromium.org
BUG=472031, 481502
Committed: https://crrev.com/9a5d453e3874add8cf533d20100767369b76d2f8
Cr-Commit-Position: refs/heads/master@{#332887}

[Dan Beam, 2015-06-04 03:27:31]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-06-04 03:30:29]

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_commands_overlay.js (left):

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_commands_overlay.js:38:
this.handleDismiss_.bind(this));
this was short-circuiting the focus restoration code.

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/overlay.js (right):

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/overlay.js:101:
cr.dispatchSimpleEvent(e.currentTarget, 'focus', true);
FocusOutlineManager is not guaranteed to exist in all cases when this is run
(otherwise I'd probably do cr.ui.FocusManager.forDocument(document).update() or
something) and it'd be kind of a bummer to add an <include> just for this.

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (left):

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:500: if
(document.activeElement != document.body) {
i think this isn't relevant/desired any more

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:631:
currentPage.lastFocusedElement.focus();
i honestly don't know what was happening by keeping this around but it sounds
like a bad idea.  maybe i'll pay later for this...

[dmazzoni, 2015-06-04 03:34:56]

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_commands_overlay.js (right):

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_commands_overlay.js:38:
cr.dispatchEvent(overlay, 'cancelOverlay');
Just curious, is there a reason this is dispatchEvent and the others are
dispatchSimpleEvent?

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (left):

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:500: if
(document.activeElement != document.body) {
On 2015/06/04 03:30:29, Dan Beam wrote:
> i think this isn't relevant/desired any more

I think it is - a lot of the overlays are dialogs, and if you trigger opening
the overlay with the keyboard it's generally desirable to put focus inside the
overlay.

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:631:
currentPage.lastFocusedElement.focus();
On 2015/06/04 03:30:29, Dan Beam wrote:
> i honestly don't know what was happening by keeping this around but it sounds
> like a bad idea.  maybe i'll pay later for this...

Wait, isn't this the code that restores focus after you open an overlay and then
close it? I think we want this.

[Dan Beam, 2015-06-04 18:50:35]

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
File chrome/browser/resources/extensions/extension_commands_overlay.js (right):

https://codereview.chromium.org/1162293008/diff/20001/chrome/browser/resource...
chrome/browser/resources/extensions/extension_commands_overlay.js:38:
cr.dispatchEvent(overlay, 'cancelOverlay');
On 2015/06/04 03:34:56, dmazzoni wrote:
> Just curious, is there a reason this is dispatchEvent and the others are
> dispatchSimpleEvent?

Done. (and closure compiled for first time and actually tried this code)

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (left):

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:500: if
(document.activeElement != document.body) {
On 2015/06/04 03:34:56, dmazzoni wrote:
> On 2015/06/04 03:30:29, Dan Beam wrote:
> > i think this isn't relevant/desired any more
> 
> I think it is - a lot of the overlays are dialogs, and if you trigger opening
> the overlay with the keyboard it's generally desirable to put focus inside the
> overlay.

The code on the right does this *more*. 

Previously, something like the search bar needed to be focused for initial focus
of a dialog to be set. See the associated clearBrowserData on guest mode bug:
because the search input doesn't exist in guest mode the initial focus isn't
set.

https://codereview.chromium.org/1162293008/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:631:
currentPage.lastFocusedElement.focus();
On 2015/06/04 03:34:56, dmazzoni wrote:
> On 2015/06/04 03:30:29, Dan Beam wrote:
> > i honestly don't know what was happening by keeping this around but it
sounds
> > like a bad idea.  maybe i'll pay later for this...
> 
> Wait, isn't this the code that restores focus after you open an overlay and
then
> close it? I think we want this.
> 

We still restore focus. We just now actually clear lastFocusedElement after.

[dmazzoni, 2015-06-04 19:17:49]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-06-04 19:17:49]

lgtm

[commit-bot: I haz the power, 2015-06-04 19:18:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162293008/40001

[commit-bot: I haz the power, 2015-06-04 19:49:21]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-06-04 19:50:02]

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

[Dan Beam, 2015-08-11 02:31:41]

https://codereview.chromium.org/1162293008/diff/40001/chrome/browser/resource...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/1162293008/diff/40001/chrome/browser/resource...
chrome/browser/resources/extensions/extensions.js:398:
console.log('focusOutlineManager.visible', focusOutlineManager.visible);
^ oops