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

Issue 687689 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Ensure correct theming and lack of ripple animations for non-clickable rows in the login/lock screen

Project Member Reported by tdander...@chromium.org, Feb 1 2017

Issue description

Certain default rows in the Ash system menu are not clickable in the login/lock screens. These rows should have the correct 'disabled' theming (color of icon and text) and should not show a ripple when clicked/tapped.

As a first step, an audit of all impacted rows will be required.
 

Comment 1 by moh...@chromium.org, Feb 10 2017

To my understanding, these items are all of type TrayItemMore which has a |show_more_| flag determining if a detailed view should be shown or not? If that's right, we can probably use the same flag to make the row disabled (SetEnabled(false)) which at least prevents the ripple.

Sebastian, do you have any opinion whether we need to also theme these rows as disabled? (considering that for rows that the feature is disabled, we are already theming them as disabled, e.g. netowrk, bluetooth)
I'm a bit confused. Why are we trying to disable rows that are already disabled ?
To clarify, certain rows/actions are already disabled. However, some are not disabled but we just prevent the user from accessing the detailed view. Right now the arrow is removed, but the question is whether we should also gray out the row to indicate that clicking won't do anything. See the screenshot, eg. Network or Cast rows.
Screenshot 2017-02-10 at 12.53.43 PM.png
51.5 KB View Download

Comment 4 by moh...@chromium.org, Feb 10 2017

Re c#2:

Consider network row:

1) If the user is logged in and connected to a network, the row is not grayed out and the user can click on it to switch to the detailed network view.

2) If the user is logged in, but does not have any network connection, the row is grayed out, however the user still can click on it to switch to the detailed view and possibly select a network to connect to.

3) If the device is locked, regardless of network connectivity, the user cannot click on the network row and switch to the detailed view. The row might be grayed out or not depending on the connectivity status.

This bug is about the third case. The row is not clickable, however a ripple is shown if user taps on it.
ah got it thanks.
ok so there's 2 "greyed" out version.

0.54a: Row feature is off but the row is clickable
0.36a: Row is disabled regardless of state (this is applied to settings/help/lock on lock screen).

So in the current example, Network, connected or not should be marked disabled, i.e 0.36a. 
this applies to bluetooth as well.

Comment 7 by moh...@chromium.org, Feb 10 2017

Status: Started (was: Assigned)
Great. Thanks
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 16 2017

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

commit 39de08056d4d8cb6a4f2928b4c142f907c37e6b7
Author: mohsen <mohsen@chromium.org>
Date: Thu Feb 16 23:47:54 2017

Make RotationLockDefaultView inherit directly from ActionableView

RotationLockDefaultView was inheriting from TrayItemMore to be able to
use its layout. However, it did not use TrayItemMore's chevron or
detailed view transition. This patch changes RotationLockDefaultView to
inherit directly from ActionableView and use factory functions in
TrayPopupUtils to layout its contents.

This patch also fixes an issue with rotation lock row where its label
was not updated properly when rotation lock status was changed (e.g.
due to use of rotation shortcut).

This patch is a prerequisite for fixing correct theming and ripple
animation for non-clickable rows in the lock screen.

BUG= 687689 
TEST=TrayRotationLockTest.* in ash_unittests, manual

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

[modify] https://crrev.com/39de08056d4d8cb6a4f2928b4c142f907c37e6b7/ash/system/chromeos/rotation/tray_rotation_lock.cc
[modify] https://crrev.com/39de08056d4d8cb6a4f2928b4c142f907c37e6b7/ash/system/chromeos/rotation/tray_rotation_lock.h

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 22 2017

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

commit 469c3b662e9bb4bfc28853a166420458079cf19d
Author: mohsen <mohsen@chromium.org>
Date: Wed Feb 22 08:04:57 2017

Modify TrayItemMore to use enabled status

With this patch, if a TrayItemMore object is set to be disabled, it will
not show the more arrow and ripple animations. Therefore, |show_more_|
field is not needed anymore and is removed.

Also, non-MD code path is removed from TrayItemMore.

BUG= 687689 , 687817 

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

[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/chromeos/bluetooth/tray_bluetooth.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/chromeos/cast/tray_cast.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/chromeos/network/tray_network.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/chromeos/network/tray_vpn.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/ime/tray_ime_chromeos.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/tray/tray_item_more.cc
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/tray/tray_item_more.h
[modify] https://crrev.com/469c3b662e9bb4bfc28853a166420458079cf19d/ash/common/system/tray_accessibility.cc

@mohsen is this done?
Re #10: Almost done. The only remaining part is that some icons (e.g. network icon) are not themed as disabled, yet. However, the ripples should work (i.e. not work in lock screen) and texts and some icons should be themed correctly.
Status: Fixed (was: Started)
I am going to mark this as done for M-58. Mohsen I would treat what you describe in #11 as a separate issue; can you please file a separate bug for that?
Re #12: Issue 699262 filed for the remaining work in icons.
Status: Verified (was: Fixed)
9534.0.0, 60.0.3092.0

Sign in to add a comment