[MD Extensions] toggling "allow in incognito" control causes flickering. |
|||||||||
Issue description1) Open detail view 2) Change the state of the "allow in incognito" checkbox. See screencast (the flicker happens every time, but in the screencast it is not always noticeable because of the framerate).
,
Dec 6 2017
,
Dec 7 2017
The flickering appears to be caused by the extension info being update repeatedly. The multiple updates will also happen when toggling the "Allow access to File Urls" toggle.
Specifically, the extensions/renderer/dispatcher.cc(609) is called multiple times. Each call contains a base::Value with new info. Below I've pasted diffs from each of four updates.
As an example: the On/Off toggle at the top of the page is controlled by the "state" value in this update. That toggle begins "ENABLED". It get's "DISABLED" in the first update and set back to "ENABLED" in the third update -- causing the On/Off text to flicker in the UI.
$ diff ~/foo0 ~/foo1
1c1
< "event_type": "VIEW_UNREGISTERED",
---
> "event_type": "UNLOADED",
$ diff ~/foo1 ~/foo2
1c1
< "event_type": "UNLOADED",
---
> "event_type": "LOADED",
3c3,10
< "commands": [ ],
---
> "commands": [ {
> "description": "Activate the extension",
> "isActive": false,
> "isExtensionAction": true,
> "keybinding": "",
> "name": "_execute_browser_action",
> "scope": "CHROME"
> } ],
48c55
< "state": "DISABLED",
---
> "state": "ENABLED",
$ diff ~/foo2 ~/foo3
1c1
< "event_type": "LOADED",
---
> "event_type": "VIEW_REGISTERED",
60c60,67
< "views": [ ]
---
> "views": [ {
> "incognito": false,
> "isIframe": false,
> "renderProcessId": 9,
> "renderViewId": 2,
> "type": "EXTENSION_BACKGROUND_PAGE",
> "url": "chrome-extension://lpcaedmchfhocbbapmcbpinfpgnhiddi/_generated_background_page.html"
> } ]
,
Dec 7 2017
,
Dec 7 2017
These are good findings. I encountered similar findings when I refactored Service and Manager at https://chromium-review.googlesource.com/745903. Currently this function will trigger updateItem() for various types of C++ events.
,
Dec 7 2017
> Currently this function will trigger updateItem() for various types of C++ events. See https://cs.chromium.org/chromium/src/chrome/browser/resources/md_extensions/manager.js?l=207.
,
Dec 7 2017
Here's some *ideas* on reducing the flicker. This CLs are not ready for review and are intended to help discuss options: timeout before updating: https://chromium-review.googlesource.com/c/chromium/src/+/812432 ignore some updates: https://chromium-review.googlesource.com/c/chromium/src/+/813022
,
Dec 7 2017
So, this is kinda-sorta WAI. Changing whether the extension is allowed in incognito (or has file access) causes the extension to reload. When an extension reloads, it is disabled and then re-enabled. This enable -> disable -> enable cycle is what causes the "flicker". I'm not sure that this is something that we need to fix. I think the flicker can be useful to queue the user that something is happening (even if they don't fully understand why). Additionally, there's no guarantee about if the reload will succeed, or how long it will take. If the reload fails, the extension *will* be disabled, and we should show that disabled state. Similarly, if the reload takes awhile, it might be good to indicate to the user that it is disabled for that period of time. I think generally, having the UI reflect the "true" state of the extension is desirable, and don't see this as something we necessarily need to fix. If we were to tackle it, I think the right thing to do would actually be change the behavior of toggling settings to not require a reload, rather than trying to coerce the UI into trying to correctly predict the outcome (since ignoring the event will be incorrect in some cases, and timeouts can be flaky). Additionally, this behavior is present on the old page, and we decided not to fix it. See issue 466955 . Just my vote, but I'd be fine duping this into that. Anyone else have opinions?
,
Dec 7 2017
Thanks for the detailed response. Based on the corner cases about reloading take a while, or failing, I agree that we should not try to predict the outcome in the UI. Having said that, I think there is an easy (non-heuristic) UI tweak we can do to reduce the annoyingness of the flicker. It is mainly caused by the "inspect views" section disappearing, causing everything below it to shift upwards, the re-appearing and causing items to shift downwards again. How about we always have that section visible? See "inspect_views_no_active" screenshot for proposal.
,
Dec 7 2017
> How about we always have that section visible? Just to clarify, by "always" I mean, when developer mode is On. The section is hidden anyway when dev mode is Off, and the flicker is already not happening in such cases.
,
Dec 7 2017
Alan, could you give a SGTM to comments #9 and #10 (or not, if you prefer not to)? (Please change Owner back to me with your decision).
,
Dec 8 2017
Alan gave an okay (in chat) to #9.
,
Dec 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae36376292a9f75297ab557e2edae7d4c17f9c9c commit ae36376292a9f75297ab557e2edae7d4c17f9c9c Author: Dave Schuyler <dschuyler@chromium.org> Date: Sat Dec 09 03:42:30 2017 [MD extensions] reduce flicker when toggling in details This CL makes the Inspect Views section of the extension details page stay present when disabling the extension. This reduces the UI flicker when toggling extension options that cause the extension to be reloaded. Bug: 777564 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: I1e1307814943f546252e17c9b8515e05a4b30378 Reviewed-on: https://chromium-review.googlesource.com/818526 Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Commit-Queue: Dave Schuyler <dschuyler@chromium.org> Cr-Commit-Position: refs/heads/master@{#522974} [modify] https://crrev.com/ae36376292a9f75297ab557e2edae7d4c17f9c9c/chrome/app/md_extensions_strings.grdp [modify] https://crrev.com/ae36376292a9f75297ab557e2edae7d4c17f9c9c/chrome/browser/resources/md_extensions/detail_view.html [modify] https://crrev.com/ae36376292a9f75297ab557e2edae7d4c17f9c9c/chrome/browser/ui/webui/extensions/extensions_ui.cc
,
Dec 11 2017
"fixed" in the sense described in #9.
,
Dec 13 2017
Verified this issue on Windows-10, Ubuntu 14.04 and Mac OS 10.12.6 using chrome latest dev #65.0.3291.0 by following steps mentioned in the original comment, observed no flickering while toggling "allow in incognito" control. Hence adding TE-Verified label for M65. Thanks!
,
Dec 16 2017
,
Dec 16 2017
Removing Merge-Request-64 because this adds a string (and iiuc it's too late to add a string). |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by dpa...@chromium.org
, Dec 6 2017Labels: Hotlist-Polish OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Available (was: Untriaged)