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

Issue 777564 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

[MD Extensions] toggling "allow in incognito" control causes flickering.

Project Member Reported by dpa...@chromium.org, Oct 23 2017

Issue description

1) 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).
 
allow_incognito_flicker.mp4
360 KB View Download
Cc: dschuyler@chromium.org
Labels: Hotlist-Polish OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Available (was: Untriaged)
It would be nice if we could eliminate the flicker.
Owner: dschuyler@chromium.org
Status: Started (was: Available)
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"
>       } ]

Cc: rdevlin....@chromium.org
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.
> 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.
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
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?
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.


inspect_views.png
3.8 KB View Download
inspect_views_no_active.png
3.6 KB View Download
> 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.
Owner: bettes@chromium.org
Status: Assigned (was: Started)
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).
Owner: dschuyler@chromium.org
Status: Started (was: Assigned)
Alan gave an okay (in chat) to #9.
Project Member

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

Status: Fixed (was: Started)
"fixed" in the sense described in #9.
Labels: TE-Verified-M65 TE-Verified-65.0.3291.0
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!
777564.mp4
602 KB View Download
Labels: Merge-Request-64
Labels: -Merge-Request-64
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