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

Issue 654062 link

Starred by 2 users
Status: Fixed
Owner:
█ rolfe@chromium.org
Last visit > 30 days ago
Closed: Oct 2016
Cc:
twelling...@chromium.org
tedc...@chromium.org
█ bhal...@amazon.com
█ dfalcant...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Default text color is inconsistently set on Clank

Project Member Reported by dfalcant...@chromium.org, Oct 7 2016 
We've got a bunch of UI components that explicitly define the text color as "default_text_color", which was defined as "#333333".  However, a bunch of our other UI components don't do this and instead inherit from the default Android theme (which seems to alternatively use #000000 or Google Grey 900 (#212121)).  A couple of _other_ UI components explicitly define #000000, but I didn't find too many with a two minute search.


I've got a CL from bhallam@amazon.com that:
1) Explicitly sets the main theme color to be #333333
2) Changes styling behavior so that many components will stop explicitly defining their text color as #333333, making them inherit from the main theme

https://chromiumcodereview.appspot.com/2365733002/


(1) forces all UI that was originally inheriting from the default Android theme to switch to #333333, which changes things everywhere, including our menus and dialog headers (just the two places I checked).  Is this something UX would approve?  Images attached show the subtle differences (you'll need to diff the images).
menu_before.png
181 KB View Download
menu_after.png
180 KB View Download
before.png
166 KB View Download
after.png
165 KB View Download

Comment 1 by rolfe@chromium.org, Oct 7 2016 

Actually Chrome color spec states #000000 on light backgrounds:
https://spec.googleplex.com/chrome/style/color.html#

I'd support setting default style to that (instead of #333333) and overriding Android inheritance. More importantly though I'd love it if we could streamline colors across Chrome on Android per this internal bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=424645
I went through the spreadsheet of all colors linked there and matched them to Material ones, but want to align with the Chrome spec first and foremost. Unfortunately the spreadsheet doesn't say how the color appears (e.g. over white or dark backgrounds, which affects the color choice). Whenever someone on the front-end eng team has time to go through it we can see live shots to know if the color recommendation is right.

If you do the override here, feel free to comment on that bug and update the spreadsheet to the proper colors so we have a current document.

Comment 2 by dfalcant...@chromium.org, Oct 8 2016 

Hokay, I'm cool with actual black instead of sorta-black.

bhallam@: Can you update your CL to set default_text_color to #000000 and make the CL point at this bug?

rolfe@: Thaaaaat's a lot of places to look at.  I could help knock out a few at a time if you keep it on my radar.

Comment 3 by bhal...@amazon.com, Oct 8 2016 

Thanks dfalcantra, I will do that immediately!
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 11 2016 

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

commit e4f0a51143b3db3643b7bac31d0be4d08642fe4e
Author: bhallam <bhallam@amazon.com>
Date: Tue Oct 11 23:44:32 2016

Inherit default text color from parent themes.

This change removes default_text_color being
hard-coded in ui components everywhere.

Most controls are created as part of the their parent Themes (MainTheme and DownloadsActivityTheme for this cl)
they will automatically get android:textColor set to default_text_color.

BUG= 654062 

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

[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/account_chooser_dialog_item.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/account_chooser_dialog_title.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/account_signin_account_view.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/account_signin_view.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/bookmark_row_content.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/bookmark_widget_item.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/data_reduction_promo_screen.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/download_item_view.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/download_manager_ui_drawer_filter.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/infobar_control_icon_with_description.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/infobar_control_message.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/infobar_control_spinner_view.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/infobar_control_toggle.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/password_entry_editor_interactive.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/password_generation_popup_suggestion.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/payment_request_header.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/signin_and_sync_view.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/tab_switcher_callout.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/layout/upgrade_activity.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/values-v17/styles.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/res/values/colors.xml
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuTitleView.java
[modify] https://crrev.com/e4f0a51143b3db3643b7bac31d0be4d08642fe4e/chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilterAdapter.java

Comment 5 by rolfe@chromium.org, Oct 13 2016

Status: Fixed (was: Assigned)
Marking as fixed even though I did no work. : )
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 17 2016 

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

commit 563218db4ebf939eb9505e675611821c25a0d1da
Author: twellington <twellington@chromium.org>
Date: Mon Oct 17 23:29:42 2016

Revert "Inherit default text color from parent themes."

This reverts commit e4f0a51143b3db3643b7bac31d0be4d08642fe4e.

This CL broke the text color for disabled text fields and dialog
buttons.

BUG=655930,  654062 
TBR=dfalcantara@chromium.org

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

[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_chooser_dialog_item.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_chooser_dialog_title.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_signin_account_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_signin_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/bookmark_row_content.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/bookmark_widget_item.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/data_reduction_promo_screen.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/download_item_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/download_manager_ui_drawer_filter.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_icon_with_description.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_message.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_spinner_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_toggle.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/password_entry_editor_interactive.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/password_generation_popup_suggestion.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/payment_request_header.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/signin_and_sync_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/tab_switcher_callout.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/upgrade_activity.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/values-v17/styles.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/values/colors.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuTitleView.java
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 17 2016 

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

commit 563218db4ebf939eb9505e675611821c25a0d1da
Author: twellington <twellington@chromium.org>
Date: Mon Oct 17 23:29:42 2016

Revert "Inherit default text color from parent themes."

This reverts commit e4f0a51143b3db3643b7bac31d0be4d08642fe4e.

This CL broke the text color for disabled text fields and dialog
buttons.

BUG=655930,  654062 
TBR=dfalcantara@chromium.org

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

[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_chooser_dialog_item.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_chooser_dialog_title.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_signin_account_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/account_signin_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/bookmark_row_content.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/bookmark_widget_item.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/data_reduction_promo_screen.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/download_item_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/download_manager_ui_drawer_filter.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_icon_with_description.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_message.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_spinner_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/infobar_control_toggle.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/password_entry_editor_interactive.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/password_generation_popup_suggestion.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/payment_request_header.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/signin_and_sync_view.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/tab_switcher_callout.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/layout/upgrade_activity.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/values-v17/styles.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/res/values/colors.xml
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
[modify] https://crrev.com/563218db4ebf939eb9505e675611821c25a0d1da/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuTitleView.java
►

Sign in to add a comment