Issue metadata
Sign in to add a comment
|
Regression : ‘Close’ (x) button on download shelf is not visible after applying the ‘Core’ theme.
Reported by
yfulgaon...@etouch.net,
Oct 17 2016
|
||||||||||||||||||||||
Issue descriptionChrome Version : 56.0.2891.0 db45a537654c59feee0308a5643cff514ea6446e-refs/heads/master@{#425529} 64-bit OS : Mac (10.10.5, 10.11.4) URL : https://chrome.google.com/webstore/detail/core/gkhcgfdghbiidgeccbldhfceleibkkpe?utm_source=chrome-ntp-icon What steps will reproduce the problem? 1. Launch chrome and add a theme from above URL. 2. Now open NTP, hit ‘Cmd + S’ key and save the current page. (Download shelf is seen at the bottom of page) 3. Observe the ‘Close’ (x) button on download shelf at bottom right corner. Actual : ‘Close’ (x) button on download shelf is not visible after applying the ‘Core’ theme. Expected : ‘Close’ (x) button on download shelf should be seen properly (in white color) after applying the ‘Core’ theme. This is a regression issue broken in ‘M-54’, below is the Manual Regression range and will soon update bisect info. Good build : 54.0.2796.0 Bad build : 54.0.2797.0 Note : This is Mac OS specific issue and the same is working fine on Windows and Linux OS.
,
Oct 25 2016
+sdy who is working on download shelf.
,
Nov 17 2016
Still able to reproduce the issue on Mac 10.11.6 using chrome latest version 56.0.2922.0. spqchan@ could you please look into this issue.
,
Nov 17 2016
I will look at this once I have the chance
,
Jan 4 2017
Just to update, still able to reproduce this issue on Mac 10.12.2 using latest canary #57.0.2970.0. spqchan@ - Gentle Ping...!! Could you please have a look into this issue. Thanks...!!
,
Jan 9 2017
,
Jan 17 2017
Just to update, still able to reproduce this issue on Mac 10.12.2 using latest canary #57.0.2983.0. sdy@ - Gentle Ping...!! Could you please have a look into this issue. Thanks...!!
,
Jan 23 2017
Just to update, still able to reproduce this issue on Mac 10.12.2 using latest canary #58.0.2990.0 sdy@, Could you please have a look into this issue.
,
Feb 2 2017
,
Feb 2 2017
The change has landed as 524a361c86c23bb4f5384c1738f786532a296e46, I'm not sure where bugdroid is today. Please report back if this fixed/not fixed in the next Canary.
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/524a361c86c23bb4f5384c1738f786532a296e46 commit 524a361c86c23bb4f5384c1738f786532a296e46 Author: sdy <sdy@chromium.org> Date: Thu Feb 02 20:04:33 2017 Fix using theme colors for the download shelf's close button. The theme world is messy: - ThemeProvider encapsulates some state like incognito status, but not other state like whether the active or inactive version of a color should be used (e.g. COLOR_TAB_TEXT vs. COLOR_BACKGROUND_TAB_TEXT). - A browser window always(?) has a ThemeProvider, but since some views try to access theme information when they're not in a window, there are null checks all over. - Some views listen for theme callbacks. Others, like HoverCloseButton, expect to be told to redraw when the theme changes. - -[TabView iconColor] modifies a theme color by tweaking its alpha and wants its children to use it. It also returns a hard-coded color that isn't in the theme at all for "dark" themes. This isn't really supported by the theme system. The issue that this CL fixes was the result of HoverCloseButton expecting to be a child of a TabView and falling back to a default color. This fix doesn't really change that dependency, but it lets anyone set the color instead of assuming a TabView parent. It will still use a default color if nobody tells it otherwise, since it doesn't look at the theme itself. A more complex solution would let HoverCloseButton pull a color from the theme but allow that color to be overridden (by TabView), but that doesn't seem worth it. I'd be a fan of rethinking details of how we deal with themes to make issues like this harder to come by. BUG= 656553 Review-Url: https://codereview.chromium.org/2668393003 Cr-Commit-Position: refs/heads/master@{#447826} [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/app/nibs/DownloadShelf.xib [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/hover_close_button.h [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/hover_close_button.mm [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/tabs/tab_view.h [modify] https://crrev.com/524a361c86c23bb4f5384c1738f786532a296e46/chrome/browser/ui/cocoa/tabs/tab_view.mm
,
Feb 8 2017
,
Feb 8 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973 commit 9adf7c095e768a3ab1e0b3aed26379ff5c3f8973 Author: Sidney San Martín <sdy@chromium.org> Date: Wed Feb 08 03:06:22 2017 Fix using theme colors for the download shelf's close button. The theme world is messy: - ThemeProvider encapsulates some state like incognito status, but not other state like whether the active or inactive version of a color should be used (e.g. COLOR_TAB_TEXT vs. COLOR_BACKGROUND_TAB_TEXT). - A browser window always(?) has a ThemeProvider, but since some views try to access theme information when they're not in a window, there are null checks all over. - Some views listen for theme callbacks. Others, like HoverCloseButton, expect to be told to redraw when the theme changes. - -[TabView iconColor] modifies a theme color by tweaking its alpha and wants its children to use it. It also returns a hard-coded color that isn't in the theme at all for "dark" themes. This isn't really supported by the theme system. The issue that this CL fixes was the result of HoverCloseButton expecting to be a child of a TabView and falling back to a default color. This fix doesn't really change that dependency, but it lets anyone set the color instead of assuming a TabView parent. It will still use a default color if nobody tells it otherwise, since it doesn't look at the theme itself. A more complex solution would let HoverCloseButton pull a color from the theme but allow that color to be overridden (by TabView), but that doesn't seem worth it. I'd be a fan of rethinking details of how we deal with themes to make issues like this harder to come by. BUG= 656553 Review-Url: https://codereview.chromium.org/2668393003 Cr-Commit-Position: refs/heads/master@{#447826} (cherry picked from commit 524a361c86c23bb4f5384c1738f786532a296e46) Review-Url: https://codereview.chromium.org/2687633002 . Cr-Commit-Position: refs/branch-heads/2987@{#379} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/app/nibs/DownloadShelf.xib [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.h [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/download/download_shelf_view_cocoa.mm [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/hover_close_button.h [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/hover_close_button.mm [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/tabs/tab_view.h [modify] https://crrev.com/9adf7c095e768a3ab1e0b3aed26379ff5c3f8973/chrome/browser/ui/cocoa/tabs/tab_view.mm
,
Feb 15 2017
Verified the fix on Mac 10.12.2 using Chrome beta version #57.0.2987.54 as per the comment #0. Observed that "Close" (x) button on download shelf is seen properly (in white color) after applying the ‘Core’ theme as expected. Hence, the fix is working as expected. Attaching the screencast for reference Adding the verified labels. Thanks...!! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by yfulgaon...@etouch.net
, Oct 17 2016Status: Assigned (was: Unconfirmed)