New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 726684 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Regression: Weird behavior is seen after typing '++' in search of md-settings

Reported by sans...@etouch.net, May 26 2017

Issue description

Chrome 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

 
Actual_video.mp4
432 KB View Download
Expected_video.mp4
155 KB View Download
Cc: rbasuvula@chromium.org
Labels: hasbisect-per-revision
Owner: dpa...@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good build:56.0.2907.0 (Revision:429169).
Bad build:56.0.2908.0 (Revision:429486).

You are probably looking for a change made after 429464 (known good), but no later than 429465 (first known bad).

CHANGE-LOG URL:
---------------
https://chromium.googlesource.com/chromium/src/+log/be9e1f1ffc1e1d3287c7d994587446f9d7e703ee..63797594bc2bb93e645129b0c679081dc925ba5f

From the CL above, assigning the issue to the concern owner

@dpapad: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url:https://codereview.chromium.org/2449663002
Note :Able to reproduce the issue in Win 10.0,Ubuntu 14.04 & Mac 10.12.3 and Able to reproduce in latest Canary #60.0.3112.0

Comment 2 by dpa...@chromium.org, May 26 2017

Labels: -Pri-1 Hotlist-MD-Settings-SearchBox Pri-2
Status: Started (was: Assigned)

Comment 3 by dpa...@chromium.org, 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
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by dpa...@chromium.org, May 27 2017

Status: Fixed (was: Started)

Comment 6 by dpa...@chromium.org, May 31 2017

Labels: Merge-Request-60
Project Member

Comment 7 by sheriffbot@chromium.org, May 31 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
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
sansari@ Please verify the issue in latest canary.
dpapad@ Feel free to merge to 3112 branch once its verified.

Comment 9 by sans...@etouch.net, 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.
Fixed_video.mp4
224 KB View Download
Labels: TE-Verified-61.0.3117.0 TE-Verified-M61
As per above comment adding TE-verified labels
Fix has been verified, please merge to 3112 branch ASAP.
I am working on merging this.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 1 2017

Labels: -merge-approved-60 merge-merged-3112
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

Labels: TE-Verified-M60 TE-Verified-60.0.3112.20
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.
726684.ogv
456 KB View Download

Sign in to add a comment