From 2effd41efd4d2e06a571f03f96bec7e3dea88e95 Mon Sep 17 00:00:00 2001 From: spdonghao Date: Fri, 12 Mar 2021 00:36:42 +0000 Subject: [PATCH] [Start] Update triggerUrlFocusAnimation logic. Before this CL, when calling triggerUrlFocusAnimation(), the value of |hasFocus| is set as |!urlHasFocus()| sometimes, which is not easy to understand. This CL makes the logic more understandable and keeps toolbar expanded on start surface homepage. Bug: 1184865 Change-Id: I5ebde44bc10fdec8a9674bdd4ae7f9c2d01b96d7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2739915 Commit-Queue: Hao Dong Reviewed-by: Filip Gorski Reviewed-by: Patrick Noland Reviewed-by: Tomasz Wiszkowski Reviewed-by: Xi Han Cr-Commit-Position: refs/heads/master@{#862218} --- .../omnibox/LocationBarCoordinator.java | 14 ++-- .../browser/omnibox/LocationBarMediator.java | 16 +++-- .../omnibox/status/StatusCoordinator.java | 6 +- .../omnibox/status/StatusMediator.java | 11 +-- .../browser/toolbar/top/ToolbarPhone.java | 72 +++++++++---------- 5 files changed, 63 insertions(+), 56 deletions(-) diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java index 7912599bf9174c..ef033a46ff01d3 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java @@ -473,15 +473,17 @@ public void setUrlFocusChangeInProgress(boolean inProgress) { * change. This will be called after any animations are performed to transition from one * focus state to the other. * - * @param hasFocus Whether the URL field has gained focus. - * @param shouldShowKeyboard Whether the keyboard should be shown. This value should be the same - * as hasFocus by default. + * @param showExpandedState Whether the url bar is expanded. + * @param shouldShowKeyboard Whether the keyboard should be shown. This value is determined by + * whether url bar has got focus. Most of the time this is the same as + * showExpandedState, but in some cases, e.g. url bar is scrolled to the top of the + * screen on homepage but not focused, we set it differently. * @param shouldShowInOverviewMode Whether the location bar should be shown when in overview * mode. */ - public void finishUrlFocusChange( - boolean hasFocus, boolean shouldShowKeyboard, boolean shouldShowInOverviewMode) { - mLocationBarMediator.finishUrlFocusChange(hasFocus, shouldShowKeyboard); + public void finishUrlFocusChange(boolean showExpandedState, boolean shouldShowKeyboard, + boolean shouldShowInOverviewMode) { + mLocationBarMediator.finishUrlFocusChange(showExpandedState, shouldShowKeyboard); if (shouldShowInOverviewMode) { mStatusCoordinator.onSecurityStateChanged(); } diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java index ce5219abe7da5e..7371591a49370b 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java @@ -585,17 +585,19 @@ public void set(LocationBarMediator object, Float value) { * Handles any actions to be performed after all other actions triggered by the URL focus * change. This will be called after any animations are performed to transition from one * focus state to the other. - * @param hasFocus Whether the URL field has gained focus. - * @param shouldShowKeyboard Whether the keyboard should be shown. This value should be the same - * as hasFocus by default. + * @param showExpandedState Whether the url bar is expanded. + * @param shouldShowKeyboard Whether the keyboard should be shown. This value is determined by + * whether url bar has got focus. Most of the time this is the same as + * showExpandedState, but in some cases, e.g. url bar is scrolled to the top of the + * screen on homepage but not focused, we set it differently. */ - /* package */ void finishUrlFocusChange(boolean hasFocus, boolean shouldShowKeyboard) { + /* package */ void finishUrlFocusChange(boolean showExpandedState, boolean shouldShowKeyboard) { if (mUrlCoordinator == null) return; - mUrlCoordinator.setKeyboardVisibility(hasFocus && shouldShowKeyboard, true); + mUrlCoordinator.setKeyboardVisibility(shouldShowKeyboard, true); setUrlFocusChangeInProgress(false); updateShouldAnimateIconChanges(); - mStatusCoordinator.onUrlAnimationFinished(hasFocus); - if (!mIsTablet && !hasFocus) { + mStatusCoordinator.onUrlAnimationFinished(showExpandedState); + if (!mIsTablet && !showExpandedState) { mLocationBarLayout.setUrlActionContainerVisibility(View.GONE); } } diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusCoordinator.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusCoordinator.java index d6d482c1e6c7e1..303c184e5e641d 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusCoordinator.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusCoordinator.java @@ -147,9 +147,9 @@ public void onUrlFocusChange(boolean urlHasFocus) { updateVerboseStatusVisibility(); } - /** @param urlHasFocus Whether the url currently has focus. */ - public void onUrlAnimationFinished(boolean urlHasFocus) { - mMediator.setUrlAnimationFinished(urlHasFocus); + /** @param showExpandedState Whether the url bar is expanded currently. */ + public void onUrlAnimationFinished(boolean showExpandedState) { + mMediator.setUrlAnimationFinished(showExpandedState); } /** @param show Whether the status icon should be VISIBLE, otherwise GONE. */ diff --git a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusMediator.java b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusMediator.java index e414396061ec84..adb6c7b00383ac 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusMediator.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusMediator.java @@ -279,9 +279,12 @@ void setUrlHasFocus(boolean urlHasFocus) { if (!mUrlHasFocus) updateLocationBarIconForDefaultMatchCategory(true); } - // Extra logic to support extra NTP use cases which show the status icon when animating and when - // focused, but hide it when unfocused. - void setUrlAnimationFinished(boolean urlHasFocus) { + /** + * Extra logic to support extra NTP use cases which show the status icon when animating and when + * focused, but hide it when unfocused. + * @param showExpandedState Whether the url bar is expanded currently. + */ + void setUrlAnimationFinished(boolean showExpandedState) { if (mIsTablet || !mSearchEngineLogoUtils.shouldShowSearchEngineLogo( mLocationBarDataProvider.isIncognito())) { @@ -291,7 +294,7 @@ void setUrlAnimationFinished(boolean urlHasFocus) { // Hide the icon when the url unfocus animation finishes. // Note: When mUrlFocusPercent is non-zero, that means we're still in the focused state from // scrolling on the NTP. - if (!urlHasFocus && MathUtils.areFloatsEqual(mUrlFocusPercent, 0f) + if (!showExpandedState && MathUtils.areFloatsEqual(mUrlFocusPercent, 0f) && mSearchEngineLogoUtils.currentlyOnNTP(mLocationBarDataProvider)) { setStatusIconShown(false); } diff --git a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java index f40a9967d7429a..822e62e1827ef7 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java @@ -140,6 +140,7 @@ public class ToolbarPhone extends ToolbarLayout implements OnClickListener, TabC @ViewDebug.ExportedProperty(category = "chrome") protected int mTabSwitcherState; + private boolean mIsShowingStartSurface; // This determines whether or not the toolbar draws as expected (false) or whether it always // draws as if it's showing the non-tabswitcher, non-animating toolbar. This is used in grabbing @@ -472,14 +473,14 @@ protected boolean handleEnterKeyPress() { } }); - // Calls the {@link triggerUrlFocusAnimation()} here if it is skipped in {@link - // handleOmniboxInOverviewMode()}. In the overview mode focusing omnibox should also show - // keyboard. + // Calls the {@link triggerUrlFocusAnimation()} here to finish the pending focus request if + // it is skipped in {@link handleOmniboxInOverviewMode()}. if (mPendingTriggerUrlFocusRequest) { + // This pending focus request must be from user's click on the fake omnibox on start + // surface before native library is ready. + assert getToolbarDataProvider().isInOverviewAndShowingOmnibox(); mPendingTriggerUrlFocusRequest = false; - boolean hasFocus = - getToolbarDataProvider().isInOverviewAndShowingOmnibox() && !urlHasFocus(); - triggerUrlFocusAnimation(hasFocus, /* shouldShowKeyboard= */ hasFocus); + triggerUrlFocusAnimation(true); } updateVisualsForLocationBarState(); @@ -1859,6 +1860,7 @@ private void updateTabSwitcherButtonRipple() { */ private boolean handleOmniboxInOverviewMode(boolean inTabSwitcherMode) { if (!getToolbarDataProvider().shouldShowLocationBarInOverviewMode()) return false; + mIsShowingStartSurface = inTabSwitcherMode; if (mToggleTabStackButton != null) { boolean isGone = inTabSwitcherMode; @@ -1871,15 +1873,8 @@ private boolean handleOmniboxInOverviewMode(boolean inTabSwitcherMode) { // possible that this function is called before native initialization when Instant Start // is enabled. Keyboard shouldn't be shown here. if (isNativeLibraryReady()) { - boolean hasFocus = inTabSwitcherMode && !urlHasFocus(); - boolean shouldShowKeyboard = false; - // When switching out of the tab switcher and the url has got focused, we don't clear - // the focus. - if (!inTabSwitcherMode && urlHasFocus()) { - hasFocus = true; - shouldShowKeyboard = true; - } - triggerUrlFocusAnimation(hasFocus, shouldShowKeyboard); + // When the url has got focused, we don't clear the focus. + triggerUrlFocusAnimation(urlHasFocus()); } else { mPendingTriggerUrlFocusRequest = true; } @@ -1981,7 +1976,7 @@ public void set(TextView view, Integer scrollX) { }; } - private void populateUrlFocusingAnimatorSet(List animators) { + private void populateUrlExpansionAnimatorSet(List animators) { TraceEvent.begin("ToolbarPhone.populateUrlFocusingAnimatorSet"); Animator animator = ObjectAnimator.ofFloat(this, mUrlFocusChangeFractionProperty, 1f); animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS); @@ -2036,7 +2031,7 @@ private void populateUrlFocusingAnimatorSet(List animators) { TraceEvent.end("ToolbarPhone.populateUrlFocusingAnimatorSet"); } - private void populateUrlClearFocusingAnimatorSet(List animators) { + private void populateUrlClearExpansionAnimatorSet(List animators) { Animator animator = ObjectAnimator.ofFloat(this, mUrlFocusChangeFractionProperty, 0f); animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS); animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE); @@ -2096,21 +2091,24 @@ public void onUrlFocusChange(final boolean hasFocus) { super.onUrlFocusChange(hasFocus); if (mToggleTabStackButton != null) mToggleTabStackButton.setClickable(!hasFocus); - - if (getToolbarDataProvider().isInOverviewAndShowingOmnibox()) { - mUrlBar.setText(""); - if (!hasFocus) return; - } - - triggerUrlFocusAnimation(hasFocus, /* shouldShowKeyboard= */ hasFocus); + triggerUrlFocusAnimation(hasFocus); } /** - * @param hasFocus Whether the URL field has gained focus. - * @param shouldShowKeyboard Whether the keyboard should be shown. This value should be the same - * as hasFocus by default. + * @param showExpandedState Whether the url bar should be expanded. */ - private void triggerUrlFocusAnimation(final boolean hasFocus, boolean shouldShowKeyboard) { + private void triggerUrlFocusAnimation(boolean showExpandedState) { + boolean shouldShowKeyboard = urlHasFocus(); + + // Sometimes when it's exiting start surface and showing a normal tab, + // |getToolbarDataProvider().isInOverviewAndShowingOmnibox()| is not updated yet and still + // true. So we need to check |mIsEnteringStartSurface| here. + if (mIsShowingStartSurface) { + // If now it's on start surface, omnibox should be expanded without showing the keyboard + // even though it's not focus. + showExpandedState = true; + } + TraceEvent.begin("ToolbarPhone.triggerUrlFocusAnimation"); if (mUrlFocusLayoutAnimator != null && mUrlFocusLayoutAnimator.isRunning()) { mUrlFocusLayoutAnimator.cancel(); @@ -2119,19 +2117,21 @@ private void triggerUrlFocusAnimation(final boolean hasFocus, boolean shouldShow if (mOptionalButtonAnimationRunning) mOptionalButtonAnimator.end(); List animators = new ArrayList<>(); - if (hasFocus) { - populateUrlFocusingAnimatorSet(animators); + if (showExpandedState) { + populateUrlExpansionAnimatorSet(animators); } else { - populateUrlClearFocusingAnimatorSet(animators); + populateUrlClearExpansionAnimatorSet(animators); } mUrlFocusLayoutAnimator = new AnimatorSet(); mUrlFocusLayoutAnimator.playTogether(animators); mUrlFocusChangeInProgress = true; + // |showExpandedState| needs to be final when accessed within inner class. + final boolean showExpandedStateFinal = showExpandedState; mUrlFocusLayoutAnimator.addListener(new CancelAwareAnimatorListener() { @Override public void onStart(Animator animation) { - if (!hasFocus) { + if (!showExpandedStateFinal) { mDisableLocationBarRelayout = true; } else { mLayoutLocationBarInFocusedMode = true; @@ -2141,19 +2141,19 @@ public void onStart(Animator animation) { @Override public void onCancel(Animator animation) { - if (!hasFocus) mDisableLocationBarRelayout = false; + if (!showExpandedStateFinal) mDisableLocationBarRelayout = false; mUrlFocusChangeInProgress = false; } @Override public void onEnd(Animator animation) { - if (!hasFocus) { + if (!showExpandedStateFinal) { mDisableLocationBarRelayout = false; mLayoutLocationBarInFocusedMode = false; requestLayout(); } - mLocationBar.finishUrlFocusChange(hasFocus, shouldShowKeyboard, + mLocationBar.finishUrlFocusChange(showExpandedStateFinal, shouldShowKeyboard, getToolbarDataProvider().shouldShowLocationBarInOverviewMode()); mUrlFocusChangeInProgress = false; } @@ -2303,7 +2303,7 @@ private void updateNtpAnimationState() { if (mTabSwitcherState == STATIC_TAB && previousNtpScrollFraction > 0f) { mUrlFocusChangeFraction = Math.max(previousNtpScrollFraction, mUrlFocusChangeFraction); - triggerUrlFocusAnimation(/* hasFocus= */ false, /* shouldShowKeyboard= */ false); + triggerUrlFocusAnimation(false); } requestLayout(); }