Should PWA windows use browser theme colours? |
||||||||||||||||
Issue descriptionChrome Version: 71 What steps will reproduce the problem? (1) Install theme: https://chrome.google.com/webstore/detail/yulia-brodskaya/jlgdloilieclkegafohackmhffbmdpko (2) Visit killer-marmot.appspot.com (3) Menu > Install Killer Marmot (4) Mouse over a link What is the expected result? The minimise, maximise, close buttons and the link address should be unthemed. What happens instead? They are green.
,
Oct 10
,
Oct 10
Screenshots of WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/1272896 Note the GTK theme still survives the nullptr AutoReset.
,
Oct 10
+hwi, austinknight, pcovell: A UX question was raised on the CL, do we want PWA windows to use Chrome browser theme colours (particularly if no theme colour is provided)? Should the PWA window status bubble and frame border be green if https://chrome.google.com/webstore/detail/yulia-brodskaya/jlgdloilieclkegafohackmhffbmdpko is installed?
,
Oct 11
Regarding comment 5, the specific proposal Evan made was "the hosted app frame color, when not specified by the app itself, should default to the browser theme's toolbar color". I would propose to use the browser theme's frame color instead of the browser theme's toolbar color. Not clear if we'd ever want to use the image resources. In any case, the idea would be to change the whole PWA frame for Killer Marmot to match the browser theme, instead of changing other elements to match the default theme. Both routes achieve color consistency, which is better than the blended world we have today (screenshot on comment 0).
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd15fe23c80306273a7e6ad736b02f4843ad3d7b commit cd15fe23c80306273a7e6ad736b02f4843ad3d7b Author: Alan Cutter <alancutter@chromium.org> Date: Thu Oct 11 18:38:52 2018 Windows 10: Fix mismatched theme colors for PWA frame caption buttons This CL is a quick fix for the "green buttons" bug where PWA windows use the main browser theme colours for frame caption buttons on Windows 10. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=362481&signed_aid=wZlcGI_yPhzQvEb3LIIImA==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=362482&signed_aid=oxJ3bwmmjcCErDhvgZue-w==&inline=1 This is a temporary work around for M71 while a more robust fix is worked on for PWA windows and browser themes: https://chromium-review.googlesource.com/c/chromium/src/+/1272896 Bug: 891560 Change-Id: Ic5fe4ddc1b8f60011d3b687a78ae2aad5fe7e185 Reviewed-on: https://chromium-review.googlesource.com/c/1275466 Commit-Queue: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#598865} [modify] https://crrev.com/cd15fe23c80306273a7e6ad736b02f4843ad3d7b/chrome/browser/ui/views/frame/windows_10_caption_button.cc [modify] https://crrev.com/cd15fe23c80306273a7e6ad736b02f4843ad3d7b/chrome/browser/ui/views/frame/windows_10_caption_button.h
,
Oct 12
Able to reproduce the issue on Windows 10 using chrome build without fix. Tried testing the fix on win-10 and ubuntu 17.10 using chrome version #71.0.3578.0. Observed that the minimise, maximise, close buttons and the link address are themed and appeared green. Hence, it seems the issue still reproduces. alancutter@ - Could you please check the attached screen shot and please help us in verifying the fix. Thanks...!!
,
Oct 12
#9: The PWA hasn't been installed in that screenshot. You need to go Menu > Install Killer Marmot.
,
Oct 16
New before and after screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/1272896 after https://chromium-review.googlesource.com/c/chromium/src/+/1275466 landed.
,
Oct 16
The main offending issue (big green buttons on Windows 10) has been resolved so this bug is now less urgent.
,
Oct 17
This change was raised in our last sync. It's not clear which option we think is best: 1. User browser theme overrides PWA theme. 2. User browser theme applies in the absence of a PWA theme. 3. User browser theme is ignored and default theming is used if no PWA theme. What was decided was that inconsistency is definitely bad and we should go with something. Picking which one is very much a P3 and something we can look at later when we run out of bigger issues. An exception to this would be if there's an accessibility reason for supporting themes.
,
Oct 17
@13: Accessibility (in particular, high contrast) is a major reason why people use themes, so comment 13 option 3 is problematic. If we always used the system theme instead of the Chrome theme in that case, perhaps it would be less of an issue, but that's a whole different can of worms. From an a11y perspective option 1 is probably best, but from an aesthetic perspective option 2 is probably best. I'm not certain what the right path to go is.
,
Oct 17
pkasting@ if this is a general accessibility problem, would we be better off making the themeable elements higher contrast for everyone?
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fa6292a67320ac8775549f3a7bf3df9d478ade60 commit fa6292a67320ac8775549f3a7bf3df9d478ade60 Author: Alan Cutter <alancutter@chromium.org> Date: Thu Oct 18 02:55:05 2018 Use default browser theming for hosted app windows PWA windows are intended to appear stand alone from the main browser so they should not adopt the main browser's theme colors. This CL provides a default theme getter to be used by hosted app (and PWA) windows. This CL also removes the quick fix for this issue done by https://chromium-review.googlesource.com/c/chromium/src/+/1275466. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=363145&signed_aid=TMzuMVlMKC2dm6ppdYp9PQ==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=363146&signed_aid=Og46P66KeV7iP-mnBHO3cA==&inline=1 Bug: 891560 Change-Id: I2d94bff9f72bfca59fcf7d6cb940f42e4a25cc70 Reviewed-on: https://chromium-review.googlesource.com/c/1272896 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#600643} [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/themes/theme_service.h [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/themes/theme_service_unittest.cc [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/ui/views/frame/windows_10_caption_button.cc [modify] https://crrev.com/fa6292a67320ac8775549f3a7bf3df9d478ade60/chrome/browser/ui/views/frame/windows_10_caption_button.h
,
Oct 22
,
Oct 22
|
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by alancutter@chromium.org
, Oct 9