Commit 20939185 authored by Emily Stark's avatar Emily Stark Committed by Commit Bot

Reland "Restore omnibox bounds when resetting if not eligible for eliding"

This reverts commit 56db3421.

Reason for revert: Fixing bug that we were going into ELIDE_TAIL mode
when focused

Original change's description:
> Revert "Restore omnibox bounds when resetting if not eligible for eliding"
> 
> This reverts commit 8ea67049.
> 
> Reason for revert: I noticed that this change causes the omnibox to be in ELIDE_TAIL mode during editing, which causes a DCHECK failure.
> 
> Original change's description:
> > Restore omnibox bounds when resetting if not eligible for eliding
> > 
> > This CL fixes a bug in the code that resets to the on-page-load state
> > for the simplified domain hide-on-interaction field trial.
> > 
> > The bug is that this reset code let the omnibox bounds go unchanged
> > if the new text was not eligible for simplified domain eliding.
> > For example, when switching to a new tab (as shown in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=1112577#c4), the
> > placeholder text could get cut off because the omnibox was remaining
> > at the bounds used for the last simplified domain elision. The fix is
> > to restore the omnibox to its local bounds, with ELIDE_TAIL behavior,
> > when the text is not eligible for simplified domain eliding.
> > 
> > Bug: 1107912, 1112577
> > Change-Id: I36c359831fc1f7b18c814d9144b801655bd3b1c5
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2342123
> > Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
> > Commit-Queue: Emily Stark <estark@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#795953}
> 
> TBR=jdonnelly@chromium.org,estark@chromium.org
> 
> Change-Id: Id44cd8e39064af890399109fcca8ca729ddba5d4
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1107912
> Bug: 1112577
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343153
> Reviewed-by: Emily Stark <estark@chromium.org>
> Commit-Queue: Emily Stark <estark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#796133}

TBR=jdonnelly@chromium.org,estark@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1107912
Bug: 1112577
Change-Id: I4e6c6b4ee1403404cac454f1602a4bab107b6058
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2344654Reviewed-by: default avatarEmily Stark <estark@chromium.org>
Reviewed-by: default avatarJustin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796704}
parent c1ecf650
......@@ -2455,8 +2455,13 @@ void OmniboxViewViews::ResetToHideOnInteraction() {
elide_after_web_contents_interaction_animation_.reset();
hover_elide_or_unelide_animation_ =
std::make_unique<OmniboxViewViews::ElideAnimation>(this, GetRenderText());
if (IsURLEligibleForSimplifiedDomainEliding())
if (IsURLEligibleForSimplifiedDomainEliding()) {
ShowFullURLWithoutSchemeAndTrivialSubdomain();
} else {
if (!HasFocus() && !model()->user_input_in_progress())
GetRenderText()->SetElideBehavior(gfx::ELIDE_TAIL);
FitToLocalBounds();
}
}
void OmniboxViewViews::OnShouldPreventElisionChanged() {
......
......@@ -227,6 +227,9 @@ class OmniboxViewViews : public OmniboxView,
UnsetAlwaysShowFullURLs);
FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
RegistrableDomainRepeated);
FRIEND_TEST_ALL_PREFIXES(
OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
TabChangeWhenNotEligibleForEliding);
FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,
EmitAccessibilityEvents);
// TODO(tommycli): Remove the rest of these friends after porting these
......
......@@ -2290,6 +2290,76 @@ TEST_P(OmniboxViewViewsRevealOnHoverAndMaybeHideOnInteractionTest,
EXPECT_TRUE(elide_animation->IsAnimating());
}
// Tests that in the hide-on-interaction field trial, the omnibox is reset to
// the local bounds on tab change when the new text is not eligible for
// simplified domain elision.
TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
TabChangeWhenNotEligibleForEliding) {
SetUpSimplifiedDomainTest();
content::MockNavigationHandle navigation;
navigation.set_is_same_document(false);
omnibox_view()->DidFinishNavigation(&navigation);
ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
omnibox_view()->GetRenderText(),
gfx::Range(kSimplifiedDomainDisplayUrlScheme.size(),
kSimplifiedDomainDisplayUrl.size())));
// Simulate a user interaction and advance through the animation to elide the
// URL.
omnibox_view()->DidGetUserInteraction(blink::WebKeyboardEvent());
OmniboxViewViews::ElideAnimation* elide_animation =
omnibox_view()->GetElideAfterInteractionAnimationForTesting();
gfx::AnimationContainerElement* elide_as_element =
elide_animation->GetAnimationForTesting();
elide_as_element->SetStartTime(base::TimeTicks());
elide_as_element->Step(base::TimeTicks() + base::TimeDelta::FromSeconds(1));
ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
kSimplifiedDomainDisplayUrlSubdomain,
kSimplifiedDomainDisplayUrlHostnameAndScheme,
kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
// Change the tab and set state such that the current text is not eligible for
// simplified domain eliding (specifically, use an ftp:// URL; only http/https
// URLs are eligible for eliding). The omnibox should take up the full local
// bounds and be reset to tail-eliding behavior, just as if the above
// simplified domain elision had not happened.
location_bar_model()->set_url(GURL("ftp://foo.example.test"));
location_bar_model()->set_url_for_display(
base::ASCIIToUTF16("ftp://foo.example.test"));
omnibox_view()->model()->ResetDisplayTexts();
omnibox_view()->RevertAll();
std::unique_ptr<content::WebContents> web_contents =
content::WebContentsTester::CreateTestWebContents(profile(), nullptr);
omnibox_view()->SaveStateToTab(web_contents.get());
omnibox_view()->OnTabChanged(web_contents.get());
EXPECT_EQ(gfx::ELIDE_TAIL, omnibox_view()->GetRenderText()->elide_behavior());
EXPECT_EQ(base::ASCIIToUTF16("ftp://foo.example.test"),
omnibox_view()->GetRenderText()->GetDisplayText());
// Change the tab and simulate user input in progress. In this case, the
// omnibox should take up the full local bounds but should not be reset to
// tail-eliding behavior, because it should always be in NO_ELIDE mode when
// editing.
location_bar_model()->set_url(GURL(kSimplifiedDomainDisplayUrl));
location_bar_model()->set_url_for_display(kSimplifiedDomainDisplayUrl);
omnibox_view()->model()->ResetDisplayTexts();
omnibox_view()->RevertAll();
omnibox_view()->Focus();
omnibox_view()->model()->SetInputInProgress(true);
std::unique_ptr<content::WebContents> web_contents2 =
content::WebContentsTester::CreateTestWebContents(profile(), nullptr);
omnibox_view()->SaveStateToTab(web_contents2.get());
omnibox_view()->OnTabChanged(web_contents2.get());
EXPECT_EQ(gfx::NO_ELIDE, omnibox_view()->GetRenderText()->elide_behavior());
ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
omnibox_view()->GetRenderText(),
gfx::Range(0, kSimplifiedDomainDisplayUrl.size())));
}
// Tests that in the hide-on-interaction field trial, when the path changes
// while being elided, the animation is stopped.
TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment