MD Settings: Search highlights can cause obsolete text to be displayed, in some cases. |
||||||||||||
Issue descriptionChrome Version: Canary (59.0.3069.0), older versions (58.0.3029.54) OS: Windows 10.1, haven't tested others What steps will reproduce the problem? (0) have default Downloads location setting (e.g., C:\Users\foo\Downloads) (1) chrome://md-settings (2) search "downloads (3) under Downloads, tap the "Change" button in the Location row (4) choose some other folder Expected: Location text updated to show new Downloads location. Actual: Nothing happens (but the setting *is* saved properly). I'd guess this is because searching for "downloads" fiddles with the structure of the text displaying the Location value, and it only seems to happen after searching, so assigning to dpapad.
,
Apr 13 2017
Another reasonable approach would be to not search the dynamic text that displays the selected download location, and side step the problem. This actually seems to be the approach of the old Options, see screenshot.
,
Apr 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f19ae8a724c71178e3ab46dd2e0955f72cd0adb6 commit f19ae8a724c71178e3ab46dd2e0955f72cd0adb6 Author: dpapad <dpapad@chromium.org> Date: Thu Apr 13 18:56:05 2017 MD Settings: Don't search dynamic download location text. This matches old Options behavior, and side steps the problem of showing an obsolete download location value, if the user changes download location while a search highlight is being displayed. BUG= 711156 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2808403009 Cr-Commit-Position: refs/heads/master@{#464484} [modify] https://crrev.com/f19ae8a724c71178e3ab46dd2e0955f72cd0adb6/chrome/browser/resources/settings/downloads_page/downloads_page.html
,
Apr 13 2017
Since the originally reported issue has been fixed, I am rephrasing this bug to reflect the underlying cause (described at comment#1).
,
Apr 17 2017
,
Oct 24 2017
Marking bugs (mostly lower priority ones) that I am unlikely to get to soon as Available.
,
Feb 5 2018
Issue 801309 has been merged into this issue.
,
Apr 3 2018
,
Apr 5 2018
Issue 735092 has been merged into this issue.
,
Apr 5 2018
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90a0340239b22badcac1a10665d4b2a21010c408 commit 90a0340239b22badcac1a10665d4b2a21010c408 Author: Esmael El-Moslimany <aee@chromium.org> Date: Fri Apr 06 05:52:30 2018 Settings WebUI: when text changes in settings, redo search Bug: 711156 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: Ic2847a86c7ec3671db79e524a25fa15c08193455 Reviewed-on: https://chromium-review.googlesource.com/996768 Reviewed-by: Hector Carmona <hcarmona@chromium.org> Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Cr-Commit-Position: refs/heads/master@{#548689} [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/browser/resources/settings/basic_page/basic_page.js [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/browser/resources/settings/downloads_page/downloads_page.html [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/browser/resources/settings/search_settings.js [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/browser/resources/settings/settings_main/settings_main.js [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/test/data/webui/settings/basic_page_browsertest.js [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/test/data/webui/settings/search_settings_test.js [modify] https://crrev.com/90a0340239b22badcac1a10665d4b2a21010c408/chrome/test/data/webui/settings/settings_main_test.js
,
Apr 6 2018
,
Apr 6 2018
FYI, I am re-opening this bug, since I am planning to revert the CL at #11. Reasons for revert 1) The followed approach for triggering a re-search as soon as the text is updated is problematic in some cases. See attached screencast where the UI is disappearing when the user makes an edit operation that triggers a re-search. This is suprising to the user, and does not seem the right approach. 2) Did not have a chance to review the CL before it landed. After looking at the CL, there are quite a few comments (some of which related to performance). I'd like to flesh out the solution more, before we attempt to re-land. Given the corner case at 1, I am thinking that the right approach is to *not* trigger a re-search. Instead, simply removing the highlight and letting the non-highlighted (original, non-obsolete) text node be displayed, should be sufficient.
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f71d99c312939088b91d17a4165c05110a45dc6d commit f71d99c312939088b91d17a4165c05110a45dc6d Author: Demetrios Papadopoulos <dpapad@chromium.org> Date: Fri Apr 06 19:14:16 2018 Revert "Settings WebUI: when text changes in settings, redo search" This reverts commit 90a0340239b22badcac1a10665d4b2a21010c408. Reason for revert: Caused corner cases where the UI disappears unexpectedly after the user interacts with it. Original change's description: > Settings WebUI: when text changes in settings, redo search > > Bug: 711156 > Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation > Change-Id: Ic2847a86c7ec3671db79e524a25fa15c08193455 > Reviewed-on: https://chromium-review.googlesource.com/996768 > Reviewed-by: Hector Carmona <hcarmona@chromium.org> > Commit-Queue: Esmael El-Moslimany <aee@chromium.org> > Cr-Commit-Position: refs/heads/master@{#548689} TBR=hcarmona@chromium.org,scottchen@chromium.org,aee@chromium.org Change-Id: Ia5b01260673e6a781f654b11f8a49bd222a91631 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 711156 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Reviewed-on: https://chromium-review.googlesource.com/998082 Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#548887} [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/browser/resources/settings/basic_page/basic_page.js [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/browser/resources/settings/downloads_page/downloads_page.html [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/browser/resources/settings/search_settings.js [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/browser/resources/settings/settings_main/settings_main.js [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/test/data/webui/settings/basic_page_browsertest.js [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/test/data/webui/settings/search_settings_test.js [modify] https://crrev.com/f71d99c312939088b91d17a4165c05110a45dc6d/chrome/test/data/webui/settings/settings_main_test.js
,
Apr 6 2018
Posting a similar case where the UI did disappear unexpectedly as soon as the user interacted with it (ui_disappearing_2.mp4). Also uploaded a proof-of-concept [1] for simply removing the highlights when the highlighted text changes. Note that the CL is not complete, since it does not remove mutation observers when search results are cleared. But it showcases the high level approach. For example see ui_updated_but_not_disappearing.mp4. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1000115
,
Apr 9 2018
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74bb270449d0350af88e974ff6b2b9d05ae9ce16 commit 74bb270449d0350af88e974ff6b2b9d05ae9ce16 Author: Esmael El-Moslimany <aee@chromium.org> Date: Fri Apr 13 07:37:02 2018 Settings WebUI: after settings search, remove text highlight when text changes Bug: 711156 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I45c797eb4b653a9ae1c5a181b855595b9853d89f Reviewed-on: https://chromium-review.googlesource.com/1002861 Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#550536} [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/browser/resources/settings/downloads_page/downloads_page.html [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/browser/resources/settings/search_settings.js [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/test/data/webui/settings/search_settings_test.js [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/ui/webui/resources/js/search_highlight_utils.js
,
Apr 13 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74bb270449d0350af88e974ff6b2b9d05ae9ce16 commit 74bb270449d0350af88e974ff6b2b9d05ae9ce16 Author: Esmael El-Moslimany <aee@chromium.org> Date: Fri Apr 13 07:37:02 2018 Settings WebUI: after settings search, remove text highlight when text changes Bug: 711156 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I45c797eb4b653a9ae1c5a181b855595b9853d89f Reviewed-on: https://chromium-review.googlesource.com/1002861 Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Cr-Commit-Position: refs/heads/master@{#550536} [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/browser/resources/settings/downloads_page/downloads_page.html [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/browser/resources/settings/search_settings.js [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/chrome/test/data/webui/settings/search_settings_test.js [modify] https://crrev.com/74bb270449d0350af88e974ff6b2b9d05ae9ce16/ui/webui/resources/js/search_highlight_utils.js |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by dpa...@chromium.org
, Apr 13 2017Cc: tbuck...@chromium.org
Labels: -Pri-1 Pri-2
94.6 KB
94.6 KB View Download