MD WebUI: Improve tab focus behavior for toolbar search field

MD WebUI: Improve tab focus behavior for toolbar search field

In wide mode, ensure the input field gets focus rather than the search
icon when tabbing. The toolbar will only expand when the user starts
typing.

BUG=615318
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[tsergeant, 2016-05-30 03:15:57]

Description was changed from

==========
MD WebUI: Improve tab focus behavior for toolbar search field

In wide mode, ensure the input field gets focus rather than the search
icon when tabbing. The toolbar will only expand when the user starts
typing.

BUG=615318
==========

to

==========
MD WebUI: Improve tab focus behavior for toolbar search field

In wide mode, ensure the input field gets focus rather than the search
icon when tabbing. The toolbar will only expand when the user starts
typing.

BUG=615318
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-05-30 04:55:24]

tsergeant@chromium.org changed reviewers:
+ dpapad@chromium.org

[tsergeant, 2016-05-30 04:55:25]

dpapad@, PTAL.

(+calamity@ as FYI)

[dpapad, 2016-05-31 18:57:58]

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html
(right):

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:89:
on-keydown="onSearchTermKeydown_" on-keypress="onSearchTermKeypress_"
It is confusing that onSearchTermKeydown_ is a reference to a private method of
the behavior. Since this method is used outside the behavior, I think it should
be public.

Also why are we using both keypress and keydown events here and not just stick
with one of the two?

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:29: *
@param {boolean} narrow
@return missing

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:44: * it
can be used correctly by tab-focusing.
I am a bit skeptical about this behavior UX-wise. How would the user know that
she/he can start typing if the search field is not expanded? Should it be
expanded as soon as user focus it?

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:48: if
(!this.showingSearch)
Why is this check necessary? Could we blindly set this boolean to true? Or is
this to avoid triggiring bindings if the boolean is already true? I would hope
that Polymer is smart enough to not do unnecessary work when the new and old
value of a Polymer property is the same.

[tsergeant, 2016-06-01 01:20:49]

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html
(right):

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.html:89:
on-keydown="onSearchTermKeydown_" on-keypress="onSearchTermKeypress_"
On 2016/05/31 18:57:58, dpapad wrote:
> It is confusing that onSearchTermKeydown_ is a reference to a private method
of
> the behavior. Since this method is used outside the behavior, I think it
should
> be public.

Good point, done.
 
> Also why are we using both keypress and keydown events here and not just stick
> with one of the two?

Keydown is used in the behavior because it wants to pick up the Escape key,
which does not fire keypress events.

Keypress is used in this CL because it doesn't want to pick up non-text keys
like Esc, Tab, Shift, etc, which all fire keydown events but not keypress
events. I've added a comment to explain this.

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:29: *
@param {boolean} narrow
On 2016/05/31 18:57:58, dpapad wrote:
> @return missing

Done.

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:44: * it
can be used correctly by tab-focusing.
On 2016/05/31 18:57:58, dpapad wrote:
> I am a bit skeptical about this behavior UX-wise. How would the user know that
> she/he can start typing if the search field is not expanded? Should it be
> expanded as soon as user focus it?

There's a blinking cursor shown in the field when they tab-focus in, so they
will know they can type. We only expand (and do the corresponding animation)
when they type to avoid doing an animation in-and-out when they just want to tab
straight through the field (it's quite distracting when that happens).

This is very similar to the behavior in Inbox and Google Photos, both of which
change layout when the search field is clicked.

https://codereview.chromium.org/2025583002/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:48: if
(!this.showingSearch)
On 2016/05/31 18:57:58, dpapad wrote:
> Why is this check necessary? Could we blindly set this boolean to true? Or is
> this to avoid triggiring bindings if the boolean is already true? I would hope
> that Polymer is smart enough to not do unnecessary work when the new and old
> value of a Polymer property is the same.

Good point, done.

[dpapad, 2016-06-01 01:27:10]

lgtm

https://codereview.chromium.org/2025583002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2025583002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:45: * it
can be used correctly by tab-focusing. We use keypress instead of
Thanks for adding the explanatory comment, it helps.
Nit: Avoid using 'we' in comments, instead rephrase as follows

'keypress' is used instead of 'keydown' to avoid ....

[tsergeant, 2016-06-01 01:46:35]

https://codereview.chromium.org/2025583002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2025583002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:45: * it
can be used correctly by tab-focusing. We use keypress instead of
On 2016/06/01 01:27:10, dpapad wrote:
> Thanks for adding the explanatory comment, it helps.
> Nit: Avoid using 'we' in comments, instead rephrase as follows
> 
> 'keypress' is used instead of 'keydown' to avoid ....

Done.

[tsergeant, 2016-06-01 01:46:40]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-06-01 01:46:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2025583002/#ps60001
(title: "Tweak comment")

[commit-bot: I haz the power, 2016-06-01 01:47:11]

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

[commit-bot: I haz the power, 2016-06-01 04:04:44]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
MD WebUI: Improve tab focus behavior for toolbar search field

In wide mode, ensure the input field gets focus rather than the search
icon when tabbing. The toolbar will only expand when the user starts
typing.

BUG=615318
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD WebUI: Improve tab focus behavior for toolbar search field

In wide mode, ensure the input field gets focus rather than the search
icon when tabbing. The toolbar will only expand when the user starts
typing.

BUG=615318
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-06-01 04:06:30]

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