[NTP] Unnecessary 'Restore default shortcuts' option is enabled even none of the added shortcuts are present on NTP.
Reported by
dchau...@etouch.net,
Oct 18
|
|||
Issue descriptionChrome Version: 72.0.3583.0 (Official Build) Revision 6d94d291fef5dc66b71e3456371e2722c07f945b-refs/branch-heads/3583@{#1} (32/64-bit) OS: Windows (7, 8, 8.1, 10), Mac(10.13.1, 10.13.6, 10.14) & Linux(14.04 LTS). Pre-condition: Enable "Enable using the Google local NTP" and "New Tab Page Custom Links" flags from chrome://flags. What steps will reproduce the problem? 1. Launch Chrome, navigate to NTP and add the any shortcut from 'Add shortcut' icon. 2. Once shortcut is added, click on 'Edit shortcut' icon to open 'Edit shortcut' overlay and click on 'Remove' button. 3. Now click on gear icon at right bottom corner to open 'Customize this page' menu list and observe the 'Restore default shortcuts' option. Actual: Unnecessary 'Restore default shortcuts' option is enabled even none of the added shortcuts are present on NTP. Expected: 'Restore default shortcuts' option should be disabled when none of the added shortcuts are present on NTP. This is a non-regression issue, seen from M-70 series as 'Restore to default' option is introduced from build #70.0.3508.0 in 'Customize this page' menu list. NOTE: 1. This issue is also seen on Stable & Beta #70.0.3538.67 and Dev #71.0.3578.10 2. After Step-3: Nothing happens after clicking on 'Restore default shortcuts' option and it is still enabled after clicking on it. Kindly review the attached screen-cast for reference. Thank you.
,
Oct 19
This is not quite the same as the "undo after add" case, so I don't think custom links should be uninitialized here. @Yana, what do you think?
,
Oct 19
Noting that the separate issue indicated by this bug ("Restore default shortcuts" is not disabled after clicking) will be fixed.
,
Oct 19
Eghhh. It seems a tricky case. The end result is the same as the initial, but the user has in fact added the shortcut (even if they have removed it). It seems tracking this would be tricky. In general: how complex would it be for us to uninitialize the custom links if the user reverts to a state identical to what they'd have with most visited? Could we add a check where if the two states are the same?
,
Oct 19
It would be rather complicated, and keeping another set of tiles for the initial Most Visited (and continuously updating it in order to prevent staleness) adds extra overhead that I'd rather avoid.
,
Oct 19
That was my suspicion. Let's mark this as won't fix then (except for the "Restore default shortcuts" is not disabled after clicking issue).
,
Oct 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db8138f8711c4eb1ead267727e380ae75671fb1d commit db8138f8711c4eb1ead267727e380ae75671fb1d Author: Kristi Park <kristipark@chromium.org> Date: Mon Oct 22 17:06:37 2018 [NTP] Refresh the Most Visited tiles when the tile source changes The "Restore default shortcuts" menu option is not updated unless an onMostVisitedChanged event is sent. This only occurs when a URL/title of the new tile set is different from the current tiles. As such (after adding then removing a link), the restore default option will not be disabled after clicking since the tiles are considered unchanged. Now consider tiles with different sources to be unequal. Screencast: https://screencast.googleplex.com/cast/NTQ0MDY0MTk2MjA4MjMwNHw0MTY5NmRmYi1mNg Bug: 896566 Change-Id: I8c1474d2a567889fc5296c3a58228a92c3a67b71 Reviewed-on: https://chromium-review.googlesource.com/c/1292149 Commit-Queue: Kristi Park <kristipark@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#601619} [modify] https://crrev.com/db8138f8711c4eb1ead267727e380ae75671fb1d/chrome/browser/android/ntp/most_visited_sites_bridge.cc [modify] https://crrev.com/db8138f8711c4eb1ead267727e380ae75671fb1d/chrome/renderer/searchbox/searchbox.cc [modify] https://crrev.com/db8138f8711c4eb1ead267727e380ae75671fb1d/components/ntp_tiles/most_visited_sites.cc [modify] https://crrev.com/db8138f8711c4eb1ead267727e380ae75671fb1d/components/ntp_tiles/most_visited_sites.h [modify] https://crrev.com/db8138f8711c4eb1ead267727e380ae75671fb1d/components/ntp_tiles/most_visited_sites_unittest.cc
,
Oct 22
|
|||
►
Sign in to add a comment |
|||
Comment 1 by ramyan@chromium.org
, Oct 18Owner: kristip...@chromium.org