[Actor][Task Indicator] Update hover color [chromium/src : main]
Maggie Jennings (Gerrit)
Maggie Jennings voted and added 2 comments![]( "Open in Gerrit")
Votes added by Maggie Jennings
| Code-Review | +1 |
2 comments
SetBackgroundFrameActiveColorId(
kColorTabBackgroundInactiveHoverFrameActive);
SetBackgroundFrameInactiveColorId(
kColorTabBackgroundInactiveHoverFrameInactive);optional + uber-nit: move these to a SetHoveredColors() method and call from here?
SetHighlighted(is_pressed);nit: this isn't related to HighlightTaskIcon, right? if so, can you add a TODO to remove HighlightTaskIcon when we clean up the feature?
Related details
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Ying (Gerrit)
Christine Ying added 4 comments![]( "Open in Gerrit")
void GlicActorTaskIcon::StateChanged(ButtonState old_state) {instead of manually setting the color in statechanged, could we instead make use of the hover inkdrop thats already set on the button? https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc;l=129;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3
if (is_pressed_) {
// If the button is currently pressed, apply the pressed color state.
SetPressedColor(true);
} else {
// Otherwise, set default colors.
SetDefaultColors();
// If the button is hovered, override the default background with hover
// colors.i dont think we need any of this logic here
SetBackgroundFrameActiveColorId(
kColorTabBackgroundInactiveHoverFrameActive);
SetBackgroundFrameInactiveColorId(
kColorTabBackgroundInactiveHoverFrameInactive);this looks to be the same as HighlightTaskIcon, could we call that here instead?
void GlicActorTaskIcon::HighlightTaskIcon() {can you rename this function, as now we have SetHighlighted and HighlightTaskIcon but both do different things
Related details
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Ying (Gerrit)
Christine Ying added 1 comment![]( "Open in Gerrit")
if (is_pressed_) {
// If the button is currently pressed, apply the pressed color state.
SetPressedColor(true);
} else {
// Otherwise, set default colors.
SetDefaultColors();
// If the button is hovered, override the default background with hover
// colors.i dont think we need any of this logic here
also I don't think we should be manually controlling hover/pressed color states since those are already controlled by the underlying inkdrop https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc;l=129?q=SetCreateHighlightCallback&ss=chromium%2Fchromium%2Fsrc, instead we should figure out a solution to override the hover color
Kene Okoye (Gerrit)
Kene Okoye voted and added 6 comments![]( "Open in Gerrit")
Votes added by Kene Okoye
| Commit-Queue | +1 |
6 comments
void GlicActorTaskIcon::StateChanged(ButtonState old_state) {instead of manually setting the color in statechanged, could we instead make use of the hover inkdrop thats already set on the button? https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc;l=129;drc=a9f09a1411a697ace066a84efd018f49c28cc8a3
Revised TabStripControlButton to set a custom hover color for this. Thanks.
if (is_pressed_) {
// If the button is currently pressed, apply the pressed color state.
SetPressedColor(true);
} else {
// Otherwise, set default colors.
SetDefaultColors();
// If the button is hovered, override the default background with hover
// colors.Christine Yingi dont think we need any of this logic here
also I don't think we should be manually controlling hover/pressed color states since those are already controlled by the underlying inkdrop https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc;l=129?q=SetCreateHighlightCallback&ss=chromium%2Fchromium%2Fsrc, instead we should figure out a solution to override the hover color
Done
SetBackgroundFrameActiveColorId(
kColorTabBackgroundInactiveHoverFrameActive);
SetBackgroundFrameInactiveColorId(
kColorTabBackgroundInactiveHoverFrameInactive);optional + uber-nit: move these to a SetHoveredColors() method and call from here?
Acknowledged - my approach changed to call newly created methods in TabStripControlButton.
SetBackgroundFrameActiveColorId(
kColorTabBackgroundInactiveHoverFrameActive);
SetBackgroundFrameInactiveColorId(
kColorTabBackgroundInactiveHoverFrameInactive);this looks to be the same as HighlightTaskIcon, could we call that here instead?
Acknowledged
can you rename this function, as now we have SetHighlighted and HighlightTaskIcon but both do different things
Function will be removed after Redesign is launched so added a TODO and note.
nit: this isn't related to HighlightTaskIcon, right? if so, can you add a TODO to remove HighlightTaskIcon when we clean up the feature?
- Christine Ying
- Maggie Jennings
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kene Okoye (Gerrit)
Kene Okoye voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kene Okoye
| Commit-Queue | +1 |
- Christine Ying
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Ying (Gerrit)
Christine Ying voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christine Ying
| Code-Review | +1 |
1 comment
Related details
- David Pennington
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kene Okoye (Gerrit)
Kene Okoye voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kene Okoye
| Auto-Submit | +1 |
1 comment
nit: space between these functions
Done
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
kColorTabBackgroundInactiveHoverFrameActive);is the goal to always match the frame active color? its likely that you probably want the active state to change what color this inkdrop hover is.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kene Okoye (Gerrit)
Kene Okoye added 1 comment![]( "Open in Gerrit")
kColorTabBackgroundInactiveHoverFrameActive);is the goal to always match the frame active color? its likely that you probably want the active state to change what color this inkdrop hover is.
Yes. That is the intended behavior, similar to the GlicButton right beside it.
screencast - https://screencast.googleplex.com/cast/NTA5ODM1NjI1MzY1NTA0MHw5YTNhZTk2OS04Mw
Is there anything else you suggest I do?
Related details
- David Pennington
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
kColorTabBackgroundInactiveHoverFrameActive);Kene Okoyeis the goal to always match the frame active color? its likely that you probably want the active state to change what color this inkdrop hover is.
Yes. That is the intended behavior, similar to the GlicButton right beside it.
screencast - https://screencast.googleplex.com/cast/NTA5ODM1NjI1MzY1NTA0MHw5YTNhZTk2OS04Mw
Is there anything else you suggest I do?
i see, if thats intended behaviour ok, but IMO that looks pretty bad.
I would recommend switching the color based on the frame active state (similar to the rest of our UI). i can provide code pointers if needed for that but you can create an observer to listen to the frame state updates.
Related details
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kene Okoye (Gerrit)
Kene Okoye voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kene Okoye
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Kene Okoyeis the goal to always match the frame active color? its likely that you probably want the active state to change what color this inkdrop hover is.
David PenningtonYes. That is the intended behavior, similar to the GlicButton right beside it.
screencast - https://screencast.googleplex.com/cast/NTA5ODM1NjI1MzY1NTA0MHw5YTNhZTk2OS04Mw
Is there anything else you suggest I do?
i see, if thats intended behaviour ok, but IMO that looks pretty bad.
I would recommend switching the color based on the frame active state (similar to the rest of our UI). i can provide code pointers if needed for that but you can create an observer to listen to the frame state updates.
Done. Here is an updated screencast - https://screencast.googleplex.com/cast/NjU4NzUyNTc4NjIzODk3NnxjMThiOTMyNS1mZQ. And thank you for the suggestion/code pointer shared offline!
Related details
- Christine Ying
- David Pennington
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington added 1 comment![]( "Open in Gerrit")
- Christine Ying
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Pennington (Gerrit)
David Pennington voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Christine Ying
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christine Ying (Gerrit)
Christine Ying voted Commit-Queue+2![]( "Open in Gerrit")
- Kene Okoye
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Actor][Task Indicator] Update hover color
Update TabStripControlButton to allow custom ColorIds for inkdrop hover
and ripple effects. Previously, these colors were hardcoded, which
prevented certain UI components from meeting specific UX requirements
This change:
* Adds member variables and setters to TabStripControlButton for
managing custom inkdrop and ripple ColorIds.
* Updates GlicActorTaskIcon to dynamically refresh its inkdrop hover
color based on the browser window's active state.
* Registers GlicActorTaskIcon for window activation events to ensure
colors are updated when the frame focus changes.
* Includes a note regarding the misleading name of HighlightTaskIcon
and its planned removal once the task indicator is fully enabled
Screencast - https://screencast.googleplex.com/cast/NjU4NzUyNTc4NjIzODk3NnxjMThiOTMyNS1mZQ
UX Mock - https://docs.google.com/presentation/d/1RO65Q90GFMn7-Vq1-1gEmsl8phG-IHTl0BeMSB4Wehc/edit?slide=id.g3b17ca054dc_0_22#slide=id.g3b17ca054dc_0_22
- M chrome/browser/ui/views/tabs/glic/glic_actor_task_icon.cc
- M chrome/browser/ui/views/tabs/glic/glic_actor_task_icon.h
- M chrome/browser/ui/views/tabs/tab_strip_action_container.cc
- M chrome/browser/ui/views/tabs/tab_strip_control_button.cc
- M chrome/browser/ui/views/tabs/tab_strip_control_button.h
Code-Review: +1 by David Pennington
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |