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

Issue 681049 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-11-10
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug
Launch-M-Target: 64-Beta

Blocked on:
issue 683808

Blocking:
issue 603386


Participants' hotlists:
MacViews-Task-Queue


Sign in to add a comment

Harmony - update Enterprise Signin Confirmation dialog

Project Member Reported by tapted@chromium.org, Jan 13 2017

Issue description

Chrome Version       : 55.0.2883.95
OS Version: OS X 10.12.2

side-by-side for the CL in https://codereview.chromium.org/2625813003/ (Cocoa on the right Views/Harmony on the left)

I'm calling it the "Enterprise Signin Confirmation Dialog" (it's profile_signin_confirmation_dialog_* in code).

Once http://crrev.com/2625813003 lands, this can be invoked with

browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ProfileSigninConfirmationDialogTest.InvokeDialog_default --secondary-ui-md


Supporting CLs were needed to add better support for window modal sheets and cross-platform testing:
 - http://crrev.com/2534743002 MacViews: Harmony for TabDialogs' dialogs (starting with Collected Cookies) (landed r443130)
 - http://crrev.com/2629593005 MacViews: Support -[NSWindow close] on sheets.
 - http://crrev.com/2628373002 MacViews: Implement NativeWidgetMac::GetAllOwnedWidgets()
 - http://crrev.com/2625813003 MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used
 
Screen Shot 2017-01-12 at 12.05.44.png
345 KB View Download
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 13 2017

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

commit caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c
Author: tapted <tapted@chromium.org>
Date: Fri Jan 13 23:21:09 2017

MacViews: Support -[NSWindow close] on sheets.

Usually a sheet is closed with a call to endSheet:, which blocks the UI
thread to run the close animation, then runs the didEndSelector passed
to -[NSApp beginSheet:..]. Calling -[NSWindow close] bypasses this and
shouldn't be used. However, there are times where we need to close a
sheet more violently. E.g.: When -[NSWindow close] is invoked on the
parent window currently hosting a sheet, or during a dialog test that
wants to invoke the synchronous Widget::CloseNow() rather than
asynchronous Widget::Close(). Attempts to do this used to DCHECK().

Now, support -[NSWindow close] of a sheet by posting a task to
-[NSWindow endSheet:] on the parent when the sheet is closed. This has
tricky lifetime implications, but is necessary to ensure the modal
session on the parent window is unblocked.

BUG= 681049 

Review-Url: https://codereview.chromium.org/2629593005
Cr-Commit-Position: refs/heads/master@{#443718}

[modify] https://crrev.com/caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c/ui/views/cocoa/views_nswindow_delegate.mm
[modify] https://crrev.com/caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c/ui/views/cocoa/widget_owner_nswindow_adapter.mm
[modify] https://crrev.com/caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c/ui/views/widget/native_widget_mac.mm
[modify] https://crrev.com/caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c/ui/views/widget/native_widget_mac_unittest.mm

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 13 2017

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

commit 67b5583718d2ed35011af6d7468068e3c0237166
Author: tapted <tapted@chromium.org>
Date: Fri Jan 13 23:31:53 2017

MacViews: Support MODAL_TYPE_WINDOW in dialog tests.

This requires implementing NativeWidgetMac::GetAllOwnedWidgets(), and
ensuring that it (and GetAllChildWidgets()) includes window modal sheets
when the parent window is a native NSWindow rather than a views::Widget.

GetAllOwnedWidgets() is almost the same as GetAllChildWidgets(). It's
needed because client code needs to call GetAllOwnedWidgets() on other
platforms to pick up "transient" child windows, and it should have
similar behaviour on mac (just without the transients), rather than be
NOTIMPLEMENTED().

BUG= 681049 

Review-Url: https://codereview.chromium.org/2628373002
Cr-Commit-Position: refs/heads/master@{#443724}

[modify] https://crrev.com/67b5583718d2ed35011af6d7468068e3c0237166/ui/views/widget/native_widget_mac.mm
[modify] https://crrev.com/67b5583718d2ed35011af6d7468068e3c0237166/ui/views/widget/native_widget_mac_unittest.mm
[modify] https://crrev.com/67b5583718d2ed35011af6d7468068e3c0237166/ui/views/widget/widget_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 14 2017

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

commit 0dd1562593d3cb4ebe882801d76d747641d7b7f0
Author: tapted <tapted@chromium.org>
Date: Sat Jan 14 00:58:39 2017

MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. This uses the TestBrowserDialog framework, which was using
GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
but on Aura the dialog is added as a "transient" child. Update
TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
children as well as other child windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG= 681049 

Review-Url: https://codereview.chromium.org/2625813003
Cr-Commit-Position: refs/heads/master@{#443751}

[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.mm
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.mm
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller_browsertest.mm
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/tab_dialogs_cocoa.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/tab_dialogs_views_mac.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/cocoa/tab_dialogs_views_mac.mm
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/collected_cookies_browsertest.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/tab_dialogs.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/test/test_browser_dialog.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h
[add] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/views/tab_dialogs_views.cc
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/browser/ui/views/tab_dialogs_views.h
[modify] https://crrev.com/0dd1562593d3cb4ebe882801d76d747641d7b7f0/chrome/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 16 2017

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

commit c6f827f406ef2b71339fa34bc82a632519a84e5b
Author: pkalinnikov <pkalinnikov@chromium.org>
Date: Mon Jan 16 15:34:02 2017

Revert of MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used (patchset #7 id:140001 of https://codereview.chromium.org/2625813003/ )

Reason for revert:
The "ProfileSigninConfirmationDialogTest.InvokeDialog_default" test is failing on "Win10 Tests x64" builder.

Original issue's description:
> MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used
>
> The toolkit-views dialog will be used when --secondary-ui-md is enabled.
>
> (With that flag) The dialog also becomes window-modal on Mac, rather
> than tab-modal. This is consistent with other platforms. That's the easy
> bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".
>
> Adds a browser_test for showing the views dialog - it had no prior
> coverage. This uses the TestBrowserDialog framework, which was using
> GetAllChildWidgets() to detect a dialog being added. This worked on Mac,
> but on Aura the dialog is added as a "transient" child. Update
> TestBrowserDialog to use GetAllOwnedWidgets(), which includes transient
> children as well as other child windows.
>
> Also, there was a memory leak.
> OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
> SigninDialogDelegate with `new` which was never released. Cocoa tests in
> profile_signin_confirmation_view_controller_browsertest.mm didn't pick
> this up because the tests use the test harness as the delegate (passing
> `this`). The right fix is to pass a std::unique_ptr - do that.
>
> BUG= 681049 
>
> Review-Url: https://codereview.chromium.org/2625813003
> Cr-Commit-Position: refs/heads/master@{#443751}
> Committed: https://chromium.googlesource.com/chromium/src/+/0dd1562593d3cb4ebe882801d76d747641d7b7f0

TBR=msw@chromium.org,tapted@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 681049 

Review-Url: https://codereview.chromium.org/2632123003
Cr-Commit-Position: refs/heads/master@{#443899}

[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.mm
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.mm
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller_browsertest.mm
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/tab_dialogs_cocoa.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/tab_dialogs_views_mac.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/cocoa/tab_dialogs_views_mac.mm
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/collected_cookies_browsertest.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/tab_dialogs.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/test/test_browser_dialog.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h
[delete] https://crrev.com/ef7bded5e6fb6db4cdef36254e688e12b75eb7bb/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/views/tab_dialogs_views.cc
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/browser/ui/views/tab_dialogs_views.h
[modify] https://crrev.com/c6f827f406ef2b71339fa34bc82a632519a84e5b/chrome/test/BUILD.gn

Comment 5 by tapted@chromium.org, Jan 16 2017

https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds/7559

Errors like

ProfileSigninConfirmationDialogTest.InvokeDialog_default (run #1):
[ RUN      ] ProfileSigninConfirmationDialogTest.InvokeDialog_default
c:\c\win\src\chromerowser\ui	est	est_browser_dialog.cc(102): error: Value of: added.size()
  Actual: 0
Expected: 1u
Which is: 1
[  FAILED  ] ProfileSigninConfirmationDialogTest.InvokeDialog_default, where TypeParam =  and GetParam() =  (431 ms)


I have a theory that something to do with DWM is causing these to be added as "toplevel", but then failing to get associated as a transient child window. Which is probably a bad thing - we can "leak" windows that way.

Comment 6 by ew...@chromium.org, Jan 18 2017

Thanks for helping us move over to Harmony, Trent! Just to enumerate all the dialogues we have during the sign-in flow (that I'm aware of):

(1) Sign-in dialogue:
- Where you enter your credentials
- Web UI page served by GAIA
- Tab modal
(2) Enterprise sign in confirmation dialogue
- Shown when you are signing in with an enterprise account to an existing profile
- Currently views, being converted to Harmony in this bug
- Currently tab modal, being made window modal in this bug
(3) Create new person warning dialogue
- Shown when you sign into a profile that was previously syncing to a different account
- Currently MD web UI
- Currently tab modal
(4) Sync confirmation dialogue
- Shown for all successful new sign-ins to allow user to review sync/Narnia text and configure sync settings
- Currently MD web UI
- Currently browser modal
(5) Sign-in error dialogues
- Shown when an error is encountered during the sign-in flow
- Currently MD web UI
- Currently browser modal

Does it make sense to convert this single dialogue over to Harmony by itself? Should we be trying to move (2), (3), (4), and (5) all over to Harmony at the same time, or are we okay moving them one-by-one? These may be better questions for the Chrome UX team :)

Also, we probably should run the button ordering past UI review before landing this change. I think it's a little confusing that "Cancel" is next to "Create new profile" and "Link data" is off on its own.

Comment 7 by ew...@chromium.org, Jan 18 2017

Cc: ew...@chromium.org

Comment 8 by tapted@chromium.org, Jan 18 2017

The non-Harmony Windows dialog already has the "Link data" button off on its own (see attached).

Are we switching the MD webUI to Harmony webUI? (is that a thing?). One of the arguments for Harmony is that it is closer to the MD style we're already using for webui than the "Constrained window button" style we're currently using for this dialog on Mac. I.e. the Harmony version of this dialog is a closer match to the other sync dialogs as they are currently.

I don't think we're planning to launch any Harmony dialogs over by themselves. They will all switch with --secondary-ui-md so that the secondary UI is consistent. This dialog pops so rarely though due to Enterprise + first sign-in + other factors (especially on Mac - unless you're a Googler :). 
chrome2.png
85.8 KB View Download

Comment 9 by ew...@chromium.org, Jan 20 2017

Interesting, thanks for the heads up about the Windows dialogue.

I think our eventual goal is to move all of the dialogues described above (except the actual sign-in flow, which is served by GAIA) over to Harmony, for exactly the reasons you mentioned. Understood that we'll be switching over all secondary UI to Harmony at once by flipping the flag. Maybe it makes sense to just move over our sign-in-/sync-related dialogues one at a time (starting with this enterprise confirmation dialogue). Agreed that it's relatively low impact, since it's not shown very frequently :)
Blockedon: 683808
I've added this dialogue to row 25 in our tracker sheet. This doesn't have a mock attached to it, not knowing it'd be worked on, but this doesn't meet some of our basic harmony phase 1 expectations. Consider this feedback applicable to all Harmony dialogues you've have/will work on. 

I've annotated your original screenshots with comments. Feel free to assign this bug to me when you've addressed the following. 

1. wrong dialog width

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03a-dialog-metrics.png%3Fz=width

2. there's an unwanted drop shadow on the top of the dialogue
3. incorrect body text line-height and color

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-01-typography.png%3Fz=width

4. text is misaligned on the LHS
5. Remove the banner style and abide by vertical keylines 

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03b-dialog-keylines.png%3Fz=width

6. missing rounded corners

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03c-dialog-specs.png%3Fz=width

Screen Shot 2017-01-23 at 5.21.22 PM.png
213 KB View Download
**Expected dialogue width: 512pt
Nice! Thanks for the red lines.

2. and 6. might not be fixable on Mac, while this dialog is still Window Modal :/. Mac only supports Window Modal via this "native sheet" UI, which forces the top drop-shadow on us (and it will look strange without rounded corners and that top shadow). There's an associated animation too - you can try Bookmark Toolbar -> Edit on Canary after flipping chrome://flags/#secondary-ui-md

The rest should be fixable.

Comment 14 by ew...@chromium.org, Jan 25 2017

Thanks Alan, and thanks Trent for following up! We have the same drop shadow and missing rounded corners on our sync confirmation dialogue as well now, which has also been moved to a sheet :(

Trent, I spoke with Alan today - can we also make the "Create a new profile" button a (blue) primary action button, instead of having them all be secondary?
#c14: Can do! (although, anecdotally, I always pick "Link data" when this dialog pops for me ... maybe that's why my profile is such a mess).
Labels: OS-Chrome OS-Linux OS-Windows
Summary: Harmony - update Enterprise Signin Confirmation dialog (was: Convert Enterprise Signin Confirmation dialog to Harmony, use it on Mac)
Broadening to other OSes since this will affect non-Mac as well (it looks like much of the remaining work is general "Harmonize this views dialog" work)

Comment 17 by ew...@chromium.org, Jan 27 2017

Re #15: heh, yeah :) "Create new profile" is certainly the safer option (less risk of cross-sync issues and merging another person(a)'s local data into your corp account), so let's stick with that for now. Thanks Trent!
Project Member

Comment 18 by bugdroid1@chromium.org, Feb 10 2017

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

commit 3d35b33c364f4a5b8872b96b73dfa72f6ec64921
Author: tapted <tapted@chromium.org>
Date: Fri Feb 10 00:17:22 2017

[reland] MacViews: Allow the toolkit-views Enterprise Signin Confirmation Dialog to be used

Previously reviewed in https://codereview.chromium.org/2625813003.
Reverted due to Win10 failures  http://crbug.com/683808 . Fix in r447216.

The toolkit-views dialog will be used when --secondary-ui-md is enabled.

(With that flag) The dialog also becomes window-modal on Mac, rather
than tab-modal. This is consistent with other platforms. That's the easy
bit -- ProfileSigninConfirmationDialogViews::ShowDialog() "Just Works".

Adds a browser_test for showing the views dialog - it had no prior
coverage. Uses the fix in r447216 to detect these "transient" child
windows.

Also, there was a memory leak.
OneClickSigninSyncStarter::OnRegisteredForPolicy() allocated a
SigninDialogDelegate with `new` which was never released. Cocoa tests in
profile_signin_confirmation_view_controller_browsertest.mm didn't pick
this up because the tests use the test harness as the delegate (passing
`this`). The right fix is to pass a std::unique_ptr - do that.

BUG= 681049  Review-Url: https://codereview.chromium.org/2625813003

Review-Url: https://codereview.chromium.org/2667773003
Cr-Commit-Position: refs/heads/master@{#449485}

[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_dialog_cocoa.mm
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller.mm
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/profiles/profile_signin_confirmation_view_controller_browsertest.mm
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/tab_dialogs_cocoa.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/tab_dialogs_cocoa.mm
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/tab_dialogs_views_mac.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/cocoa/tab_dialogs_views_mac.mm
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/collected_cookies_browsertest.cc
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/sync/one_click_signin_sync_starter.cc
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/tab_dialogs.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h
[add] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/views/tab_dialogs_views.cc
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/browser/ui/views/tab_dialogs_views.h
[modify] https://crrev.com/3d35b33c364f4a5b8872b96b73dfa72f6ec64921/chrome/test/BUILD.gn

Cc: zkoch@chromium.org bettes@chromium.org
Zach was talking to me about how bad this dialogue is today. It's a gigantic wall of confusing text, there are three poorly-named action buttons (and an x-to-close), and there's no clear default options.

While we're moving it over to native Harmony, should we take a crack at improving the UI/strings? Alan, what do you think, should we loop in Shimi for help as well?

Also happy to file a separate bug for improving the UI/strings in this dialogue to keep this one focused on Harmonizing it.
The Harmony team is interested in changing styling for Harmony without changing more fundamental content/design.  It's OK for you to update those other things either before or after we do these dialog updates, but the Harmony group itself is bandwidth-limited.
Fair enough! I filed Issue 709687 to track improving the UX/strings of this dialogue.
Labels: -M-58
Labels: Launch-M-Target-64-Beta
NextAction: 2017-11-10
The NextAction date has arrived: 2017-11-10
Blocking: -662128
Status: Fixed (was: Started)
MacViews triage: fixing the look of this dialog is separately tracked in issue 678803. I'm going to mark this one Fixed.

Sign in to add a comment