Regression: Weird behavior is seen after typing '++' in search of md-settings
Reported by
sans...@etouch.net,
May 26 2017
|
||||||||
Issue descriptionChrome Version: 60.0.3112.0 (Official Build)b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}-32/64 bit OS: Windows(7,8,8.1,10), Linux(14.04 LTS), Mac(10.11.6, 10.12.3). Steps: 1. Launch Chrome and navigate to chrome://settings 2. Type '++'keys in search box and observe Actual: '++' disappears from search box and Setting page is visible below 'No search result found' Expected: '++' should appear in search box and Setting page should not be visible below 'No search result found' This is Regression issue broken in M-56, will soon bisect info. Good Build: 56.0.2907.0 Bad Build: 56.0.2908.0
,
May 26 2017
,
May 26 2017
Found the problem. When we place the query text in the URL, we don't URL encode it first. Then when we pull it out to perform the actual search, an empty string is returned, which causes the problem. A call to encodeURIComponent() is missing, similar to the old Options, https://cs.chromium.org/chromium/src/chrome/browser/resources/options/search_page.js?l=379
,
May 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b3779b51c2acb9f65812723406c28d72fb6e26f commit 7b3779b51c2acb9f65812723406c28d72fb6e26f Author: dpapad <dpapad@chromium.org> Date: Sat May 27 02:24:02 2017 MD Settings: URL encode text queries before adding them to the URL. Searching is triggered via currentRouteChanged, which was getting a wrong value from QuerySearchParams(), because the original query was not URL encoded first. BUG= 726684 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2905393002 Cr-Commit-Position: refs/heads/master@{#475221} [modify] https://crrev.com/7b3779b51c2acb9f65812723406c28d72fb6e26f/chrome/browser/resources/settings/basic_page/basic_page.js [modify] https://crrev.com/7b3779b51c2acb9f65812723406c28d72fb6e26f/chrome/browser/resources/settings/settings_main/settings_main.js [modify] https://crrev.com/7b3779b51c2acb9f65812723406c28d72fb6e26f/chrome/browser/resources/settings/settings_ui/settings_ui.js [modify] https://crrev.com/7b3779b51c2acb9f65812723406c28d72fb6e26f/chrome/test/data/webui/settings/settings_ui_browsertest.js
,
May 27 2017
,
May 31 2017
,
May 31 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 31 2017
sansari@ Please verify the issue in latest canary. dpapad@ Feel free to merge to 3112 branch once its verified.
,
Jun 1 2017
With response to comment #8, above issue is fixed on latest canary version: 61.0.3117.0 Please refer attached video. Thank you.
,
Jun 1 2017
As per above comment adding TE-verified labels
,
Jun 1 2017
Fix has been verified, please merge to 3112 branch ASAP.
,
Jun 1 2017
I am working on merging this.
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4 commit fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4 Author: Lei Zhang <thestig@chromium.org> Date: Thu Jun 01 22:50:20 2017 M60: MD Settings: URL encode text queries before adding them to the URL. Searching is triggered via currentRouteChanged, which was getting a wrong value from QuerySearchParams(), because the original query was not URL encoded first. BUG= 726684 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2905393002 Cr-Original-Commit-Position: refs/heads/master@{#475221} Review-Url: https://codereview.chromium.org/2921653003 . Cr-Commit-Position: refs/branch-heads/3112@{#102} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4/chrome/browser/resources/settings/basic_page/basic_page.js [modify] https://crrev.com/fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4/chrome/browser/resources/settings/settings_main/settings_main.js [modify] https://crrev.com/fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4/chrome/browser/resources/settings/settings_ui/settings_ui.js [modify] https://crrev.com/fd24bcc440ee14cc235b1409ce0ad42ef9b1a4f4/chrome/test/data/webui/settings/settings_ui_browsertest.js
,
Jun 6 2017
Tested the issue on Latest Dev# 60.0.3112.20 on Windows, Mac and Linux and found to be fixed. Search results are being displayed as expected. Hence adding TE-Verified labels. Attaching screen cast for reference. Thank You. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by rbasuvula@chromium.org
, May 26 2017Labels: hasbisect-per-revision
Owner: dpa...@chromium.org
Status: Assigned (was: Unconfirmed)