Skip to content

Commit

Permalink
Allow keyboard to be dismissed even if suggestions list doesn't scroll
Browse files Browse the repository at this point in the history
The suggestions list doesn't need to scroll for suggestions to show
beneath the keyboard, even though it is most of then the case.

In certain situations, additional suggestions may be available below
the keyboard, but their number may not make the list scrollable.

This change relaxes the current restriction, permitting users to
dismiss the keyboard even if suggestions list doesn't show.

(cherry picked from commit 4300766)

Change-Id: I478a17da65e01124b35d2e4a29f74538b00bc138
Fixed: 1479437
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4850684
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Auto-Submit: Tomasz Wiszkowski <ender@google.com>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1194189}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4857107
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5993@{#187}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}
  • Loading branch information
tomasz-wiszkowski authored and Chromium LUCI CQ committed Sep 12, 2023
1 parent 9772489 commit 023392d
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,9 @@ public int scrollVerticallyBy(
// visibility), or
// 2. the list is too short, and almost entirely fits on the screen, leaving at most
// just a few pixels of content hiding under the keyboard.
// There's no need to dismiss the keyboard in any of these cases.
if (resultingDeltaY < requestedDeltaY) return resultingDeltaY;
// Note that the list may extend below the keyboard and still be non-scrollable:
// http://crbug/1479437

// Otherwise decide whether keyboard should be shown or not.
// We want to call keyboard up only when we know we reached the top of the list.
// Note: the condition below evaluates `true` only if the scroll direction is "up",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,36 +221,41 @@ public void testScrollListener_keyboardShouldShowOnScrollToTop() {

@Test
@SmallTest
public void testScrollListener_notDismissingKeyboardWhenScrollDoesNotHappen() {
public void testScrollListener_dismissingKeyboardWhenScrollDoesNotHappen() {
// In some cases the list may be long enough to stretch below the keyboard, but not long
// enough to be scrollable. We want to dismiss the keyboard in these cases, too.
mDropdown.setSuggestionDropdownScrollListener(mDropdownScrollListener);
mDropdown.setSuggestionDropdownOverscrolledToTopListener(mDropdownScrollToTopListener);

// Pretend we're scrolling down (delta=10) but there is no content to move to (scroll=0).
assertEquals(0, mListener.updateKeyboardVisibilityAndScroll(0, 10));
// Confirm that we're not hiding the keyboard.
verifyNoMoreInteractions(mDropdownScrollListener);
// Confirm that we're hiding the keyboard.
verify(mDropdownScrollListener).run();

// Pretend we're scrolling up now (delta=-10) but we're already on top and can't move.
assertEquals(0, mListener.updateKeyboardVisibilityAndScroll(0, -10));
// Confirm that we're not trying to show the keyboard.
verifyNoMoreInteractions(mDropdownScrollListener);
verify(mDropdownScrollToTopListener).run();

verifyNoMoreInteractions(mDropdownScrollListener, mDropdownScrollToTopListener);
}

@Test
@SmallTest
public void testScrollListener_notDismissingKeyboardWhenTheListIsOnlyBarelyUnderTheKeyboard() {
public void testScrollListener_dismissingKeyboardWhenTheListIsOnlyBarelyUnderTheKeyboard() {
mDropdown.setSuggestionDropdownScrollListener(mDropdownScrollListener);
mDropdown.setSuggestionDropdownOverscrolledToTopListener(mDropdownScrollToTopListener);

// We want to scroll by 10px, but there's only 1px of content. Don't hide the keyboard.
assertEquals(1, mListener.updateKeyboardVisibilityAndScroll(1, 10));
verifyNoMoreInteractions(mDropdownScrollListener);

// We want to scroll by 10px, but there's only 9px of content. Don't hide the keyboard.
assertEquals(9, mListener.updateKeyboardVisibilityAndScroll(9, 10));
verifyNoMoreInteractions(mDropdownScrollListener);
// We want to scroll by 10px, but there's only 1px of slack. This means the suggestions list
// spans entirely under the keyboard. Hide the keyboard.
assertEquals(0, mListener.updateKeyboardVisibilityAndScroll(1, 10));
verify(mDropdownScrollListener).run();

// But then, if we scroll back up, we likely should not ask for keyboard to show.
// Reset keyboard state.
assertEquals(-9, mListener.updateKeyboardVisibilityAndScroll(-9, -10));
verifyNoMoreInteractions(mDropdownScrollListener);
verify(mDropdownScrollToTopListener).run();

verifyNoMoreInteractions(mDropdownScrollListener, mDropdownScrollToTopListener);
}

@Test
Expand Down

0 comments on commit 023392d

Please sign in to comment.