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

Issue 878636 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Sep 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocked on:
issue 879030
issue 880363



Sign in to add a comment

desktop-pwas: "theme-color" is not applied to the titlebar in GTK+

Project Member Reported by ortuno@chromium.org, Aug 29

Issue description

Chrome Version: (copy from chrome://version)
OS: (e.g. Win10, MacOS 10.12, etc...)

What steps will reproduce the problem?
(1) Go to about:settings and change the theme to GTK+
(2) Go to https://www.pokedex.org/
(3) Open the three dot menu and click "Install Pokedex.org" or sometimes "Create shortcut".
(4) Wait for the new window to open

What should happen?
The title bar should be purple (the app's theme color).

What happens instead?
The title bar is grey. The three dot menu and title are white and the frame borders are purple. See screenshot.

 
white titlebar.png
155 KB View Download
Cc: -mgiuca@chromium.org
Labels: -Pri-3 M-70 Pri-1
Owner: mgiuca@chromium.org
Status: Assigned (was: Untriaged)
Interesting. I didn't notice this because Air Horner background color is blue, so it just looked like there was no border. And title color is black, so it looked fine. It seems the title color is based on the contrast with the theme color, even though the theme color is not applied.

We should probably just ignore the GTK setting and always use the theme color, like this:
Screenshot from 2018-08-29 11-59-42.png
92.4 KB View Download
Status: Started (was: Assigned)
Fixed in CL: https://crrev.com/c/1195226

It was easier to keep respecting the GTK setting (which is enforced at a very low level in the FrameBackground class), and just fix the frame color to match the native theme.

Screenshot shows fixed window frame when using the GTK theme. If using any other theme, the window will be styled according to the app theme color, as shown in #1.
Screenshot from 2018-08-29 14-05-48.png
95.3 KB View Download
Blockedon: 879030
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 30

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

commit cac6956430e23423886b7124b42b5a9450ccc21f
Author: Matt Giuca <mgiuca@chromium.org>
Date: Thu Aug 30 09:05:32 2018

Fix Linux hosted app / PWA window frame color with GTK theme.

The title text color and side/bottom frame color now match the GTK
theme, rather than the app's theme color (which clashed with the title
bar background color, that would use the native color scheme regardless
of the app theme color).

Adds a browser test for GetFrameColor.

Bug:  878636 
Change-Id: If5eaca1bc800f6c52f3f49918f455ffa2d881016
Reviewed-on: https://chromium-review.googlesource.com/1195226
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587490}
[modify] https://crrev.com/cac6956430e23423886b7124b42b5a9450ccc21f/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/cac6956430e23423886b7124b42b5a9450ccc21f/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
[modify] https://crrev.com/cac6956430e23423886b7124b42b5a9450ccc21f/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc

Status: Fixed (was: Started)
Labels: TE-Verified-71.0.3541.0 TE-Verified-M71
Able to reproduce the issue on ubuntu 17.10 using chrome build without fix. Observed that the title bar was grey and the three dot menu and title remained invisible.

Verified the fix on Ubuntu 17.10 using latest chrome version #71.0.3541.0 as per the comment #0 and #4.
Attaching screen cast for reference.
Observed that the title text color and side/bottom frame color now matched the GTK
theme, rather than the app's theme color.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
878636.webm
5.9 MB View Download
Blockedon: 880363
Status: Started (was: Fixed)
I reverted the CL due to test failure, linking bug to 880363.
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 5

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

commit 9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a
Author: Samuel Huang <huangs@chromium.org>
Date: Tue Sep 04 17:49:13 2018

Revert "Fix Linux hosted app / PWA window frame color with GTK theme."

This reverts commit cac6956430e23423886b7124b42b5a9450ccc21f.

Reason for revert: Suspected of causing consistent failure for mac-cocoa-rel tests

BrowserNonClientFrameViewBrowserTest.BrowserFrameColorThemed
BrowserNonClientFrameViewBrowserTest.BookmarkAppFrameColorSystemTheme

starting from:

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-cocoa-rel/1090

Original change's description:
> Fix Linux hosted app / PWA window frame color with GTK theme.
>
> The title text color and side/bottom frame color now match the GTK
> theme, rather than the app's theme color (which clashed with the title
> bar background color, that would use the native color scheme regardless
> of the app theme color).
>
> Adds a browser test for GetFrameColor.
>
> Bug:  878636 
> Change-Id: If5eaca1bc800f6c52f3f49918f455ffa2d881016
> Reviewed-on: https://chromium-review.googlesource.com/1195226
> Commit-Queue: Matt Giuca <mgiuca@chromium.org>
> Reviewed-by: Bret Sepulveda <bsep@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587490}

TBR=mgiuca@chromium.org,bsep@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  878636 ,  880363 
Change-Id: I999ae00f1bb5f1963e77c8394c47b8516a036343
Reviewed-on: https://chromium-review.googlesource.com/1204450
Commit-Queue: Samuel Huang <huangs@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588581}
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
[modify] https://crrev.com/9e6f06a2d395fd32c5dc2112f77d6ad15cfebc9a/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 6

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

commit d4f9bbd56e958e973205b04288c90d790c23afdc
Author: Matt Giuca <mgiuca@chromium.org>
Date: Thu Sep 06 01:53:29 2018

[reland] Fix Linux hosted app / PWA window frame color with GTK theme.

This is a reland of cac6956430e23423886b7124b42b5a9450ccc21f (r587490,
reverted in r588581).

The reland fixes a browser test crash on Mac Cocoa (because it's running
Views-only code) ( https://crbug.com/880363 ). Added a
ScopedMacViewsBrowserMode to force the test to run in Views mode.

Original change's description:

The title text color and side/bottom frame color now match the GTK
theme, rather than the app's theme color (which clashed with the title
bar background color, that would use the native color scheme regardless
of the app theme color).

Adds a browser test for GetFrameColor.

Bug:  878636 ,  880363 
Change-Id: I21003e78b4232cc6b8583267b08e2b37a823cfed
Reviewed-on: https://chromium-review.googlesource.com/1205972
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589086}
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc
[modify] https://crrev.com/d4f9bbd56e958e973205b04288c90d790c23afdc/chrome/browser/ui/views/frame/opaque_browser_frame_view_linux.cc

Status: Fixed (was: Started)
Should be fixed again. No merge is required, since the original fix was already in M70 branch before it got reverted. (May want to merge the updated fix to avoid test failures on mac-cocoa, but I will discuss that on  Issue 880363 .)

Sign in to add a comment