[Media Router WebUI] Fix escape bug in filter menu.

[Media Router WebUI] Fix escape bug in filter menu.

This change lets the Escape key exit the filter view when the keyboard focus is
on a menu item. Previously, the dialog would exit in this case.

BUG=594003
R=apacible@chromium.org

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

[btolsch, 2016-04-15 02:41:27]

One part that is still undecided for this change is what to do with the keyboard
focus when returning to the sink list view. When built in Debug mode, the
FocusController seems to complain (by crashing) because the currently focused
element doesn't exist. This isn't a problem when not in a Debug build. However,
this behavior already exists when pressing the back button with a search menu
item focused, so whatever we decide applies to that as well.

I think there are three options: do nothing (Debug crashes, Release wraps focus
to the header), focus the search input, or focus the same element in the sink
list that was just focused in the filter list. I will follow up with Jonny for
his opinion, but I also wanted to get your opinion. I would prefer the last
option.

[apacible, 2016-04-15 16:34:13]

On 2016/04/15 02:41:27, btolsch wrote:
> One part that is still undecided for this change is what to do with the
keyboard
> focus when returning to the sink list view. When built in Debug mode, the
> FocusController seems to complain (by crashing) because the currently focused
> element doesn't exist. This isn't a problem when not in a Debug build.
However,
> this behavior already exists when pressing the back button with a search menu
> item focused, so whatever we decide applies to that as well.
> 
> I think there are three options: do nothing (Debug crashes, Release wraps
focus
> to the header), focus the search input, or focus the same element in the sink
> list that was just focused in the filter list. I will follow up with Jonny for
> his opinion, but I also wanted to get your opinion. I would prefer the last
> option.

The third option sounds good. If there are no sinks or focus is not on the sinks
when ESC was pressed, where would the focus go?

[btolsch, 2016-04-15 19:35:29]

I fixed the focus logic to use the third option. PTAL, thanks!

[apacible, 2016-04-15 22:30:32]

Demo looks great!

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1022:
findFocusedSearchElement_: function() {
Do you plan on using findFocusedSearchElement_() and
giveKeyboardFocusToSinkListItem_() for something else? If not, you can just put
everything in fixKeyboardFocusOnFilterViewExit_().

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1032:
* same sink in the sink list.
Mention that if focus was not on a sink, whatever is currently focused will keep
focus.

https://codereview.chromium.org/1877233006/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_filter_tests.js
(right):

https://codereview.chromium.org/1877233006/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_filter_tests.js:164:
test('filter view escape key on menu item', function(done) {
Add a test if a sink was not focused (e.g. close button focused when pressed
ESC).

[btolsch, 2016-04-15 22:59:49]

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1022:
findFocusedSearchElement_: function() {
On 2016/04/15 22:30:32, apacible wrote:
> Do you plan on using findFocusedSearchElement_() and
> giveKeyboardFocusToSinkListItem_() for something else? If not, you can just
put
> everything in fixKeyboardFocusOnFilterViewExit_().

Done.

https://codereview.chromium.org/1877233006/diff/20001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1032:
* same sink in the sink list.
On 2016/04/15 22:30:32, apacible wrote:
> Mention that if focus was not on a sink, whatever is currently focused will
keep
> focus.

Done.

https://codereview.chromium.org/1877233006/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/media_router/media_router_container_filter_tests.js
(right):

https://codereview.chromium.org/1877233006/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/media_router/media_router_container_filter_tests.js:164:
test('filter view escape key on menu item', function(done) {
On 2016/04/15 22:30:32, apacible wrote:
> Add a test if a sink was not focused (e.g. close button focused when pressed
> ESC).

Done.

[apacible, 2016-04-15 23:12:17]

lgtm + nit

https://codereview.chromium.org/1877233006/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1877233006/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1024:
fixKeyboardFocusOnFilterViewExit_: function() {
nit: rename to something like "maybeUpdateFocusOnFilterViewExit_"?

[btolsch, 2016-04-15 23:38:37]

https://codereview.chromium.org/1877233006/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1877233006/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:1024:
fixKeyboardFocusOnFilterViewExit_: function() {
On 2016/04/15 23:12:17, apacible wrote:
> nit: rename to something like "maybeUpdateFocusOnFilterViewExit_"?

Done.

[btolsch, 2016-04-15 23:38:42]

The CQ bit was checked by btolsch@chromium.org

[btolsch, 2016-04-15 23:38:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1877233006/#ps60001
(title: "Rename focus update function")

[commit-bot: I haz the power, 2016-04-15 23:39:13]

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

[commit-bot: I haz the power, 2016-04-16 01:39:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-16 01:42:27]

Description was changed from

==========
[Media Router WebUI] Fix escape bug in filter menu.

This change lets the Escape key exit the filter view when the keyboard focus is
on a menu item. Previously, the dialog would exit in this case.

BUG=594003
R=apacible@chromium.org
==========

to

==========
[Media Router WebUI] Fix escape bug in filter menu.

This change lets the Escape key exit the filter view when the keyboard focus is
on a menu item. Previously, the dialog would exit in this case.

BUG=594003
R=apacible@chromium.org

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

[commit-bot: I haz the power, 2016-04-16 01:42:29]

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