[Demo] Animate background colors on url focus change [chromium/src : main]
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 1 comment![]( "Open in Gerrit")
// Set the initial state to ensure the delayed transition animates from the correct starting
// values.
updateBackground(!hasFocus);
updateToolbarAndLocationBarColorForFocusChange(hasFocus ? 0f : 1f);What happens if we dont set this?
Related details
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado added 1 comment![]( "Open in Gerrit")
// Set the initial state to ensure the delayed transition animates from the correct starting
// values.
updateBackground(!hasFocus);
updateToolbarAndLocationBarColorForFocusChange(hasFocus ? 0f : 1f);What happens if we dont set this?
Sync'd offline; I _think_ this we needed to set the correct starting color & corner radius specifically for the NTP (as we swap drawables on NTP), and I was seeing stale starting color in testing.
I also think the `updateBackground` call is unnecessary. Will revisit.
Related details
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado added 1 comment![]( "Open in Gerrit")
// Set the initial state to ensure the delayed transition animates from the correct starting
// values.
updateBackground(!hasFocus);
updateToolbarAndLocationBarColorForFocusChange(hasFocus ? 0f : 1f);Neil CoronadoWhat happens if we dont set this?
Sync'd offline; I _think_ this we needed to set the correct starting color & corner radius specifically for the NTP (as we swap drawables on NTP), and I was seeing stale starting color in testing.
I also think the `updateBackground` call is unnecessary. Will revisit.
To be more specific, I _think_ that when we unfocus, we:
- already have the focused-colored location bar drawable
- switch to NTP drawable
- run transition
- observe that since we've already switched to the NTP drawable, we _do not_ change the location bar drawable from the focused-color -> unfocused-color.
So when we would next focus, we would
- switch to the location bar drawable
- observe that it's still the (old) focused-color
- run transition
- observe that the color does not animate, as it already started at the focused-color.
I'm not sure what the ideal pattern is here. I think it'd be preferable if we could:
- avoid changing the current properties
- `#beginDelayedTransition`
- `#setEndProperties(hasFocus)`
But there's some state that might not be correctly set at the start of the transition. Namely the drawable state & potentially the location bar bounds (if it's not being updated while invisible due to the fakebox showing instead). So we may want to:
- `#setEndProperties(!hasFocus)`
- `#beginDelayedTransition`
- `#setEndProperties(hasFocus)`
Though unclear if some things would still need to be special-cased.
Related details
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Also oops, the `updateBackground(!hasFocus);` is currently required, since when unfocusing, we've already switched to the NTP drawable in `#onUrlFocusChange`.
The NTP drawable doesn't currently change color (instead, with the flag disabled, we interpolate the location bar drawable properties while translating down, then switch to the NTP drawable when the bounds + color + corners matched the NTP drawable).
So it's awkward, but we:
1. switch to NTP drawable in `#onUrlFocusChange` -> `updateBackground(hasFocus);`
2. switch back to the location bar drawable with `updateBackground(!hasFocus);`
3. grab the correct starting color in `#beginDelayedTransition`
4. switch back to the NTP drawable with `updateBackground(hasFocus);`
5. animate the NTP drawable's properties in the transition.
This is also why we need to `#mutate()` the NTP drawable, as we're now changing its state in the transition, which we didn't do before.
It might be cleaner to just move the call from 1. to the animator methods; I don't have a strong opinion yet.
Related details
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 1 comment![]( "Open in Gerrit")
Removing from my attention set. Please add me back when ready for review
Related details
- Neil Coronado
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. |