New issue
Advanced search Search tips

Issue 834494 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Extension Click-to-Script: Update wants-to-run UI

Project Member Reported by rdevlin....@chromium.org, Apr 18 2018

Issue description

New mocks include a different wants-to-run state and using an infobar instead of a bubble for the "refresh required" case.
 
Labels: Proj-Crx-Cts
Correction: we're still going to use the bubble, but it should only have one button (since it has an 'x' in the upper right) and the strings should be updated.
Project Member

Comment 4 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1af9c1fb44039d460bca5f5210b562e4c845acf

commit d1af9c1fb44039d460bca5f5210b562e4c845acf
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed May 02 03:19:53 2018

[Extensions Click-To-Script] Add a InvokeUi test for the refresh bubble

Add a test to invoke the refresh-required bubble for click-to-script.

Bug:  834494 
Change-Id: Ie58dc3f98699f3a2b54289a25078c5b414f18fb9
Reviewed-on: https://chromium-review.googlesource.com/1038661
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555299}
[add] https://crrev.com/d1af9c1fb44039d460bca5f5210b562e4c845acf/chrome/browser/ui/extensions/blocked_action_bubble_browsertest.cc
[modify] https://crrev.com/d1af9c1fb44039d460bca5f5210b562e4c845acf/chrome/test/BUILD.gn

Project Member

Comment 5 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01c4b1c4f2065c1e044d0191d5bed637abea49dd

commit 01c4b1c4f2065c1e044d0191d5bed637abea49dd
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed May 02 18:19:06 2018

[Extensions Click-to-Script] Update refresh-required bubble

New mocks use a bubble with title only (as opposed to title and body),
different strings, and no cancel button. Update the bubble accordingly,
and allow ToolbarActionsBarBubble* to accept a null body.

Bug:  834494 

Change-Id: Iea23483d894e001e23b14e4e7450120c21d6c87a
Reviewed-on: https://chromium-review.googlesource.com/1033320
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555470}
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/app/generated_resources.grd
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac.mm
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_mac_unittest.mm
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/extensions/blocked_action_bubble_delegate.cc
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/extensions/blocked_action_bubble_delegate.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/extensions/extension_message_bubble_bridge.cc
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/extensions/extension_message_bubble_bridge.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.cc
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/toolbar/toolbar_actions_bar_bubble_delegate.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h
[modify] https://crrev.com/01c4b1c4f2065c1e044d0191d5bed637abea49dd/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc

Cc: karandeepb@chromium.org pkasting@chromium.org
In-progress screenshots.  Note that these aren't quite as pretty since the test extension uses the auto-generated icon, and we haven't fully figured out the ink drop (which provides its own hover state).

Extension badge mocks: https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZXcV33V4QcZ_/files/MCHtA7U1iMGr6-XzcF1OM357Gg_ZVMR-hMU
wants_to_run-steady.png
1.7 KB View Download
wants_to_run-hovered.png
2.5 KB View Download
Yeah, you should draw this stuff programmatically.  I'm not aware of existing UI precisely like this that you could copy, though.  If these mocks didn't come from bettes@, can you make sure he's seen them and is OK with them?
Cc: bklmn@chromium.org bettes@chromium.org
+bklmn@ and bettes@.  Mocks are from bklmn@, and I believe he ran them by bettes@, but it's good to double-check.

Re programmatic drawing - sounds reasonable to me, but I'm not quite sure how to go about getting the shadow (and the associated blur) - is there any place we do something similar?
Look for code that calls CreateShadowDrawLooper(), e.g. ShadowBorder.  Also talk to bsep@ about how to translate the spec's shadow details into the right things in code, since he's working on that for refresh.
Project Member

Comment 10 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c

commit 4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Mon May 14 21:46:37 2018

[Extensions Click-To-Script] Update blocked actions UI

Update the UI for extensions that have had actions blocked on a page.
Instead of the old 'x' placeholder, use the new assets. These include a
"bubble" shown behind the extension icon with different colors for
steady, hover, and pressed states.

This won't work well with the ink drop that these buttons also use, but
it's undecided exactly what we want the behavior there to be. For now,
leave it as is.

On mac, this only uses the steady state (never hovered or pressed). This
is because there is a decent likelihood that MacViews will ship before
this project, so it is perhaps not worth the complexity of adding hooks
to be able to specify a hovered/pressed image.

Bug:  834494 

Change-Id: I5d02ee12df141d7d9c1d3ea79ded115af711caf2
Reviewed-on: https://chromium-review.googlesource.com/1037608
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558481}
[delete] https://crrev.com/f0189de8a2074fcfe88855c939404c100234fa0a/chrome/app/theme/default_100_percent/common/blocked_extension_script.png
[delete] https://crrev.com/f0189de8a2074fcfe88855c939404c100234fa0a/chrome/app/theme/default_200_percent/common/blocked_extension_script.png
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/app/theme/theme_resources.grd
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/cocoa/extensions/browser_action_button.mm
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/extensions/extension_action_view_controller.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/extensions/extension_action_view_controller.h
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/extensions/icon_with_badge_image_source.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/extensions/icon_with_badge_image_source.h
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/media_router_action.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/media_router_action.h
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/media_router_action_unittest.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/test_toolbar_action_view_controller.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/test_toolbar_action_view_controller.h
[add] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/toolbar_action_button_state.h
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/toolbar/toolbar_action_view_controller.h
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/views/toolbar/browser_actions_container.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/4f5af39d7e7fdf4948be2d250fd17d5dea25ff4c/chrome/browser/ui/views/toolbar/toolbar_action_view.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d634324fd9d4d3f06e914b0d6973f6c5670aae4

commit 1d634324fd9d4d3f06e914b0d6973f6c5670aae4
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Tue Jun 05 02:16:41 2018

[Extensions] Move ExtensionActionAPI updates to ExtensionActionRunner

Move updating the ExtensionActionAPI out of extensions::TabHelper and
into the ExtensionActionRunner. Each is a WebContentsObserver per tab
(the ExtensionActionRunner is owned by the TabHelper), but the
ExtensionActionRunner does a lot of other interaction with the
ExtensionActionAPI already. This eliminates the dependency of
TabHelper on the ExtensionActionAPI, and helps group similar logic
together.

This also fixes a race condition that could manifest with runtime
host permissions. The extension icon would be updated from the
TabHelper call into ExtensionActionAPI on WebContents navigation, but
the ExtensionActionRunner wouldn't have cleared its state. This resulted
in appearing that the extension still wanted to run, even if it didn't.

Add a browser test as a regression test for the above issue.

Bug:  834494 

Change-Id: I46f81da853d06d5f9fc3a104750081d176da6e55
Reviewed-on: https://chromium-review.googlesource.com/1079724
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564348}
[modify] https://crrev.com/1d634324fd9d4d3f06e914b0d6973f6c5670aae4/chrome/browser/extensions/extension_action_runner.cc
[modify] https://crrev.com/1d634324fd9d4d3f06e914b0d6973f6c5670aae4/chrome/browser/extensions/extension_action_runner.h
[modify] https://crrev.com/1d634324fd9d4d3f06e914b0d6973f6c5670aae4/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/1d634324fd9d4d3f06e914b0d6973f6c5670aae4/chrome/browser/extensions/tab_helper.h
[modify] https://crrev.com/1d634324fd9d4d3f06e914b0d6973f6c5670aae4/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc

Cc: -karandeepb@chromium.org rdevlin....@chromium.org
Owner: karandeepb@chromium.org
karandeepb@ has heroically accepted to take on some of the other work here.
Screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/1141352.

The icons with a white badge denote that the extension "wants to run".
wants_to_run.png
3.2 KB View Download
Thanks, karandeepb@! Can we also add badges with the new UI features flag flipped?
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faee83dfe6fb84d53ebd79432c12c619de9fa72d

commit faee83dfe6fb84d53ebd79432c12c619de9fa72d
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Thu Jul 19 06:20:59 2018

Extensions: Update blocked actions badge UI.

This CL updates the UI for extension icons in the browser toolbar that have had
actions blocked on a page. For such extension icons, a white circular badge is
shown behind the extension icon with surrounding shadows.

This removes the ToolbarActionButtonState enum added in r558481 and the
associated plumbing. This is an artifact of the fact that the the extension icon
UI does not depend on the button state now.

BUG= 834494 ,  859108 

Change-Id: I9da8963451ad4f7e09e539d57b96d9d3591d0ba5
Reviewed-on: https://chromium-review.googlesource.com/1141352
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576393}
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/cocoa/extensions/browser_action_button.mm
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/extensions/extension_action_view_controller.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/extensions/extension_action_view_controller.h
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/extensions/icon_with_badge_image_source.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/extensions/icon_with_badge_image_source.h
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/media_router_action.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/media_router_action.h
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/media_router_action_unittest.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/test_toolbar_action_view_controller.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/test_toolbar_action_view_controller.h
[delete] https://crrev.com/cde0f2f178a172fc31912545fc398df061ffffd3/chrome/browser/ui/toolbar/toolbar_action_button_state.h
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/toolbar/toolbar_action_view_controller.h
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/views/toolbar/browser_actions_container.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/faee83dfe6fb84d53ebd79432c12c619de9fa72d/chrome/browser/ui/views/toolbar/toolbar_action_view.h

Status: Fixed (was: Started)
Closing this out. Remaining improvements to the badging UI, if any can be tracked on other bugs.
See the attached screenshot comparison of the two themes.
The white circle background stands out too much in dark themes compared to the default white theme.
It unintentionally draws too much attention to extensions with wants-to-run highlight.

Is there any chance of calculating this color based on the theme's toolbar color?

@17 We've discussed whether we could do something like that a couple of times (particularly to at least accommodate common theme variations like dark mode and incognito - the themes framework is flexible enough that sometimes accounting for all of them, even with dynamic calculations, is very hard).  This is another good example of something that would benefit from that.  Can you file a new bug for it, and cc me?  That way, it won't get lost.

Sign in to add a comment