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

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

Ensures that when the search value is changed by something other than
direct typing the search field will still open correctly. This fixes
issues with dragging or pasting text into the input field.

BUG=616740, 618633, 622074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7b28158033e882c541f6ff3dc0111b94f59a2ed0
Cr-Commit-Position: refs/heads/master@{#401527}

[tsergeant, 2016-06-22 07:02:01]

Description was changed from

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

Ensures that when the search value is changed by something other than
direct typing the search field will still open correctly. This fixes
issues with dragging or pasting text into the input field.

BUG=616740,618633,622074
==========

to

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

Ensures that when the search value is changed by something other than
direct typing the search field will still open correctly. This fixes
issues with dragging or pasting text into the input field.

BUG=616740,618633,622074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-06-22 07:21:14]

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

[tsergeant, 2016-06-22 07:21:14]

PTAL!

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

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:36:
'searchInput.bind-value-changed': 'onBindValueChanged_',
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>)?

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:70:
onBindValueChanged_: function() {
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:50]

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:36:
'searchInput.bind-value-changed': 'onBindValueChanged_',
On 2016/06/22 18:18:36, dpapad wrote:
> 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>)?

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.

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:70:
onBindValueChanged_: function() {
On 2016/06/22 18:18:36, dpapad wrote:
> 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_()?

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]

LGTM with nit. Thanks for fixing.

https://codereview.chromium.org/2090513002/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js
(right):

https://codereview.chromium.org/2090513002/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js:96:
assertTrue(field.hasSearchText_);
Accessing private variables from within a test ends up testing internal state
instead of behavior. This is sub-optimal, for example imagine a future change
where the data binding
hidden$="[[!hasSearchText_]]
is removed. The test would not break, but the behavior would be broken.

So a better assertion would be the following.
assertFalse(field.$.clearSearch.hidden);
which would break if behavior is broken. You might need to call
Polymer.dom.flush() before the assertion.

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js
(right):

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:36:
'searchInput.bind-value-changed': 'onBindValueChanged_',
On 2016/06/23 at 00:02:49, tsergeant wrote:
> On 2016/06/22 18:18:36, dpapad wrote:
> > 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>)?
> 
> 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.

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.

https://codereview.chromium.org/2090513002/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar_search_field.js:70:
onBindValueChanged_: function() {
I understand now, thanks. It was not clear to me initially that
onValueChanged_() is still being executed.

[tsergeant, 2016-06-23 01:00:34]

Thanks!

https://codereview.chromium.org/2090513002/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js
(right):

https://codereview.chromium.org/2090513002/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_toolbar_search_field_tests.js:96:
assertTrue(field.hasSearchText_);
On 2016/06/23 00:44:19, dpapad wrote:
> Accessing private variables from within a test ends up testing internal state
> instead of behavior. This is sub-optimal, for example imagine a future change
> where the data binding
> hidden$="[[!hasSearchText_]]
> is removed. The test would not break, but the behavior would be broken.
> 
> So a better assertion would be the following.
> assertFalse(field.$.clearSearch.hidden);
> which would break if behavior is broken. You might need to call
> Polymer.dom.flush() before the assertion.

Good point, done.

[tsergeant, 2016-06-23 01:00:42]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-06-23 01:00:42]

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/2090513002/#ps20001
(title: "Tweak test")

[commit-bot: I haz the power, 2016-06-23 01:02:24]

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

[commit-bot: I haz the power, 2016-06-23 02:54:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-23 02:55:40]

Description was changed from

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

Ensures that when the search value is changed by something other than
direct typing the search field will still open correctly. This fixes
issues with dragging or pasting text into the input field.

BUG=616740,618633,622074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

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

Ensures that when the search value is changed by something other than
direct typing the search field will still open correctly. This fixes
issues with dragging or pasting text into the input field.

BUG=616740,618633,622074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7b28158033e882c541f6ff3dc0111b94f59a2ed0
Cr-Commit-Position: refs/heads/master@{#401527}
==========

[commit-bot: I haz the power, 2016-06-23 02:55:41]

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