Settings/Omnibox: leading whitespace in a keyword breaks a search engine |
||||
Issue descriptionChrome Version: 66.0.3350.0 dev (but everywhere, presumably) OS: (e.g. Win7, OSX 10.9.5, etc...) What steps will reproduce the problem? (1) Navigate to chrome://settings/searchEngines (2) Edit a search engine (3) Add a space to the beginning of a keyword (4) Try to use that keyword to activate that search engine What is the expected result? Probably just ignore the leading whitespace. What happens instead? I can't use that keyword until this is fixed (i.e. I can't manually type space + keyword nor just keyword). Please use labels and text to provide additional information. I think TemplateUrlData::keyword_ just needs a trim.
,
Feb 28 2018
dbeam@, I'd prefer to just merge this into the other bug you mention. Revising omnibox parsing code to deal with space when really we shouldn't be allowing them at all seems like a wrong direction.
,
Feb 28 2018
,
Feb 28 2018
I consulted a little with Dan on this. I think we should make this change irrespective of the other bug; doing so deals with spaces in keywords in the database. I think we should also fix the other bug by having the UI flag cases where people try to enter spaces in the middle of the keyword, but it should accept (and silently trim) leading/trailing whitespace. There's no need to do that there if we do it here, and if we don't do it here, we still have the issue of existing broken cases (or people who bypass the UI checks somehow).
,
Feb 28 2018
feel free to shuffle the bugs however you'd like; I care dramatically more about the prospective fix: https://chromium-review.googlesource.com/c/chromium/src/+/940454 I think trimming leading (and possibly trailing) whitespace is much easier to gather consensus on / fix than bug 6867.
,
Feb 28 2018
Trimming a keyword when creating it sounds good to me. Making the autocomplete system be smarter about dealing with existing keywords with spaces is more work (assuming you don't simply want to trim them all in the underlying database).
,
Feb 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9b688e8731a0aa30e9686cec098898c82c6a4e9 commit f9b688e8731a0aa30e9686cec098898c82c6a4e9 Author: Dan Beam <dbeam@chromium.org> Date: Wed Feb 28 03:04:20 2018 Trim Search Engine keywords This leaves original user input (from chrome://settings/searchEngines) untouched but effectively fixes a bug in which a user accidentally leaves a leading space while editing via Chrome's Settings UI and functionally breaks that search engine (typing \s + keyword doesn't work as far as I can tell). Bug: 817132 Change-Id: I7e34927fafc853b056e6a721e7f4e903e3652d4d Reviewed-on: https://chromium-review.googlesource.com/940454 Commit-Queue: Dan Beam (no longer on Chrome) <dbeam@chromium.org> Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#539683} [modify] https://crrev.com/f9b688e8731a0aa30e9686cec098898c82c6a4e9/components/search_engines/BUILD.gn [modify] https://crrev.com/f9b688e8731a0aa30e9686cec098898c82c6a4e9/components/search_engines/template_url_data.cc [add] https://crrev.com/f9b688e8731a0aa30e9686cec098898c82c6a4e9/components/search_engines/template_url_data_unittest.cc
,
Feb 28 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dbeam@chromium.org
, Feb 28 2018