Commit bd2526d9 authored by Carlos IL's avatar Carlos IL Committed by Commit Bot

Revert "Fix elision for bidirectional URLs"

This reverts commit bdc3ad55.

Reason for revert: Bidi handling caused bugs for non-bidi URLs, for the time being this will be reverted while those are fixed, and a CL disabling elision experiments for bidi URLs will be merged to 86.

Original change's description:
> Fix elision for bidirectional URLs
> 
> URLs where the TLD is separate from the host, which can happen for URLs
> of the form http://LTR.RTL/RTL, were displayed with a blank spot when
> elision was enabled. This CL changes this so the in between section,
> which is not elided in this case, is also not colored transparent.
> 
> Bug: 1117631
> Change-Id: I9bba2d2c0d494a4021b8c8edaacc87084e040964
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368458
> Reviewed-by: Tommy Li <tommycli@chromium.org>
> Reviewed-by: Emily Stark <estark@chromium.org>
> Commit-Queue: Carlos IL <carlosil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#801559}

TBR=tommycli@chromium.org,estark@chromium.org,carlosil@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1117631
Change-Id: Idc2ac1d682b1ec5ca1c5d8efc60dad70e0f203d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2387483Reviewed-by: default avatarCarlos IL <carlosil@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803650}
parent 7d9ce46b
......@@ -188,6 +188,9 @@ OmniboxViewViews::ElideAnimation::ElideAnimation(OmniboxViewViews* view,
OmniboxViewViews::ElideAnimation::~ElideAnimation() = default;
// TODO(estark): this code doesn't work for URLs with RTL components. Will need
// to figure out another animation or just skip the animation entirely on URLs
// with RTL components.
void OmniboxViewViews::ElideAnimation::Start(
const gfx::Range& elide_to_bounds,
uint32_t delay_ms,
......@@ -196,39 +199,25 @@ void OmniboxViewViews::ElideAnimation::Start(
SkColor ending_color) {
DCHECK(ranges_surrounding_simplified_domain.size() == 1 ||
ranges_surrounding_simplified_domain.size() == 2);
ranges_surrounding_simplified_domain_ = ranges_surrounding_simplified_domain;
starting_color_ = starting_color;
ending_color_ = ending_color;
// simplified_domain_bounds_ will be set to a rectangle surrounding the part
// of the URL that is never elided, on its original position before any
// animation runs. If ranges_surrounding_simplified_domain only contains one
// animation runs. If ranges_surrounding_simplified_domain_ only contains one
// range it means we are not eliding on the right side, so we use the right
// side of elide_to_bounds as the range as it will always be the right limit
// of the simplified section.
gfx::Range simplified_domain_range(
ranges_surrounding_simplified_domain[0].end(),
ranges_surrounding_simplified_domain.size() == 2
? ranges_surrounding_simplified_domain[1].start()
ranges_surrounding_simplified_domain_[0].end(),
ranges_surrounding_simplified_domain_.size() == 2
? ranges_surrounding_simplified_domain_[1].start()
: elide_to_bounds.end());
for (auto rect : render_text_->GetSubstringBounds(simplified_domain_range)) {
simplified_domain_bounds_.Union(rect - render_text_->GetLineOffset(0));
}
// Keep track of which ranges need to be colored. Any range that when
// displayed is in the middle of the unelided section (which can happen for
// certain bidirectional URLs) won't be elided, and should not be colored.
for (auto range : ranges_surrounding_simplified_domain) {
if (range.length() > 0) {
gfx::Rect range_bounds;
for (auto rect : render_text_->GetSubstringBounds(range))
range_bounds.Union(rect - render_text_->GetLineOffset(0));
if (!simplified_domain_bounds_.Contains(range_bounds)) {
ranges_to_color_.push_back(range);
}
}
}
// After computing |elide_to_rect_| below, |elide_to_bounds| aren't actually
// need anymore for the animation. However, the bounds provide a convenient
// way for the animation consumer to check if an animation is currently in
......@@ -333,7 +322,7 @@ void OmniboxViewViews::ElideAnimation::AnimationProgressed(
ending_display_offset_);
render_text_->SetDisplayOffset(current_offset_);
for (const auto& range : ranges_to_color_) {
for (const auto& range : ranges_surrounding_simplified_domain_) {
view_->ApplyColor(GetCurrentColor(), range);
}
......@@ -2716,19 +2705,9 @@ void OmniboxViewViews::ElideURL() {
// GetSubstringBounds() rounds outward internally, so there may be small
// portions of text still showing. Set the ranges surrounding the simplified
// domain to transparent so that these artifacts don't show. Skip this if
// the range is inside the simplified domain bounds (which can happen for
// some bidirectional URLs), since it won't be elided in that case.
for (const auto& range : ranges_surrounding_simplified_domain) {
if (range.length() > 0) {
gfx::Rect range_bounds;
for (auto rect : GetRenderText()->GetSubstringBounds(range))
range_bounds.Union(rect);
if (!shifted_simplified_domain_rect.Contains(range_bounds)) {
ApplyColor(SK_ColorTRANSPARENT, range);
}
}
}
// domain to transparent so that these artifacts don't show.
for (const auto& range : ranges_surrounding_simplified_domain)
ApplyColor(SK_ColorTRANSPARENT, range);
}
void OmniboxViewViews::ShowFullURL() {
......
......@@ -256,11 +256,9 @@ class OmniboxViewViews : public OmniboxView,
// The current offset, exposed for testing.
int current_offset_;
// As the animation runs, each range fades from |starting_color_| to
// |ending_color_|. This is normally the ranges surrounding the simplified
// domain, but can omit one of them if the simplified domain is not
// displayed contiguously (which can happen for some bidirectional URLs).
std::vector<gfx::Range> ranges_to_color_;
// Holds the ranges surrounding the simplified domain part. As the animation
// runs, each range fades from |starting_color_| to |ending_color_|.
std::vector<gfx::Range> ranges_surrounding_simplified_domain_;
SkColor starting_color_;
SkColor ending_color_;
......@@ -284,8 +282,6 @@ class OmniboxViewViews : public OmniboxView,
private:
FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, HoverAndExit);
FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, HoverAndExitIDN);
FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
PathNotTransparentSplitURL);
FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, PrivateRegistry);
FRIEND_TEST_ALL_PREFIXES(
OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
......
......@@ -1897,50 +1897,6 @@ TEST_P(OmniboxViewViewsRevealOnHoverTest, HoverAndExitIDN) {
kSimplifiedDomainDisplayIDNUrlPath, ShouldElideToRegistrableDomain()));
}
// Tests the path doesn't disappear for a URL where it appears in between of the
// unelided section (and thus is not elided).
TEST_P(OmniboxViewViewsRevealOnHoverTest, PathNotTransparentSplitURL) {
// A bidirectional URL with this format causes the tld (مثال) to appear
// separate from the host (test), and with the path (إختبار) displayed in
// between.
const base::string16 kSimplifiedDomainDisplaySplitUrl =
base::UTF8ToUTF16("https://test.مثال/إختبار");
const base::string16 kSimplifiedDomainDisplaySplitUrlHostnameAndScheme =
base::UTF8ToUTF16("https://test.مثال");
UpdateDisplayURL(kSimplifiedDomainDisplaySplitUrl);
// Call OnThemeChanged() to create the animations.
omnibox_view()->OnThemeChanged();
// Check that the path is not transparent.
EXPECT_NE(SK_ColorTRANSPARENT,
omnibox_view()->GetLatestColorForRange(gfx::Range(
kSimplifiedDomainDisplaySplitUrlHostnameAndScheme.size(),
kSimplifiedDomainDisplaySplitUrl.size())));
// Simulate mouse hovering to trigger the unelision animation.
omnibox_view()->OnMouseMoved(CreateMouseEvent(ui::ET_MOUSE_MOVED, {0, 0}));
OmniboxViewViews::ElideAnimation* hover_animation =
omnibox_view()->GetHoverElideOrUnelideAnimationForTesting();
ASSERT_TRUE(hover_animation);
ASSERT_TRUE(hover_animation->IsAnimating());
// Advance the clock to let the elision animation finish. Assume it takes less
// than 2 seconds.
omnibox_view()->StepSimplifiedDomainHoverAnimation(2000);
// Exit the mouse and let the elision animation run until it finishes.
omnibox_view()->OnMouseExited(CreateMouseEvent(ui::ET_MOUSE_EXITED, {0, 0}));
ASSERT_TRUE(hover_animation->IsAnimating());
omnibox_view()->StepSimplifiedDomainHoverAnimation(2000);
// Check the path is still not transparent after the animation runs.
EXPECT_NE(SK_ColorTRANSPARENT,
omnibox_view()->GetLatestColorForRange(gfx::Range(
kSimplifiedDomainDisplaySplitUrlHostnameAndScheme.size(),
kSimplifiedDomainDisplaySplitUrl.size())));
}
// Tests the field trial variation that shows a simplified domain by default
// using a private registry (https://publicsuffix.org/list/). Private registries
// should be ignored when computing the simplified domain, to avoid creating
......
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