Harmony: Extension install dialogs |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Issue description
,
Apr 21 2016
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/145ffb972813fef174d6a96f2ca97983ede90c8a commit 145ffb972813fef174d6a96f2ca97983ede90c8a Author: ellyjones <ellyjones@chromium.org> Date: Tue Apr 26 14:56:52 2016 MacViews: add extension install dialog to MacViewsWebUIDialogs This dialog is now behind the MacViewsWebUIDialogs feature flag. BUG= 605657 Review URL: https://codereview.chromium.org/1918793004 Cr-Commit-Position: refs/heads/master@{#389779} [modify] https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a/chrome/browser/extensions/extension_install_prompt.h [modify] https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a/chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm [modify] https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a/chrome/chrome_browser_ui.gypi
,
Apr 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b39009a349414ee6c0ac184d8125ce82aee3ac24 commit b39009a349414ee6c0ac184d8125ce82aee3ac24 Author: ellyjones <ellyjones@chromium.org> Date: Thu Apr 28 15:56:22 2016 Revert of MacViews: add extension install dialog to MacViewsWebUIDialogs (patchset #2 id:20001 of https://codereview.chromium.org/1918793004/ ) Reason for revert: Breaks mac_views_browser=1 build. Original issue's description: > MacViews: add extension install dialog to MacViewsWebUIDialogs > > This dialog is now behind the MacViewsWebUIDialogs feature flag. > > BUG= 605657 > > Committed: https://crrev.com/145ffb972813fef174d6a96f2ca97983ede90c8a > Cr-Commit-Position: refs/heads/master@{#389779} TBR=sky@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 605657 Review-Url: https://codereview.chromium.org/1926023003 Cr-Commit-Position: refs/heads/master@{#390397} [modify] https://crrev.com/b39009a349414ee6c0ac184d8125ce82aee3ac24/chrome/browser/extensions/extension_install_prompt.h [modify] https://crrev.com/b39009a349414ee6c0ac184d8125ce82aee3ac24/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/b39009a349414ee6c0ac184d8125ce82aee3ac24/chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm [modify] https://crrev.com/b39009a349414ee6c0ac184d8125ce82aee3ac24/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/b39009a349414ee6c0ac184d8125ce82aee3ac24/chrome/chrome_browser_ui.gypi
,
May 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0acb4542a40e8d4c053302da1db1c39e067a3a93 commit 0acb4542a40e8d4c053302da1db1c39e067a3a93 Author: ellyjones <ellyjones@chromium.org> Date: Mon May 16 17:20:30 2016 MacViews: add extension install dialog to MacViewsWebUIDialogs This dialog is now behind the MacViewsWebUIDialogs feature flag. BUG= 605657 Review-Url: https://codereview.chromium.org/1977593002 Cr-Commit-Position: refs/heads/master@{#393854} [modify] https://crrev.com/0acb4542a40e8d4c053302da1db1c39e067a3a93/chrome/browser/extensions/extension_install_prompt.h [modify] https://crrev.com/0acb4542a40e8d4c053302da1db1c39e067a3a93/chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm [modify] https://crrev.com/0acb4542a40e8d4c053302da1db1c39e067a3a93/chrome/browser/ui/views/browser_dialogs_views.cc [modify] https://crrev.com/0acb4542a40e8d4c053302da1db1c39e067a3a93/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/0acb4542a40e8d4c053302da1db1c39e067a3a93/chrome/chrome_browser_ui.gypi
,
Aug 4 2016
Attaching current state at r409749 (inline install and webstore install)
,
Sep 23 2016
Review notes: https://groups.google.com/a/google.com/forum/#!topic/chrome-harmony/xqTrAqnJ-JE
,
Oct 7 2016
Here's the detailed spec: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec/Phase%202#%2FSPEC-Phase%202-Add%20extension.png%3Fz=width
,
Oct 8 2016
,
Oct 11 2016
Details collapsed, details expanded. There is one visual defect on details expanded: the scrollbar overdraws the dialog edge.
,
Jan 25 2017
,
Feb 3 2017
Just to note, the mocks have the app icon on the left hand side, but I think it's better the way it is in your screenshots. So please disregard.
,
Feb 6 2017
About the scroll bar: weirdly, it looks just fine on Linux, so I guess this is a Mac-specific problem to do with the overlaid scrollbars we use there. I will have a look.
,
Feb 7 2017
aside: is it confusing having 'View details' and 'Show Details' in the same dialog [with different casing]? do we want 'View details' -> 'View in WebStore' or similar? (note this only applies to "inline install" - install dialogs popped from the WebStore pages shouldn't have that 'View..' link at all). The scrollbar thing: looks like the scroll view just isn't being laid-out correctly? (i.e. it's too wide for the dialog, regardless of whether there's a scroller visible) (we could also try increasing the height of the dialog up to some limit upon clicking 'Show Details' to reduce the liklihood of requiring a scroll: Mac overlay scrollbars actually mean it can be subtle whether or not there are additional websites hiding "below the fold" - there may be security implications for this..)
,
Feb 7 2017
oh whoops the mocks actually do say 'Open in Webstore', but as link that is a dialog 'ExtraView` (i.e. in the bottom left of the dialog rather than below the star rating). So that's something else to address. (and it doesn't look like we're interested in modifying the dialog height - not sure how big an issue the overlay scrollbar thing is wrt hiding things below the fold).
,
Feb 8 2017
Screenshot after https://codereview.chromium.org/2683843004/ attached.
,
Feb 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/191eab692b8168b5d14058cde811edf8228e4532 commit 191eab692b8168b5d14058cde811edf8228e4532 Author: ellyjones <ellyjones@chromium.org> Date: Mon Feb 13 14:08:01 2017 harmony: update extension install dialog This change: 1) Moves the "View details" link down to the extra view slot in the dialog; 2) Updates the text of that link to "Open in Web Store" BUG= 605657 Review-Url: https://codereview.chromium.org/2683843004 Cr-Commit-Position: refs/heads/master@{#449946} [modify] https://crrev.com/191eab692b8168b5d14058cde811edf8228e4532/chrome/app/generated_resources.grd [modify] https://crrev.com/191eab692b8168b5d14058cde811edf8228e4532/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/191eab692b8168b5d14058cde811edf8228e4532/chrome/browser/ui/views/extensions/extension_install_dialog_view.h
,
Mar 10 2017
,
Mar 10 2017
,
Apr 19 2017
,
May 31 2017
,
Aug 9 2017
,
Sep 22 2017
Issue 654130 has been merged into this issue.
,
Sep 22 2017
Issue 766287 has been merged into this issue.
,
Sep 22 2017
,
Oct 6 2017
,
Oct 20 2017
Load balancing
,
Oct 23 2017
Please ignore my comment in #13. I'd like to see the icon on the left, following the original spec, to begin with. Bsep@, let me know if you need more clarification on specs - i know they're a bit old.
,
Nov 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3cafb9832350a066b1d3b0ba441dbd7ca362c4db commit 3cafb9832350a066b1d3b0ba441dbd7ca362c4db Author: Bret Sepulveda <bsep@chromium.org> Date: Sat Nov 11 02:22:26 2017 Add a BrowserDialogTest for various kinds of ExtensionInstallDialogView. This patch also includes some minor cleanup. Bug: 605657 Change-Id: Id7670c7743929cd638e25e0991cbfde2bc6fbb4d Reviewed-on: https://chromium-review.googlesource.com/759378 Commit-Queue: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#515800} [modify] https://crrev.com/3cafb9832350a066b1d3b0ba441dbd7ca362c4db/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc [add] https://crrev.com/3cafb9832350a066b1d3b0ba441dbd7ca362c4db/chrome/test/data/extensions/uitest/long_name/manifest.json
,
Nov 28 2017
Issue 782201 has been merged into this issue.
,
Dec 11 2017
Before/after screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/817952.
,
Dec 12 2017
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35ebe0e082116e13e001ff025a135e26a5523fcc commit 35ebe0e082116e13e001ff025a135e26a5523fcc Author: Bret Sepulveda <bsep@chromium.org> Date: Tue Dec 12 21:19:00 2017 Remove arrow on detailed permissions when installing an extension. As per the Harmony spec, the arrow is removed. This patch also includes related cleanup. Bug: 605657 Change-Id: Icdca8e14ee135578d57840cafc4397987262a141 Reviewed-on: https://chromium-review.googlesource.com/817952 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#523554} [modify] https://crrev.com/35ebe0e082116e13e001ff025a135e26a5523fcc/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/35ebe0e082116e13e001ff025a135e26a5523fcc/chrome/browser/ui/views/extensions/extension_install_dialog_view.h
,
Jan 3 2018
,
Jan 3 2018
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9d243c71965cd3a993dd727e00fb3ad8b009c4b commit f9d243c71965cd3a993dd727e00fb3ad8b009c4b Author: Bret Sepulveda <bsep@chromium.org> Date: Thu Jan 11 03:09:05 2018 Add tests for installing an extension with retained files and devices. I missed these cases when I first added interactive UI tests for the Extension Install dialog. Bug: 605657 Change-Id: Iac124f85eec41316aaa889c7978a8cffc3b5a1e8 Reviewed-on: https://chromium-review.googlesource.com/849534 Commit-Queue: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#528544} [modify] https://crrev.com/f9d243c71965cd3a993dd727e00fb3ad8b009c4b/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
,
Jan 18 2018
Harmony screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/841746
,
Jan 18 2018
Pre-harmony screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/841746
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5 commit 0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5 Author: Bret Sepulveda <bsep@chromium.org> Date: Wed Jan 24 20:25:26 2018 Rewrite extension install dialog layout for Harmony. This patch completely changes the layout: * The extension icon is now on the left. The icon, title, and webstore data are moved into BubbleFrameView's title area, which simplifies the layout. * The extension info section (permissions, etc) is now the width of the dialog, instead of having a fixed width. * The extension info section is no longer bulleted, instead it uses a secondary text style to distinguish lines from each header. Detailed permissions are still bulleted. Indentation in this section is removed. * Each block in the extension info section now creates a {header, content} representation that's handled in a standard way when creating the layout. * The animation when showing details is removed. The dialog will now expand to accommodate the details if it has room to do so. See screenshots in the associated bug. Bug: 605657 Change-Id: Ie6815038138e524ff053449ec393d6cc6c1cf956 Reviewed-on: https://chromium-review.googlesource.com/841746 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#531661} [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/chrome/browser/ui/views/extensions/extension_install_dialog_view.h [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/chrome/browser/ui/views/harmony/harmony_layout_provider.cc [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/ui/views/controls/message_box_view.cc [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/ui/views/layout/layout_provider.cc [modify] https://crrev.com/0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5/ui/views/layout/layout_provider.h
,
Jan 31 2018
Requesting merge of 0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5 in comment #40.
,
Jan 31 2018
This bug requires manual review: There is .grd file changes and we are only 33 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 31 2018
Thanks Bret! Change looks fairly large overall. Have you already tested this extensively in Canary and Dev? Are there enough automated tests for this as well? I guess we already have a kill switch to revert back to old pre-harmony dialogues so thats good.
,
Jan 31 2018
It is a large change, which is why I was hesitating to merge it. But it's been on Canary for a week with no reported problems. Since it's a UI change, it's not really possible to cover with automated tests. The patch changes the pre-Harmony UI as well. re #42: There are no .grd changes in the requested CL.
,
Jan 31 2018
The change is fairly isolated to the Extensions Dialogue, correct? Or is there any risk of it affecting other dialogues or UIs as well?
,
Jan 31 2018
It's completely isolated to the extensions dialog, yes... though that dialog can be accessed in some unexpected ways.
,
Jan 31 2018
Approving for merge to M65. Branch:3325. Agreed that this is a risky merge; however, let's ensure we test this extensively/thoroughly on Dev and Beta.
,
Jan 31 2018
,
Jan 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e224635572560b0a4019e328d93cb157ea182c46 commit e224635572560b0a4019e328d93cb157ea182c46 Author: Bret Sepulveda <bsep@chromium.org> Date: Wed Jan 31 23:02:17 2018 Rewrite extension install dialog layout for Harmony. This patch completely changes the layout: * The extension icon is now on the left. The icon, title, and webstore data are moved into BubbleFrameView's title area, which simplifies the layout. * The extension info section (permissions, etc) is now the width of the dialog, instead of having a fixed width. * The extension info section is no longer bulleted, instead it uses a secondary text style to distinguish lines from each header. Detailed permissions are still bulleted. Indentation in this section is removed. * Each block in the extension info section now creates a {header, content} representation that's handled in a standard way when creating the layout. * The animation when showing details is removed. The dialog will now expand to accommodate the details if it has room to do so. See screenshots in the associated bug. TBR=bsep@chromium.org (cherry picked from commit 0ea59b31b832031f9fdbce7e10e26f6e0c20dbe5) Bug: 605657 Change-Id: Ie6815038138e524ff053449ec393d6cc6c1cf956 Reviewed-on: https://chromium-review.googlesource.com/841746 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#531661} Reviewed-on: https://chromium-review.googlesource.com/896390 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#216} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/chrome/browser/ui/views/extensions/extension_install_dialog_view.h [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/chrome/browser/ui/views/harmony/harmony_layout_provider.cc [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/ui/views/controls/message_box_view.cc [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/ui/views/layout/layout_provider.cc [modify] https://crrev.com/e224635572560b0a4019e328d93cb157ea182c46/ui/views/layout/layout_provider.h
,
Feb 14 2018
Two blocking issues here: - remove close-x button on all centered dialogs - affirmative (blue) button should always be ADD, not CANCEL
,
Feb 14 2018
#50: I thought we decided that Cancel should be the default action because installing an extension is an important security decision. Am I remembering incorrectly?
,
Feb 14 2018
#50: The extension team requested we keep the close-x because there might be security implications. We can follow up with that post-launch. And Elly is correct, Cancel is blue because it's the default action (which also has security implications).
,
Mar 21 2018
This feedback is non-blocking to Harmony's launch, but we should either keep this bug open or start a new one. Who's the POC for the aforementioned security implications? The UX POV still remains: Re-ordering buttons to influence certain actions is strongly discouraged. Why do we encourage developers to create extensions and sponsor a webstore if our main call to action is to cancel said extensions? The preferred action to take is make ADD EXTENSION primary, CANCEL as secondary. An alternative is to make both buttons secondary (which is the current implementation for MacViews).
,
Mar 22 2018
+sdy - note #c50 and #c52 everyone else: Sidney's looking at Issue 823325 . Basically, it's super-weird for a window-modal dialog on Mac to have a close [x]. sdy: I don't think there's any doubt that we should remove it from the edit bookmark dialog (on all platforms), so perhaps we should start there. close [x] spec link: http://go/zlyko
,
Mar 22 2018
Cross-commenting from issue 823325 #c4: Would it be correct to make *no* close button the default for window-modal dialogs and make the extension install dialog explicitly have one*? --- * For now. I agree that having a close button at the top of a window-modal dialog is not a thing in Mac apps.
,
Mar 29 2018
MacViews triage: untagging Proj-MacViews from this.
,
Jul 25
This is as good as it's going to get for the Harmony effort. File a new, more specific bug if you'd like to see further UX improvements.
,
Jul 25
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec/Phase%202#%2FSPEC-Phase%202-Add%20extension.png%3Fz=width){kind=link}
Comment 1 by ellyjo...@chromium.org
, Apr 21 2016