Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446413002:
MD Settings: Ignore whitespace only search queries. (Closed)
|
Created:
4 years, 1 month ago by dpapad Modified:
4 years, 1 month ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Ignore whitespace only search queries.
BUG=658922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d8fb849e33552296a15f3dc131c7580b8d94b9fc
Cr-Commit-Position: refs/heads/master@{#429949}
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 7
Patch Set 3 : Update CL after search URLs landed. #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== MD Settings: Ignore whitespace only search queries. BUG=658922 ========== to ========== MD Settings: Ignore whitespace only search queries. BUG=658922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:135: // TODO(dpapad): Should this be done by cr_search_field_behavior instead? MD History would probably benefit by moving this logic in cr_search_field_behavior. It will currently search for whitespace only strings which seems useless, see http://imgur.com/a/zl1di (notice the odd URL encoded spaces in the URL). https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:136: if (/^\s+$/.test(e.detail)) FWIW, I looked into Downloads, specifically at [1]. Could not find enough similarities to justify any re-use. For the MD Settings case, here we simply need to determine whether to trigger a search or not (by adding the URL parameter*), but no need to split to tokens. The more fine-grained input search query processing happens at a different layer at [2]. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/md_downloads/ac... [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search... *Note that this CL depends on the search URLs CL https://codereview.chromium.org/2449663002.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:135: // TODO(dpapad): Should this be done by cr_search_field_behavior instead? On 2016/10/25 23:38:41, dpapad wrote: > MD History would probably benefit by moving this logic in > cr_search_field_behavior. It will currently search for whitespace only strings > which seems useless, see http://imgur.com/a/zl1di (notice the odd URL encoded > spaces in the URL). can we just put the fix in a shared place? downloads, history, and extensions would all benefit
On 2016/10/26 at 02:10:03, dbeam wrote: > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): > > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/settings_ui/settings_ui.js:135: // TODO(dpapad): Should this be done by cr_search_field_behavior instead? > On 2016/10/25 23:38:41, dpapad wrote: > > MD History would probably benefit by moving this logic in > > cr_search_field_behavior. It will currently search for whitespace only strings > > which seems useless, see http://imgur.com/a/zl1di (notice the odd URL encoded > > spaces in the URL). > > can we just put the fix in a shared place? downloads, history, and extensions would all benefit Assuming that all of downloads,history,extensions are OK with the same behavior (whitespace-only queries not firing 'search-changed') we probably can, but I expect it will take longer.
On 2016/10/26 at 17:29:42, dpapad wrote: > On 2016/10/26 at 02:10:03, dbeam wrote: > > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): > > > > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/settings_ui/settings_ui.js:135: // TODO(dpapad): Should this be done by cr_search_field_behavior instead? > > On 2016/10/25 23:38:41, dpapad wrote: > > > MD History would probably benefit by moving this logic in > > > cr_search_field_behavior. It will currently search for whitespace only strings > > > which seems useless, see http://imgur.com/a/zl1di (notice the odd URL encoded > > > spaces in the URL). > > > > can we just put the fix in a shared place? downloads, history, and extensions would all benefit > > Assuming that all of downloads,history,extensions are OK with the same behavior (whitespace-only queries not firing 'search-changed') we probably can, but I expect it will take longer. Attempt to fix this for everyone at https://codereview.chromium.org/2454803002. There is a subtle behavior where the whitespace is removed from the textbox making the text jump to the left, see https://drive.google.com/file/d/0B8uNWwfvigW3b251VUNDb1NPbW8/edit (link only works with @google account). Do you think this is acceptable?
On 2016/10/26 19:16:58, dpapad wrote: > On 2016/10/26 at 17:29:42, dpapad wrote: > > On 2016/10/26 at 02:10:03, dbeam wrote: > > > > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > > > File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): > > > > > > > https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... > > > chrome/browser/resources/settings/settings_ui/settings_ui.js:135: // > TODO(dpapad): Should this be done by cr_search_field_behavior instead? > > > On 2016/10/25 23:38:41, dpapad wrote: > > > > MD History would probably benefit by moving this logic in > > > > cr_search_field_behavior. It will currently search for whitespace only > strings > > > > which seems useless, see http://imgur.com/a/zl1di (notice the odd URL > encoded > > > > spaces in the URL). > > > > > > can we just put the fix in a shared place? downloads, history, and > extensions would all benefit > > > > Assuming that all of downloads,history,extensions are OK with the same > behavior (whitespace-only queries not firing 'search-changed') we probably can, > but I expect it will take longer. > > Attempt to fix this for everyone at https://codereview.chromium.org/2454803002. > There is a subtle behavior where the whitespace is removed from the textbox > making the text jump to the left, see > https://drive.google.com/file/d/0B8uNWwfvigW3b251VUNDb1NPbW8/edit (link only > works with @google account). Do you think this is acceptable? no, it's not. that's the bug i was telling you about over chat
> no, it's not. that's the bug i was telling you about over chat Ok. In that case, we can't have CrSearchFieldBehavior communicate only the trimmed string to listeners, because listeners try to sync the URL search param with the 'search-changed' param and the textbox and this results in the user input abruptly changing, specifically for history this happens at https://cs.chromium.org/chromium/src/chrome/browser/resources/md_history/hist.... If we are still interested on fixing this for everyone, instead of transmitting the text exactly as entered by the user and have History, Settings etc do their own handling, how about transmitting two strings on 'search-changed'. displayedValue: The text as exactly entered by the user. This is what will show in the URL and the textbox. searchValue: The text that clients should use for the actual searching. Example: user searches for ' foo ' displayedValue: ' foo ' searchvalue: 'foo' To be honest, this gets quickly more complicated. When should a 'search-changed' event fired? If displayValue changes or only if searchValue changes? I will give it a shot, but let me know if you have thoughts.
i kinda think these CLs are in the wrong order https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (left): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:134: var query = e.detail; i think this entire patch could be - var query = e.detail; + var query = e.detail.trim(); https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:137: return; this code is wrong. consider moving from ' a' to ' ' in the search term (i.e. typing ' a' then pressing backspace). you'd want that to change the query
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, this CL has been adjusted after search URLs landed. https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (left): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:134: var query = e.detail; On 2016/10/26 at 20:51:11, Dan Beam wrote: > i think this entire patch could be > > - var query = e.detail; > + var query = e.detail.trim(); See latest patch for a very similar approach. https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2446413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:137: return; On 2016/10/26 at 20:51:11, Dan Beam wrote: > this code is wrong. consider moving from ' a' to ' ' in the search term (i.e. typing ' a' then pressing backspace). you'd want that to change the query Addressed in latest patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Ignore whitespace only search queries. BUG=658922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Ignore whitespace only search queries. BUG=658922 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d8fb849e33552296a15f3dc131c7580b8d94b9fc Cr-Commit-Position: refs/heads/master@{#429949} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d8fb849e33552296a15f3dc131c7580b8d94b9fc Cr-Commit-Position: refs/heads/master@{#429949} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
