MD Search Field: Properly respond to external changes to search value.

ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // TODO(tsergeant): Add tests for cr-toolbar-search-field.
 Polymer({
   is: 'cr-toolbar-search-field',
 
   behaviors: [CrSearchFieldBehavior],
 
   properties: {
     narrow: {
       type: Boolean,
       reflectToAttribute: true,
     },
 
     // Prompt text to display in the search field.
     label: String,
 
     // Tooltip to display on the clear search button.
     clearLabel: String,
 
     // When true, show a loading spinner to indicate that the backend is
     // processing the search. Will only show if the search field is open.
     spinnerActive: {
       type: Boolean,
       reflectToAttribute: true
     },
+
+    /** @private */
+    hasSearchText_: Boolean,
   },
 
   listeners: {
     'tap': 'showSearch_',
+    'searchInput.bind-value-changed': 'onBindValueChanged_',

[dpapad, 2016/06/22 18:18:36]

I am a bit confused by the fact that this fix seems to be in
cr_toolbar_search_field.js and not in the underlying behavior. Doesn't that mean
that whoever uses the undrelying behavior still exhibits this bug (for example
<cr-search-field>)?

[tsergeant, 2016/06/23 00:02:49]

I made the changes here since the problems I'm trying to address in this patch
only apply to the <cr-toolbar-search-field>:

- You can't paste into a <cr-search-field> unless it's already showingSearch
- The clearSeach button in a <cr-search-field> is visible whenever showingSearch
is true, so there's no need to keep hasSearchText updated.

However, there's no particular reason why the changes couldn't live in the
behavior, so I'm happy to move them there if you prefer.

[dpapad, 2016/06/23 00:44:19]

Thanks for the explanation. Given that the canonical cr-search-field
implementation does not use hasSearchText, I agree that having it here as a
private variable is fine.

   },
 
   /**
    * @param {boolean} narrow
    * @return {number}
    * @private
    */
   computeIconTabIndex_: function(narrow) {
     return narrow ? 0 : -1;
   },
 
   /**
    * @param {boolean} spinnerActive
    * @param {boolean} showingSearch
    * @return {boolean}
    * @private
    */
   isSpinnerShown_: function(spinnerActive, showingSearch) {
     return spinnerActive && showingSearch;
   },
 
   /** @private */
   onInputBlur_: function() {
-    if (!this.hasSearchText)
+    if (!this.hasSearchText_)
       this.showingSearch = false;
   },
 
   /**
-   * Expand the search field when a key is pressed with it focused. This ensures
-   * it can be used correctly by tab-focusing. 'keypress' is used instead of
-   * 'keydown' to avoid expanding on non-text keys (shift, escape, etc).
+   * Update the state of the search field whenever the underlying input value
+   * changes. Unlike onsearch or onkeypress, this is reliably called immediately
+   * after any change, whether the result of user input or JS modification.
    * @private
    */
-  onSearchTermKeypress_: function() {
-    this.showingSearch = true;
+  onBindValueChanged_: function() {

[dpapad, 2016/06/22 18:18:36]

There is already an onValueChanged_() method responsible for updating all
internal state based on a change that is already reflected in the underlying
text field (except this.showingSearch) at
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search....

Why can't we re-use this method instead of partly repeating that logic here. How
is lastValue_ in the behavior updated if we don't call onValueChanged_()?

[tsergeant, 2016/06/23 00:02:49]

I've tried to allude to this in the comment, but we need to react to the value
changing at two different points:

Firstly, immediately after the value has changed to ensure that the UI is
updated (so the clearSearch button appears/disappears and the search field is
opened)

Secondly, after Chrome has batched updates to the input field and fired a search
event, we need to update the data model to reflect the new search.

The logic in the two different methods doesn't overlap all that much, since this
new one is just updating the UI. onValueChanged_() will still be called when the
input field fires its search event.

[dpapad, 2016/06/23 00:44:19]

I understand now, thanks. It was not clear to me initially that
onValueChanged_() is still being executed.

+    var newValue = this.$.searchInput.bindValue;
+    this.hasSearchText_ = newValue != '';
+    if (newValue != '')
+      this.showingSearch = true;
   },
 
   /**
    * @param {Event} e
    * @private
    */
   showSearch_: function(e) {
     if (e.target != this.$.clearSearch)
       this.showingSearch = true;
   },
 
   /**
    * @param {Event} e
    * @private
    */
   hideSearch_: function(e) {
     this.showingSearch = false;
     e.stopPropagation();
   }
 });