MD History: Fix forward slash key not working in search field

MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

BUG=626996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

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

[lshang, 2016-07-15 01:13:40]

Description was changed from

==========
MD History: Fix forward slash key not working in search field


test not ready


some change


fix bug

BUG=
==========

to

==========
MD History: Fix forward slash key not working in search field


test not ready


some change


fix bug

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

[lshang, 2016-07-15 01:23:00]

Description was changed from

==========
MD History: Fix forward slash key not working in search field


test not ready


some change


fix bug

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

to

==========
MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a 
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is 
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

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

[lshang, 2016-07-15 01:34:10]

Description was changed from

==========
MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a 
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is 
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

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

to

==========
MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a 
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is 
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

BUG=626996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[lshang, 2016-07-15 01:42:34]

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

[lshang, 2016-07-15 01:42:35]

PTAL thanks! XD

[Dan Beam, 2016-07-15 03:07:40]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-07-15 03:07:41]

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:127: if
(this.$.toolbar.showingSearchField())
don't you actually want whether the search field is /focused/?

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:130: e.canExecute = true;
e.canExecute = !this.$.toolbar.showingSearchField();

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.html (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:74: <command id="slash-command"
shortcut="/">
just put this out of the <if>

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:104: 
nit: /** return {boolean} */

https://codereview.chromium.org/2152753003/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/2152753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:99:
showingSearchStatus: function() {
why is this needed if this.showingSearch is already public?

[tsergeant, 2016-07-15 04:06:21]

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:127: if
(this.$.toolbar.showingSearchField())
On 2016/07/15 03:07:40, Dan Beam wrote:
> don't you actually want whether the search field is /focused/?

Yeah, there can be cases where the field is showing but not focused: Click the
field, type in a word, then click outside the field. We'd still want the slash
command to work in those cases.

[lshang, 2016-07-15 05:02:09]

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:127: if
(this.$.toolbar.showingSearchField())
On 2016/07/15 04:06:21, tsergeant wrote:
> On 2016/07/15 03:07:40, Dan Beam wrote:
> > don't you actually want whether the search field is /focused/?
> 
> Yeah, there can be cases where the field is showing but not focused: Click the
> field, type in a word, then click outside the field. We'd still want the slash
> command to work in those cases.

Yep, good point!
I changed to regard forward slash key as normal input when the search field is
both showing and focused.

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:130: e.canExecute = true;
On 2016/07/15 03:07:40, Dan Beam wrote:
> e.canExecute = !this.$.toolbar.showingSearchField();

Done.

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history.html (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history.html:74: <command id="slash-command"
shortcut="/">
On 2016/07/15 03:07:41, Dan Beam wrote:
> just put this out of the <if>

Done.

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_toolbar.js (right):

https://codereview.chromium.org/2152753003/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_toolbar.js:104: 
On 2016/07/15 03:07:41, Dan Beam wrote:
> nit: /** return {boolean} */

Since we need to check both the search field is showing and focused, I deleted
this method and check them directly in onCanExecute().

https://codereview.chromium.org/2152753003/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/2152753003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:99:
showingSearchStatus: function() {
On 2016/07/15 03:07:41, Dan Beam wrote:
> why is this needed if this.showingSearch is already public?

Done. Deleted.

[Dan Beam, 2016-07-15 05:28:51]

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:128: /** @type {CrSearchFieldElement}
*/ this.$.toolbar.$['main-toolbar']
this type should technically be just "SearchField" I believe:
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search...

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:129: .getSearchField();
you should expose this through both HistoryToolbar:

get searchBar() {
  return this.$['main-toolbar'];
},

and CrSearchFieldBehavior, i.e.

isSearchFocused: function() {
  return this.$.searchInput.focused;
},

and do:

e.canExecute = !this.$.toolbar.searchBar.isSearchFocused();

and not poke into local DOM and stuff

[lshang, 2016-07-18 07:34:59]

Patchset #4 (id:60001) has been deleted

[Dan Beam, 2016-07-19 00:28:55]

https://codereview.chromium.org/2152753003/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js
(right):

https://codereview.chromium.org/2152753003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:106:
return this.$.searchInput.focused;
searchInput -> searchTerm may fix your issues

[lshang, 2016-07-19 01:16:25]

Patchset #5 (id:100001) has been deleted

[lshang, 2016-07-19 01:23:01]

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_history/app.js (right):

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:128: /** @type {CrSearchFieldElement}
*/ this.$.toolbar.$['main-toolbar']
On 2016/07/15 05:28:51, Dan Beam wrote:
> this type should technically be just "SearchField" I believe:
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search...

Or it should be "CrToolbarSearchFieldElement"? though this line is deleted in
the new patch since I directly use this.$.toolbar.searchBar.isSearchFocused()

https://codereview.chromium.org/2152753003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_history/app.js:129: .getSearchField();
On 2016/07/15 05:28:51, Dan Beam wrote:
> you should expose this through both HistoryToolbar:
> 
> get searchBar() {
>   return this.$['main-toolbar'];
> },
> 
> and CrSearchFieldBehavior, i.e.
> 
> isSearchFocused: function() {
>   return this.$.searchInput.focused;
> },
> 
> and do:
> 
> e.canExecute = !this.$.toolbar.searchBar.isSearchFocused();
> 
> and not poke into local DOM and stuff

Done.

https://codereview.chromium.org/2152753003/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js
(right):

https://codereview.chromium.org/2152753003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_search_field/cr_search_field_behavior.js:106:
return this.$.searchInput.focused;
On 2016/07/19 00:28:54, Dan Beam wrote:
> searchInput -> searchTerm may fix your issues

Done. Also an id is added to <paper-input-container> of cr-toolbar-search-field.

[Dan Beam, 2016-07-19 01:44:13]

lgtm if closure likes this

[tsergeant, 2016-07-19 03:34:13]

lgtm

[lshang, 2016-07-19 07:31:39]

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

[commit-bot: I haz the power, 2016-07-19 07:31:59]

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-07-19 07:32:00]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 09:32:10]

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

[commit-bot: I haz the power, 2016-07-19 09:32:11]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-07-20 00:11:15]

The CQ bit was checked by lshang@chromium.org

[commit-bot: I haz the power, 2016-07-20 00:12:33]

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-07-20 00:12:35]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-20 00:18:26]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-07-20 00:18:35]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 00:22:02]

Description was changed from

==========
MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a 
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is 
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

BUG=626996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD History: Fix forward slash key not working in search field

We previously added a shortcut of forward slash key to open and focus the search
field, but in this way when user inputs slash again it will be regarded as a
shortcut, not a normal input.

This CL fixes this by adding a check in onCanExecute(), if the search field is
already opened, then the shortcut command can not be executed so that the
forward slash key is regarded as normal input as search term.

BUG=626996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-07-20 00:22:03]

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