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

Issue 864549 link

Starred by 1 user
Status: Fixed
Owner:
█ xiaoyinh@chromium.org
Last visit > 30 days ago
Closed: Aug 2
Cc:
jdufault@chromium.org
isandrk@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

The new login screen doesn't fully support public sessions

Project Member Reported by isandrk@chromium.org, Jul 17 
Hi Sarah, I've assigned this bug to you since you wrote the code in ash/login/ui/login_expanded_public_account_view.cc.

In the old webui version of the public session login screen, clicking on "Learn more" button showed a modal dialog containing some info.  In the new version this button does nothing.  This breaks the privacy/security story around public sessions.

I've attached screenshots showing the two versions.
Screenshot from 2018-07-17 15-54-41.png
44.3 KB View Download
Screenshot from 2018-07-17 15-55-17.png
41.4 KB View Download

Comment 1 by xiaoyinh@chromium.org, Jul 17

Cc: jdufault@chromium.org
Labels: M-69
Status: Assigned (was: Untriaged)

Comment 2 by isandrk@chromium.org, Jul 17 

Btw, has the new login screen already launched?  If not, what release do you target?

I'm about to change some small bits of it (public session related), and I'm wondering whether I should work on the old one or the new one.

Comment 3 by xiaoyinh@chromium.org, Jul 17 

The new login screen is launching in M-69, and all the new UIs are located in ash/login/ui, I think it's preferred to make new changes in the views login.

jdufault@, do we want to keep webui consistent with views login at this time? i.e. make changes to both views and webui login.

Comment 4 by jdufault@chromium.org, Jul 17 

It's been enabled on 68 dev and 69 dev, we're planning to launch in 69. If possible I'd recommend deferring the public session changes to 70 and doing the work only in views-login, as the webui work will be very short-lived.

Comment 5 by isandrk@chromium.org, Jul 17 

> I'd recommend deferring the public session changes to 70 and doing the work only in views-login
Ok sounds good as I'm targeting M70 anyway.

Another one - is there someone who I can chat up with eventual questions I might have?

Comment 6 by jdufault@chromium.org, Jul 17 

I general I recommend looking at the blame list for the specific file in question to find the right person to contact, but feel free to reach out to me.

Comment 7 by xiaoyinh@chromium.org, Jul 20

Status: Started (was: Assigned)

Comment 8 by xiaoyinh@chromium.org, Jul 24

public_session_warning.png
51.7 KB View Download

Comment 9 by isandrk@chromium.org, Jul 25 

Is that the new modal dialog?  Looks great!

One thing - can you make the title text bold if that's possible to do (The device admin may monitor the following)?

P.S. How do you get that output in the upper left corner?  That looks kinda useful :-)

Comment 10 by xiaoyinh@chromium.org, Jul 25 

Re#9: Yeah, unlike html title, by default the widget window title is not bold, I will try to see if I customize it...

Try this switch to show the  debug UI in the upper left corner:
--show-login-dev-overlay

Comment 11 by xiaoyinh@chromium.org, Jul 25 

https://chromium-review.googlesource.com/c/chromium/src/+/1145864 PS4 should make the title text bold:
ps-warning2.png
51.8 KB View Download

Comment 12 by isandrk@chromium.org, Jul 26 

Looks good to me!  Thank you :-)
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 2 

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

commit 5df2779de64821ff0d764a04df2bf38a46e7c34b
Author: Sarah Hu <xiaoyinh@chromium.org>
Date: Thu Aug 02 00:43:34 2018

cros: Add warning dialog in public account view

Bug:  864549 
Change-Id: If79c0100067c62a9fedb3119f43813a87d5bd6c6
Reviewed-on: https://chromium-review.googlesource.com/1145864
Commit-Queue: Xiaoyin Hu <xiaoyinh@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580012}
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/BUILD.gn
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings.grd
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO.png.sha1
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_1.png.sha1
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_2.png.sha1
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_3.png.sha1
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_4.png.sha1
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/login/ui/login_expanded_public_account_view.cc
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/login/ui/login_expanded_public_account_view.h
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/login/ui/login_expanded_public_account_view_unittest.cc
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/login/ui/public_account_warning_dialog.cc
[add] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ash/login/ui/public_account_warning_dialog.h
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ui/views/controls/styled_label.cc
[modify] https://crrev.com/5df2779de64821ff0d764a04df2bf38a46e7c34b/ui/views/controls/styled_label.h

Comment 14 by xiaoyinh@chromium.org, Aug 2

Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 2 

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

commit 566e91d9993a991cc20d1ba6802261a34ff4fa8b
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Thu Aug 02 02:19:21 2018

Revert "cros: Add warning dialog in public account view"

This reverts commit 5df2779de64821ff0d764a04df2bf38a46e7c34b.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 580012 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzVkZjI3NzlkZTY0ODIxZmYwZDc2NGEwNGRmMmJmMzhhNDZlN2MzNGIM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/7101

Sample Failed Step: compile

Original change's description:
> cros: Add warning dialog in public account view
> 
> Bug:  864549 
> Change-Id: If79c0100067c62a9fedb3119f43813a87d5bd6c6
> Reviewed-on: https://chromium-review.googlesource.com/1145864
> Commit-Queue: Xiaoyin Hu <xiaoyinh@chromium.org>
> Reviewed-by: Jacob Dufault <jdufault@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580012}

Change-Id: Ib78a72feb7bc149a17fdef9865aa4fdffa669d2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  864549 
Reviewed-on: https://chromium-review.googlesource.com/1159802
Cr-Commit-Position: refs/heads/master@{#580031}
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ash/BUILD.gn
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ash/ash_strings.grd
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO.png.sha1
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_1.png.sha1
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_2.png.sha1
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_3.png.sha1
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_4.png.sha1
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ash/login/ui/login_expanded_public_account_view.cc
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ash/login/ui/login_expanded_public_account_view.h
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ash/login/ui/login_expanded_public_account_view_unittest.cc
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/login/ui/public_account_warning_dialog.cc
[delete] https://crrev.com/a8c8396fd20d98666d517c45b358c63736e345ef/ash/login/ui/public_account_warning_dialog.h
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ui/views/controls/styled_label.cc
[modify] https://crrev.com/566e91d9993a991cc20d1ba6802261a34ff4fa8b/ui/views/controls/styled_label.h
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 2 

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

commit 5e12952230105c513169a24b8c2f0671cce7f6a3
Author: Sarah Hu <xiaoyinh@chromium.org>
Date: Thu Aug 02 20:43:57 2018

Reland: cros: Add warning dialog in public account view

This fix the linux-chromeos-dbg compile error by export class
PublicAccountWarningDialog.

TBR=xiyuan@chromium.org,msw@chromium.org

Bug:  864549 
Change-Id: Ibf7e0b14a17460114563bf40bc562294b86993cb
Reviewed-on: https://chromium-review.googlesource.com/1145864
Commit-Queue: Xiaoyin Hu <xiaoyinh@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580012}
Reviewed-on: https://chromium-review.googlesource.com/1161082
Cr-Commit-Position: refs/heads/master@{#580307}
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/BUILD.gn
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings.grd
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO.png.sha1
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_1.png.sha1
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_2.png.sha1
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_3.png.sha1
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/ash_strings_grd/IDS_ASH_LOGIN_PUBLIC_ACCOUNT_MONITORING_INFO_ITEM_4.png.sha1
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/login/ui/login_expanded_public_account_view.cc
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/login/ui/login_expanded_public_account_view.h
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/login/ui/login_expanded_public_account_view_unittest.cc
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/login/ui/public_account_warning_dialog.cc
[add] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ash/login/ui/public_account_warning_dialog.h
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ui/views/controls/styled_label.cc
[modify] https://crrev.com/5e12952230105c513169a24b8c2f0671cce7f6a3/ui/views/controls/styled_label.h
►

Sign in to add a comment