New issue
Advanced search Search tips

Issue 876271 link

Starred by 1 user
Status: Fixed
Owner:
alancutter@chromium.org
Closed: Oct 9
Cc:
phanindra.mandapaka@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Desktop PWAs: Titlebar highlight needs to be visible on dark backgrounds

Project Member Reported by alancutter@chromium.org, Aug 21 
Chrome Version: 70

What steps will reproduce the problem?
(1) Visit soft-puppy.glitch.me
(2) App menu > Install Soft Puppy... > Install
(3) Hover mouse over app menu button.

What is the expected result?

There should be a highlight.

What happens instead?

No highlight is seen because it's black on black.
app-menu.png
39.5 KB View Download

Comment 1 by alancutter@chromium.org, Sep 27

Labels: OS-Chrome OS-Linux
Screenshots for WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/1248521
menu-before.png
4.8 KB View Download
menu-after.png
4.8 KB View Download
location-before.png
6.4 KB View Download
location-after.png
6.3 KB View Download

Comment 2 by alancutter@chromium.org, Sep 27 

Update screenshots to include find icon on dark theme.
dark-before.png
27.3 KB View Download
dark-after.png
27.8 KB View Download
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 9 

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

commit cbf3800c707ab5fdb90a34a858af397877e877bc
Author: Alan Cutter <alancutter@chromium.org>
Date: Tue Oct 09 05:40:40 2018

Fix icon highlights for hosted apps with dark theme colors

This CL fixes invisible icon highlights for hosted apps with
dark theme colors.

This CL updates the ink highlight base color logic of indicator
icons like content settings, page actions and extension actions
to be based on which container they're in.

This allows hosted app title bars to set the icon highlights
relative to the icon colors (and theme color) while leaving the
location bar to continue using its existing color logic.

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360311&signed_aid=D7ev8tHi_uOFeRdmHnjC4w==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360312&signed_aid=A_FQTmAn6KoTeq63KBhU7Q==&inline=1

A side effect of this CL is that content settings in the location
bar now use the same highlight color logic as everything else in
the location bar. Their highlight logic changed accidentally in
https://chromium-review.googlesource.com/1195721 when the location
bar started using the same code path as the hosted app button
container does for setting the icon's color, triggering a different
codepath in ContentSettingImageView::GetInkDropBaseColor().

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360308&signed_aid=sdSbBOu-p_YHCps3f1QFbw==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360309&signed_aid=8gOEBWYKu8_ZCLpyzbHHdA==&inline=1

Bug:  876271 
Change-Id: I610500fcd2362d371a82b6da79c9edc6a03d42c1
Reviewed-on: https://chromium-review.googlesource.com/c/1248521
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597810}
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/frame/hosted_app_button_container.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/frame/hosted_app_menu_button.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/frame/hosted_app_menu_button.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/content_setting_image_view.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/location_icon_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/location_icon_view.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/selected_keyword_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/location_bar/selected_keyword_view.h
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/page_action/page_action_icon_view.cc
[modify] https://crrev.com/cbf3800c707ab5fdb90a34a858af397877e877bc/chrome/browser/ui/views/page_action/page_action_icon_view.h

Comment 4 by alancutter@chromium.org, Oct 9

Status: Fixed (was: Assigned)

Comment 5 by alancutter@chromium.org, Oct 10

Labels: -Pri-2 Pri-3
Screenshots of follow up clean up CL.
before.png
2.9 KB View Download
after.png
2.9 KB View Download
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 10 

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

commit ea3ab8f2be75c63470d49a36acc3e9fadc7ec5f4
Author: Alan Cutter <alancutter@chromium.org>
Date: Wed Oct 10 06:41:26 2018

Fix visibility and ordering of HostedAppButtonContaier override methods

This is a refactor follow up CL to https://chromium-review.googlesource.com/c/chromium/src/+/1248521
which changes the visibility HostedAppButtonContainer methods to match
the visibility of the super class methods they override.
In addition to this it alphabetises the inheritances and method
overrides.

This is a code style refactor CL with no changes in behaviour.

Bug:  876271 
Change-Id: I619445937cd6f3fddbf4433cff4616913f3d0c20
Reviewed-on: https://chromium-review.googlesource.com/c/1250463
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598228}
[modify] https://crrev.com/ea3ab8f2be75c63470d49a36acc3e9fadc7ec5f4/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/ea3ab8f2be75c63470d49a36acc3e9fadc7ec5f4/chrome/browser/ui/views/frame/hosted_app_button_container.h
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 11 

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

commit 5237276e341286865a5a0c12d7dd8ba7013e6829
Author: Alan Cutter <alancutter@chromium.org>
Date: Thu Oct 11 00:43:29 2018

Make hosted app icon color and ink drop base colors the same

This is a follow up CL to https://chromium-review.googlesource.com/c/chromium/src/+/1248521
to remove the difference between icon colors and their ink drop base color
in HostedAppButtonContainers.
The difference was negligible and unneccessary, see screenshots.

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=362268&signed_aid=1eLT-9HDBq2cdGOL-ixATA==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=362269&signed_aid=WRPoxy16eqyBb1wpqA6ZZw==&inline=1

Bug:  876271 
Change-Id: Id3500ab3553e01b47dd664d8a1b258de04722d9f
Reviewed-on: https://chromium-review.googlesource.com/c/1272915
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598598}
[modify] https://crrev.com/5237276e341286865a5a0c12d7dd8ba7013e6829/chrome/browser/ui/views/frame/hosted_app_button_container.cc
[modify] https://crrev.com/5237276e341286865a5a0c12d7dd8ba7013e6829/chrome/browser/ui/views/frame/hosted_app_button_container.h
[modify] https://crrev.com/5237276e341286865a5a0c12d7dd8ba7013e6829/chrome/browser/ui/views/frame/hosted_app_menu_button.cc
[modify] https://crrev.com/5237276e341286865a5a0c12d7dd8ba7013e6829/chrome/browser/ui/views/frame/hosted_app_menu_button.h

Comment 8 by phanindra.mandapaka@chromium.org, Oct 12

Cc: phanindra.mandapaka@chromium.org
Labels: Needs-Feedback
Tried verifying the fix on build without fix #70.0.3528.0 and build with fix #71.0.3578.0 on Windows 10 by following steps as per comment #0, but unable to observe the difference between the both builds. 
Steps:
1. Launched chrome 
2. Navigated to soft-puppy.glitch.me
As we are not seen App menu, attached the screen-cast for reference.
Alan Cutter@ - Could you please help in verifying the fix.

Thanks..!
876271.mp4
3.0 MB View Download

Comment 9 by alancutter@chromium.org, Oct 15 

By app menu I mean Chrome's 3 dot menu in the top right (which is an exclamation mark in the video indicating updates are required).
►

Sign in to add a comment