Skip to content

Commit

Permalink
Add a hint for the visible text in the URL bar.
Browse files Browse the repository at this point in the history
The aim is to use this hint in the toolbar screenshots and
to not recapture a screenshot if the visible URL text is unchanged.

BUG=

Change-Id: Ia4f6badf4152cd74b2201963d499e90885b5adb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3956657
Commit-Queue: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1064937}
  • Loading branch information
Ted Choc authored and Chromium LUCI CQ committed Oct 28, 2022
1 parent f78c873 commit a826a8f
Show file tree
Hide file tree
Showing 3 changed files with 276 additions and 6 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ui/android/omnibox/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ proto_java_library("partner_location_descriptor_proto_java") {
android_library("unit_device_javatests") {
testonly = true
sources = [
"java/src/org/chromium/chrome/browser/omnibox/UrlBarUiUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/status/StatusViewRenderTest.java",
"java/src/org/chromium/chrome/browser/omnibox/status/StatusViewTest.java",
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ public abstract class UrlBar extends AutocompleteEditText {
private int mPreviousScrollResultXPosition;
private float mPreviousScrollFontSize;
private boolean mPreviousScrollWasRtl;
private CharSequence mVisibleTextPrefixHint;

// Used as a hint to indicate the text may contain an ellipsize span. This will be true if an
// ellispize span was applied the last time the text changed. A true value here does not
Expand Down Expand Up @@ -673,6 +674,30 @@ public void setScrollState(@ScrollType int scrollType, int scrollToIndex) {
scrollDisplayText();
}

/**
* Return a hint of the currently visible text displayed on screen.
*
* The hint represents the substring of the full text from the first character to the last
* visible character displayed on screen. Depending on the length of this prefix, not all of the
* characters will e displayed on screen.
*
* This will null if:
* <ul>
* <li>The width constraints have changed since the hint was calculated.</li>
* <li>The hint could not be correctly calculated.</li>
* <li>The visible text is narrower than the viewport.</li>
* </ul>
*/
@Nullable
public CharSequence getVisibleTextPrefixHint() {
if (getVisibleMeasuredViewportWidth() != mPreviousScrollViewWidth) return null;
return mVisibleTextPrefixHint;
}

private int getVisibleMeasuredViewportWidth() {
return getMeasuredWidth() - (getPaddingLeft() + getPaddingRight());
}

/**
* Scrolls the omnibox text to a position determined by the current scroll type.
*
Expand Down Expand Up @@ -708,7 +733,7 @@ private void scrollDisplayTextInternal(@ScrollType int scrollType) {
float currentTextSize = getTextSize();
boolean currentIsRtl = getLayoutDirection() == LAYOUT_DIRECTION_RTL;

int measuredWidth = getMeasuredWidth() - (getPaddingLeft() + getPaddingRight());
int measuredWidth = getVisibleMeasuredViewportWidth();
if (scrollType == mPreviousScrollType && TextUtils.equals(text, mPreviousScrollText)
&& measuredWidth == mPreviousScrollViewWidth
// Font size is float but it changes in discrete range (eg small font, big font),
Expand Down Expand Up @@ -743,6 +768,10 @@ private void scrollDisplayTextInternal(@ScrollType int scrollType) {
* Scrolls the omnibox text to show the very beginning of the text entered.
*/
private void scrollToBeginning() {
// Clearly the visible text hint as this path is not used for normal browser navigation.
// If that changes in the future, update this to actually calculate the visible text hints.
mVisibleTextPrefixHint = null;

Editable text = getText();
float scrollPos = 0f;
if (TextUtils.isEmpty(text)) {
Expand All @@ -768,29 +797,73 @@ private void scrollToBeginning() {
*/
private void scrollToTLD() {
Editable url = getText();
int measuredWidth = getMeasuredWidth() - (getPaddingLeft() + getPaddingRight());
int measuredWidth = getVisibleMeasuredViewportWidth();
int urlTextLength = url.length();

Layout textLayout = getLayout();
assert getLayout().getLineCount() == 1;
final int originEndIndex = Math.min(mOriginEndIndex, url.length());
if (mOriginEndIndex > url.length()) {
final int originEndIndex = Math.min(mOriginEndIndex, urlTextLength);
if (mOriginEndIndex > urlTextLength) {
// If discovered locally, please update crbug.com/859219 with the steps to reproduce.
assert false : "Attempting to scroll past the end of the URL: " + url + ", end index: "
+ mOriginEndIndex;
}
float endPointX = textLayout.getPrimaryHorizontal(originEndIndex);
// Compare the position offset of the last character and the character prior to determine
// the LTR-ness of the final component of the URL.
float priorToEndPointX = url.length() == 1
float priorToEndPointX = urlTextLength == 1
? 0
: textLayout.getPrimaryHorizontal(Math.max(0, originEndIndex - 1));

float scrollPos;
if (priorToEndPointX < endPointX) {
// LTR
scrollPos = Math.max(0, endPointX - measuredWidth);
if (endPointX > measuredWidth) {
// To avoid issues where a small portion of the character following originEndIndex
// is visible on screen, be more conservative and extend the visual hint by an
// additional character (this was not reproducible locally at time of authoring, but
// the complexities of font rendering / measurement suggest a conservative approach
// at the start).
//
// While this approach uses an additional character to ensure a conservative and
// more reliable hint signal, this could also use pixel based padding to get the
// visible character XX pixels past the end of the viewport. Potentially, utilizing
// both the additional character and pixel based padding and using the more
// conservative of the two could be done if this current approach is not always
// reliable.
mVisibleTextPrefixHint =
url.subSequence(0, Math.min(originEndIndex + 1, urlTextLength));
} else {
int finalVisibleCharIndex = textLayout.getOffsetForHorizontal(0, measuredWidth);
if (finalVisibleCharIndex == urlTextLength
&& textLayout.getPrimaryHorizontal(finalVisibleCharIndex)
<= measuredWidth) {
// Only store the visibility hint if the text is wider than the viewport. Text
// narrower than the viewport is not a useful hint because a consumer would not
// understand if a subsequent character would be visible on screen or not.
//
// If issues arise where text that is very close to the visible viewport is
// causing issues with the reliability of visible hint, consider checking that
// the measured text is greater than the measured width plus a small additional
// padding.
mVisibleTextPrefixHint = null;
} else {
// To avoid issues where a small portion of the character following
// finalVisibleCharIndex is visible on screen, be more conservative and extend
// the visual hint by an additional character. In testing,
// getOffsetForHorizontal returns the last fully visible character on screen.
// By extending the offset by an additional character, the risk is of having
// visual artifacts from the subsequence character on screen is mitigated.
mVisibleTextPrefixHint =
url.subSequence(0, Math.min(finalVisibleCharIndex + 1, urlTextLength));
}
}
} else {
// RTL
// Clear the visible text hint due to the complexities of Bi-Di text handling. If RTL or
// Bi-Di URLs become more prevalant, update this to correctly calculate the hint.
mVisibleTextPrefixHint = null;

// To handle BiDirectional text, search backward from the two existing offsets to find
// the first LTR character. Ensure the final RTL component of the domain is visible
Expand Down Expand Up @@ -829,8 +902,8 @@ protected void onLayout(boolean changed, int left, int top, int right, int botto
scrollDisplayTextInternal(mScrollType);
} else if (mPreviousWidth != (right - left)) {
scrollDisplayTextInternal(mScrollType);
mPreviousWidth = right - left;
}
mPreviousWidth = right - left;
}

@Override
Expand All @@ -857,6 +930,11 @@ public void setText(CharSequence text, BufferType type) {
if (DEBUG) Log.i(TAG, "setText -- text: %s", text);
super.setText(text, type);
fixupTextDirection();

if (mVisibleTextPrefixHint != null
&& (text == null || TextUtils.indexOf(text, mVisibleTextPrefixHint) != 0)) {
mVisibleTextPrefixHint = null;
}
}

private void limitDisplayableLength() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
// Copyright 2022 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.omnibox;

import android.app.Activity;
import android.text.TextUtils;
import android.view.ViewGroup;
import android.widget.FrameLayout;

import androidx.test.filters.SmallTest;

import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.runner.RunWith;

import org.chromium.base.test.BaseActivityTestRule;
import org.chromium.base.test.util.Batch;
import org.chromium.base.test.util.Criteria;
import org.chromium.base.test.util.CriteriaHelper;
import org.chromium.base.test.util.Feature;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.ui.test.util.BlankUiTestActivity;

import java.util.Collections;

/**
* Unit tests that rely on UI rendering for UrlBar.
*/
@RunWith(ChromeJUnit4ClassRunner.class)
@Batch(Batch.PER_CLASS)
public class UrlBarUiUnitTest {
@ClassRule
public static final BaseActivityTestRule<BlankUiTestActivity> sActivityTestRule =
new BaseActivityTestRule<>(BlankUiTestActivity.class);

private static Activity sActivity;
private static FrameLayout sContentView;

private UrlBar mUrlBar;

@BeforeClass
public static void setupSuite() {
sActivityTestRule.launchActivity(null);
TestThreadUtils.runOnUiThreadBlocking(() -> {
sActivity = sActivityTestRule.getActivity();
sContentView = new FrameLayout(sActivity);
sContentView.setLayoutParams(
new ViewGroup.MarginLayoutParams(ViewGroup.LayoutParams.MATCH_PARENT,
sActivity.getResources().getDimensionPixelSize(
org.chromium.chrome.R.dimen.control_container_height)));
sActivity.setContentView(sContentView);
});
}

@Before
public void setupTest() {
TestThreadUtils.runOnUiThreadBlocking(() -> {
sContentView.removeAllViews();
sActivity.getLayoutInflater().inflate(
org.chromium.chrome.R.layout.url_bar, sContentView);
mUrlBar = (UrlBar) sContentView.getChildAt(0);
});
}

private static void assertTextEquals(CharSequence a, CharSequence b) {
Assert.assertTrue(a + " should match: " + b, TextUtils.equals(a, b));
}

private void waitForUrlBarLayout() {
CriteriaHelper.pollUiThread(() -> {
Criteria.checkThat(mUrlBar.isLayoutRequested(), Matchers.is(false));
Criteria.checkThat(mUrlBar.isInLayout(), Matchers.is(false));
});
}

private void updateUrlBarText(
CharSequence text, @UrlBar.ScrollType int scrollType, int scrollIndex) {
TestThreadUtils.runOnUiThreadBlocking(() -> {
mUrlBar.setText(text);
mUrlBar.setScrollState(scrollType, scrollIndex);
});
waitForUrlBarLayout();
}

private CharSequence getUrlText() {
return TestThreadUtils.runOnUiThreadBlockingNoException(() -> mUrlBar.getText());
}

private CharSequence getVisibleTextPrefixHint() {
return TestThreadUtils.runOnUiThreadBlockingNoException(
() -> mUrlBar.getVisibleTextPrefixHint());
}

@Test
@SmallTest
@Feature("Omnibox")
public void testVisibleTextPrefixHint_ShortUrl() throws Exception {
String url = "www.test.com";
updateUrlBarText(url, UrlBar.ScrollType.SCROLL_TO_TLD, url.length());

TestThreadUtils.runOnUiThreadBlocking(() -> {
float scrollXPosForEndOfUrlText =
mUrlBar.getLayout().getPrimaryHorizontal(mUrlBar.getText().length());
Assert.assertThat(scrollXPosForEndOfUrlText,
Matchers.lessThan((float) mUrlBar.getMeasuredWidth()));
});

Assert.assertNull(getVisibleTextPrefixHint());
}

@Test
@SmallTest
@Feature("Omnibox")
public void testVisibleTextPrefixHint_ShortTld_LongPath() throws Exception {
final String domain = "www.test.com";
final String path = "/" + TextUtils.join("", Collections.nCopies(500, "a"));
updateUrlBarText(domain + path, UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());

TestThreadUtils.runOnUiThreadBlocking(() -> {
float scrollXPosForEndOfUrlText =
mUrlBar.getLayout().getPrimaryHorizontal(mUrlBar.getText().length());
Assert.assertThat(scrollXPosForEndOfUrlText,
Matchers.greaterThan((float) mUrlBar.getMeasuredWidth()));
});

final CharSequence prefixHint = getVisibleTextPrefixHint();
CharSequence urlText = getUrlText();
Assert.assertNotNull(prefixHint);
Assert.assertTrue("Expected url text: '" + urlText + "' starts with " + prefixHint,
TextUtils.indexOf(urlText, prefixHint) == 0);
Assert.assertThat(prefixHint.length(), Matchers.lessThan(urlText.length()));

// Append a string to the already long initial text and validate the prefix doesn't change.
updateUrlBarText(getUrlText() + "bbbbbbbbbbbbbbbbbbbbbbb", UrlBar.ScrollType.SCROLL_TO_TLD,
domain.length());
assertTextEquals(prefixHint, getVisibleTextPrefixHint());

// Append a character to just the hint prefix text and validate the prefix doesn't change.
updateUrlBarText(prefixHint + "a", UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());
assertTextEquals(prefixHint, getVisibleTextPrefixHint());

// Set the text to just the prefix text and ensure the hint remains unchanged.
updateUrlBarText(prefixHint, UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());
assertTextEquals(prefixHint, getVisibleTextPrefixHint());

// Set the text to be slightly shorter than the prefix, which will result in the text
// being shorter than the visual viewport, and thus generate a null hint text.
//
// We subtract by 2 because an additional trailing char is added to the visible text to
// account for rounding issues with text positioning.
updateUrlBarText(TextUtils.substring(prefixHint, 0, prefixHint.length() - 2),
UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());
Assert.assertNull(getVisibleTextPrefixHint());
}

@Test
@SmallTest
@Feature("Omnibox")
public void testVisibleTextPrefixHint_LongTld() throws Exception {
final String domain = "www." + TextUtils.join("", Collections.nCopies(500, "a")) + ".com";
updateUrlBarText(domain, UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());

final CharSequence urlText = getUrlText();
CharSequence prefixHint = getVisibleTextPrefixHint();
Assert.assertNotNull(prefixHint);
assertTextEquals(urlText, prefixHint);

updateUrlBarText(
getUrlText() + "/foooooo", UrlBar.ScrollType.SCROLL_TO_TLD, domain.length());
assertTextEquals(urlText + "/", getVisibleTextPrefixHint());
}

@Test
@SmallTest
@Feature("Omnibox")
public void testVisibleTextPrefixHint_NonUrlText() {
updateUrlBarText("a", UrlBar.ScrollType.SCROLL_TO_BEGINNING, 0);
Assert.assertNull(getVisibleTextPrefixHint());

updateUrlBarText(TextUtils.join("", Collections.nCopies(500, "a")),
UrlBar.ScrollType.SCROLL_TO_BEGINNING, 0);
Assert.assertNull(getVisibleTextPrefixHint());
}
}

0 comments on commit a826a8f

Please sign in to comment.