Set detached bookmark bar color to NTP color |
|||||||||||||||||
Issue description
Version: 53.0.2761.0 (Developer Build) (64-bit)
OS: Linux
Previously it was possible to change the background which is initialized with
contents_container_->set_background(views::Background::CreateSolidBackground(
GetThemeProvider()->GetColor(ThemeProperties::COLOR_CONTROL_BACKGROUND)));
from BrowserView::InitViews().
Indeed the CL https://codereview.chromium.org/63173016/patch/430001/440015 changed it from ThemeProperties::COLOR_TOOLBAR to COLOR_CONTROL_BACKGROUND.
In a manifest.json it was possible to customize "toolbar", but problem is that it is not possible to change "control_background".
Indeed COLOR_CONTROL_BACKGROUND is in the enum "NotOverwritableByUserThemeProperty" from Theme_properties.h
Would it be possible to move this to the user overwritable enum ?
Alternative is to add the following in ThemeService::GetDefaultColor:
case ThemeProperties::COLOR_CONTROL_BACKGROUND:
return GetColor(ThemeProperties::COLOR_TOOLBAR, incognito);
So that it at least mimic what is done in Gtk2UI::LoadGtkValues(), i.e:
colors_[ThemeProperties::COLOR_CONTROL_BACKGROUND] = toolbar_color;
colors_[ThemeProperties::COLOR_TOOLBAR] = toolbar_color;
,
Jun 27 2016
Hi Peter, please see attached screenshots.
,
Jul 1 2016
force_opaque_bg_for_detached_bookmark.png is the result when using this CL https://codereview.chromium.org/2062353002 with a manifest.json : { "manifest_version": 2, "name": "fun theme", "theme": { "colors": { "ntp_background": [20, 30, 180, 0.4] } }, "version": "2.0" } detached_bookmark.png is the visual result if I remove the force to opaque here https://codereview.chromium.org/2062353002/patch/60001/70009
,
Jul 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb4e97590966ba4138b759b8a6450e526a24fa27 commit bb4e97590966ba4138b759b8a6450e526a24fa27 Author: j.isorce <j.isorce@samsung.com> Date: Fri Jul 15 08:34:46 2016 Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by COLOR_NTP_BACKGROUND This fixes issues where the ntp background image was interrupted underneath the detached bookmark bar. Also the ntp_background color is now forced to be opaque even if a none opaque alpha value is provided from the theme. This prevent issues with subpixel AA which does not work on none opaque background. (and to avoid falling back to grayscale AA) Also see crbug.com/618278 BUG= 618335 R=estade@chromium.org, pkasting@chromium.org Review-Url: https://codereview.chromium.org/2062353002 Cr-Commit-Position: refs/heads/master@{#405725} [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/ui/chrome_style.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/ui/libgtk2ui/gtk2_ui.cc [modify] https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27/chrome/browser/ui/views/frame/browser_view.cc
,
Jul 17 2016
Thx for reporting the problem I will have a look.
,
Jul 18 2016
Issue 628970 is opened for the same.
,
Jul 18 2016
Issue 628970 has been merged into this issue.
,
Jul 18 2016
I think the fix here is twofold: * The detached bookmark bar text color should be the NTP text color, not the toolbar text color * We should ensure that in incognito we wind up using the appropriate incognito background and text colors I temporarily unchecked Evan's revert CL. Are you going to have time to address this within the next day or so? If not, we should probably proceed with the revert.
,
Jul 18 2016
Thx for he suggestion, here are possible code for:
1 >* The detached bookmark bar text color should be the NTP text color, not the >toolbar text color:
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -1777,7 +1777,7 @@ void BookmarkBarView::ConfigureButton(const BookmarkNode* node,
// We don't always have a theme provider (ui tests, for example).
if (GetThemeProvider()) {
SkColor color =
- GetThemeProvider()->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT);
+ GetThemeProvider()->GetColor(ThemeProperties::COLOR_NTP_TEXT);
button->SetEnabledTextColors(color);
if (node->is_folder()) {
button->SetImage(views::Button::STATE_NORMAL,
@@ -2103,7 +2103,7 @@ void BookmarkBarView::UpdateAppearanceForTheme() {
if (!theme_provider)
return;
SkColor color =
- theme_provider->GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT);
+ theme_provider->GetColor(ThemeProperties::COLOR_NTP_TEXT);
for (int i = 0; i < GetBookmarkButtonCount(); ++i) {
ConfigureButton(model_->bookmark_bar_node()->GetChild(i),
GetBookmarkButton(i));
2 >* We should ensure that in incognito we wind up using the appropriate >incognito background and text colors:
I have not found yet how the incognito background is set up.
3: Use theme image:
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -222,8 +222,19 @@ void PaintDetachedBookmarkBar(gfx::Canvas* canvas,
// The detached bar overlaps the |contents_container_| and so uses the same
// background color (the NTP background color).
- canvas->FillRect(fill_rect,
- tp->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND));
+ if (tp->HasCustomImage(IDR_THEME_NTP_BACKGROUND) /*||
+ !ui::MaterialDesignController::IsModeMaterial()*/) {
+ canvas->TileImageInt(*tp->GetImageSkiaNamed(IDR_THEME_NTP_BACKGROUND),
+ 0,
+ 0,
+ fill_rect.x(),
+ fill_rect.y(),
+ fill_rect.width(),
+ fill_rect.height());
+ } else {
+ canvas->FillRect(fill_rect,
+ tp->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND));
+ }
But this is not entirely correct, it should not be 0, 0. I would need to understand how the theme ntp background image is setup, especially the sub rect selected in the original image. In theme_source.cc I have found |if (GetMimeType(path) == "image/png") SendThemeImage(callback, resource_id, scale_factor);| but then I am lost from where this is called.
For these reasons I suggest you go ahead with the revert. If you have any suggestion for 2 and 3 that would be great. Thx.
,
Jul 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/88c375837d0f16c412839bb6dd1326312d607bb8 commit 88c375837d0f16c412839bb6dd1326312d607bb8 Author: estade <estade@chromium.org> Date: Mon Jul 18 22:25:50 2016 Revert of Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by COLOR_NTP_BACKGROUND (patchset #8 id:140001 of https://codereview.chromium.org/2062353002/ ) Reason for revert: caused regression: crbug.com/628970 Original issue's description: > Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by COLOR_NTP_BACKGROUND > > This fixes issues where the ntp background image was > interrupted underneath the detached bookmark bar. > > Also the ntp_background color is now forced to be opaque > even if a none opaque alpha value is provided from the > theme. This prevent issues with subpixel AA which does not > work on none opaque background. (and to avoid falling back > to grayscale AA) > > Also see crbug.com/618278 > > BUG= 618335 > > R=estade@chromium.org, pkasting@chromium.org > > Committed: https://crrev.com/bb4e97590966ba4138b759b8a6450e526a24fa27 > Cr-Commit-Position: refs/heads/master@{#405725} TBR=pkasting@chromium.org,j.isorce@samsung.com # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 618335 Review-Url: https://codereview.chromium.org/2159833002 Cr-Commit-Position: refs/heads/master@{#406118} [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/ui/chrome_style.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/ui/libgtk2ui/gtk2_ui.cc [modify] https://crrev.com/88c375837d0f16c412839bb6dd1326312d607bb8/chrome/browser/ui/views/frame/browser_view.cc
,
Jul 18 2016
It's easier to consider patches uploaded to the review tool where I can see them in context, rather than as text diffs here. For (1), make sure you don't use the NTP text color when the bookmark bar is still attached -- the cases where we should use the NTP text colors are the cases where we use the NTP background color. For (2), we probably want to be sure that if we get the NTP text/background colors on the default theme in incognito mode, we wind up with the appropriate incognito colors. That may require some fiddling in the theme provider. It probably should not require changes on the caller side. For (3), I suspect you do actually want to use 0, 0, and then make sure the page (which I think may already know the bookmark bar height??) renders its background image at an appropriate offset.
,
Aug 15 2016
Hi Peter, I updated the CL to support (1) and (2). About (3), it is not possible to use canvas->TileImageInt. Indeed the theme_nt_background image is renderer through CSS, see NTPResourceCache::CreateNewTabCSS / CreateNewTabIncognitoCSS. Actually I am not sure now what you are expecting when using custom image ? I think if we keep the ntp background color is fine. Also to avoid the regression https://bugs.chromium.org/p/chromium/issues/detail?id=628970 I did https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser/ui/views/frame/browser_view.cc
,
Jan 24 2017
The assigned owner "j.isorce@samsung.com" is not able to receive e-mails, please re-triage. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 24 2017
,
Feb 4 2017
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
,
Aug 21
Archiving old bugs that haven't been actively assigned in over a year. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks! |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by j.iso...@samsung.com
, Jun 9 2016