[EncapsulateAnimations] Re-enable optional button transitions [chromium/src : main]
Neil Coronado (Gerrit)
Neil Coronado voted and added 2 comments![]( "Open in Gerrit")
Votes added by Neil Coronado
| Commit-Queue | +1 |
2 comments
mOnBeforeShowTransitionCallback.run();Adding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
endFocusTransition(/* hasFocus= */ false);`#endUrlFocusAnimation` is called when the URL loses focus. Adding this effectively disables the unfocus animation (like with our feature disabled).
Related details
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 6 comments![]( "Open in Gerrit")
mOnBeforeShowTransitionCallback.run();Adding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
Shouldn't this be called before the setVisibility call in line 936 since this is named "beforeShow"?
// MeasuredWidth() represents the desired width of the container which is accurate most
// time, except during the optional button animations, where the MeasuredWidth changes
// instantly to the final size and Width() represents the actual size at that frame.This comment only applies to the else block?
endFocusTransition(/* hasFocus= */ false);`#endUrlFocusAnimation` is called when the URL loses focus. Adding this effectively disables the unfocus animation (like with our feature disabled).
Acknowledged
mOptionalButtonCoordinator.setOnBeforeDelayedTransitionCallback(What is the exact difference between this and setTransitionStartedCallback? Is setTransitionStartedCallback called during onTransitionStart() which might be too late to endFocusTransition?
mOptionalButtonCoordinator.setOnBeforeWidthTransitionCallback(Dont we need to set mLayoutLocationBarWithExtraButton = true here?
mOptionalButtonCoordinator.setOnBeforeShowTransitionCallback(
() -> {
mLayoutLocationBarWithExtraButton = true;
createAndRunFocusAnimatorRefactored(urlHasFocus());
});
mOptionalButtonCoordinator.setOnBeforeWidthTransitionCallback(
(type, widthDelta) -> {
mOptionalButtonTransitionWidthDelta = widthDelta;
createAndRunFocusAnimatorRefactored(urlHasFocus());
});Synced offline - wanted to check if hiding button would trigger a width change callback / should we merge callbacks
There is also setTransitionStartedCallback - would be nice to see if we can merge all of them into this one
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. |
Neil Coronado (Gerrit)
Neil Coronado added 4 comments![]( "Open in Gerrit")
mOnBeforeShowTransitionCallback.run();Sirisha KavuluruAdding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
Shouldn't this be called before the setVisibility call in line 936 since this is named "beforeShow"?
I agree the naming/timing feels weird to me, but I did this to be consistent with when the other callbacks are triggered (e.g. `OnBeforeWidth` [1] and `onBeforeHide`). I think this is because it's not meant to be interpreted as "before X state is set," but instead "after X state is set, but before the X transition happens."
[1] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=823-829;drc=264504830cc1341cd717224911502c68abc5a26b
[2] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=874-883;drc=264504830cc1341cd717224911502c68abc5a26b
mOptionalButtonCoordinator.setOnBeforeDelayedTransitionCallback(What is the exact difference between this and setTransitionStartedCallback? Is setTransitionStartedCallback called during onTransitionStart() which might be too late to endFocusTransition?
Is setTransitionStartedCallback called during onTransitionStart() which might be too late to endFocusTransition
Yes, that is the case. I wanted to "get everything into a good state" before triggering `#beginDelayedTransition` for the optional button. This is because the OptionalButton somewhat counterintuitively animates its sibling views [1] (i.e. the toolbar buttons container).
So I needed to clear any transition state on the toolbar buttons before letting the optional button animations start (i.e. before the `#beginDelayedTransition` call).
mOptionalButtonCoordinator.setOnBeforeWidthTransitionCallback(Dont we need to set mLayoutLocationBarWithExtraButton = true here?
IIUC, the width transition can be for either collapse or expand. `mLayoutLocationBarWITHOUTExtraButton` is only set when the button is explicitly hidden, and I opted to model our use case after this (i.e. set `mLayoutLocationBarWITHExtraButton` when explicitly shown).
mOptionalButtonCoordinator.setOnBeforeShowTransitionCallback(
() -> {
mLayoutLocationBarWithExtraButton = true;
createAndRunFocusAnimatorRefactored(urlHasFocus());
});
mOptionalButtonCoordinator.setOnBeforeWidthTransitionCallback(
(type, widthDelta) -> {
mOptionalButtonTransitionWidthDelta = widthDelta;
createAndRunFocusAnimatorRefactored(urlHasFocus());
});Synced offline - wanted to check if hiding button would trigger a width change callback / should we merge callbacks
There is also setTransitionStartedCallback - would be nice to see if we can merge all of them into this one
In the flow that I tested, I was only getting show/hide signals, and not widthChange signals. re:setTransitionStarted, the signal was too late as per the comment above.
Related details
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 4 comments![]( "Open in Gerrit")
mOnBeforeShowTransitionCallback.run();Sirisha KavuluruAdding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
Neil CoronadoShouldn't this be called before the setVisibility call in line 936 since this is named "beforeShow"?
I agree the naming/timing feels weird to me, but I did this to be consistent with when the other callbacks are triggered (e.g. `OnBeforeWidth` [1] and `onBeforeHide`). I think this is because it's not meant to be interpreted as "before X state is set," but instead "after X state is set, but before the X transition happens."
[1] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=823-829;drc=264504830cc1341cd717224911502c68abc5a26b
[2] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=874-883;drc=264504830cc1341cd717224911502c68abc5a26b
Ack - I'll also loop into @salg into this CL to review /optional_button/ changes as an owner
if (mOptionalButtonCoordinator != null && mLayoutLocationBarWithExtraButton) {Should we rename this is isOptionalButtonShowInTransition ?
The naming is confusing - this is set to true only when show transition is running and false otherwise (location bar will be drawn with the extra optional button after the transition is complete / during width transition)
mOptionalButtonCoordinator.setOnBeforeDelayedTransitionCallback(Neil CoronadoWhat is the exact difference between this and setTransitionStartedCallback? Is setTransitionStartedCallback called during onTransitionStart() which might be too late to endFocusTransition?
Is setTransitionStartedCallback called during onTransitionStart() which might be too late to endFocusTransition
Yes, that is the case. I wanted to "get everything into a good state" before triggering `#beginDelayedTransition` for the optional button. This is because the OptionalButton somewhat counterintuitively animates its sibling views [1] (i.e. the toolbar buttons container).
So I needed to clear any transition state on the toolbar buttons before letting the optional button animations start (i.e. before the `#beginDelayedTransition` call).
Acknowledged
mOptionalButtonCoordinator.setOnBeforeWidthTransitionCallback(Neil CoronadoDont we need to set mLayoutLocationBarWithExtraButton = true here?
IIUC, the width transition can be for either collapse or expand. `mLayoutLocationBarWITHOUTExtraButton` is only set when the button is explicitly hidden, and I opted to model our use case after this (i.e. set `mLayoutLocationBarWITHExtraButton` when explicitly shown).
- Neil Coronado
- Salvador Guerrero Ramos
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Neil Coronado
- Salvador Guerrero Ramos
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. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos added 5 comments![]( "Open in Gerrit")
private @Nullable Runnable mOnBeforeHideTransitionCallback;Could you also update this to be non-nullable? to be consistent with the other callbacks.
mOnBeforeShowTransitionCallback = callback;Could you update OptionalButtonViewTest.java with these new callbacks?
mOnBeforeShowTransitionCallback.run();Sirisha KavuluruAdding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
Neil CoronadoShouldn't this be called before the setVisibility call in line 936 since this is named "beforeShow"?
Sirisha KavuluruI agree the naming/timing feels weird to me, but I did this to be consistent with when the other callbacks are triggered (e.g. `OnBeforeWidth` [1] and `onBeforeHide`). I think this is because it's not meant to be interpreted as "before X state is set," but instead "after X state is set, but before the X transition happens."
[1] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=823-829;drc=264504830cc1341cd717224911502c68abc5a26b
[2] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=874-883;drc=264504830cc1341cd717224911502c68abc5a26b
Ack - I'll also loop into @salg into this CL to review /optional_button/ changes as an owner
Timing wise this is okay, all the UI changes after the call to beginDelayedTransition will be animated, and the animation won't start until the next frame.
if (ChromeFeatureList.sToolbarPhoneAnimationRefactor.isEnabled()) {Just to confirm, this is no longer needed because of the new callback?
// MeasuredWidth() represents the desired width of the container which is accurate most
// time, except during the optional button animations, where the MeasuredWidth changes
// instantly to the final size and Width() represents the actual size at that frame.This comment only applies to the else block?
+1
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. |
Neil Coronado (Gerrit)
Neil Coronado voted and added 6 comments![]( "Open in Gerrit")
Votes added by Neil Coronado
| Commit-Queue | +1 |
6 comments
private @Nullable Runnable mOnBeforeHideTransitionCallback;Could you also update this to be non-nullable? to be consistent with the other callbacks.
Done
Could you update OptionalButtonViewTest.java with these new callbacks?
Done
mOnBeforeShowTransitionCallback.run();Sirisha KavuluruAdding this event and the one at the start of `#beginDelayedTransition` are the only real changes to the `OptionalButton` files; the rest is boilerplate to set the callbacks (or I suppose runnables, but `mOnBeforeHideTransitionCallback` is also a runnable, so I opted to just keep the naming consistent).
Neil CoronadoShouldn't this be called before the setVisibility call in line 936 since this is named "beforeShow"?
Sirisha KavuluruI agree the naming/timing feels weird to me, but I did this to be consistent with when the other callbacks are triggered (e.g. `OnBeforeWidth` [1] and `onBeforeHide`). I think this is because it's not meant to be interpreted as "before X state is set," but instead "after X state is set, but before the X transition happens."
[1] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=823-829;drc=264504830cc1341cd717224911502c68abc5a26b
[2] https://crsrc.org/c/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java;l=874-883;drc=264504830cc1341cd717224911502c68abc5a26b
Salvador Guerrero RamosAck - I'll also loop into @salg into this CL to review /optional_button/ changes as an owner
Timing wise this is okay, all the UI changes after the call to beginDelayedTransition will be animated, and the animation won't start until the next frame.
Acknowledged
if (ChromeFeatureList.sToolbarPhoneAnimationRefactor.isEnabled()) {Just to confirm, this is no longer needed because of the new callback?
Yep, that is correct.
Or I guess to explain a bit more, the `#addTarget` call effectively disabled the optional button animations while our flag was enabled, as the timing caused it to suppress our newly-added transitions on the toolbar buttons container. We no longer need to do this, as we instead use the new callbacks to stop the transitions from conflicting.
// MeasuredWidth() represents the desired width of the container which is accurate most
// time, except during the optional button animations, where the MeasuredWidth changes
// instantly to the final size and Width() represents the actual size at that frame.Salvador Guerrero RamosThis comment only applies to the else block?
+1
Done
if (mOptionalButtonCoordinator != null && mLayoutLocationBarWithExtraButton) {Should we rename this is isOptionalButtonShowInTransition ?
The naming is confusing - this is set to true only when show transition is running and false otherwise (location bar will be drawn with the extra optional button after the transition is complete / during width transition)
- Salvador Guerrero Ramos
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salvador Guerrero Ramos (Gerrit)
Salvador Guerrero Ramos voted Code-Review+1![]( "Open in Gerrit")
- Neil Coronado
- Sirisha Kavuluru
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
[EncapsulateAnimations] Re-enable optional button transitions
Re-enable optional button transitions.
-Update the OptionalButton flow to be aware of the ToolbarPhone
transitions. If an OptionalButton transition starts, then immediately
end the URL focus transition, and restart with updated widths.
-Similarly, use the OptionalButton transition state to calculate the
LocationBar width, etc. accordingly.
Before: http://go/scrcast/NTcxNzIwNDY2OTU2Mjg4MHxhOTNhMjkwNy1hNQ
After: http://go/scrcast/NTA3NTg2NDMxNTIzMjI1Nnw2YzA5NGE4Mi04Ng
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonCoordinator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonMediator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonProperties.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonView.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonViewBinder.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/optional_button/OptionalButtonViewTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java
Code-Review: +1 by Salvador Guerrero Ramos
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |