New issue
Advanced search Search tips

Issue 903824 link

Starred by 1 user
Status: Fixed
Owner:
fhorschig@chromium.org
Closed: Nov 14
Cc:
maxwalker@chromium.org
ioanap@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 853794



Sign in to add a comment

Accessory bar needs bottom shadow and spacing on scrolling

Project Member Reported by fhorschig@chromium.org, Nov 9 
What steps will reproduce the problem?
(1) Save 3 or more credentials for a site
(2) Focus a password field on that site and click the key icon in the accessory
(3) Scroll down the list of credentials

What is the expected result?
The divider between accessory and sheet is replaced by a shadow (see e.g. Chrome Bookmarks).

What happens instead?
The divider disappears, but there is no shadow. The spacing at the bottom of the list should be 8dp larger (so it visually appears centered between dividers).
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/474ad57905c821f8deb4cb9f434f67330059386e

commit 474ad57905c821f8deb4cb9f434f67330059386e
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Tue Nov 13 18:33:24 2018

[Mfill Android] Add shadow and spacing on scroll

Currently the scrolling causes the top divider to disappear. Without a
shadow separating the accessory bar from the scrollable list, this
looks wrong. Therefore, this CL adds such a shadow.

Also, some bottom spacing related to the scrolling is fixed.

Bug:  903824 
Change-Id: I31cd318cb96fa77db3ba55d8a9a61488e9d6da0b
Reviewed-on: https://chromium-review.googlesource.com/c/1331400
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607665}
[modify] https://crrev.com/474ad57905c821f8deb4cb9f434f67330059386e/chrome/android/java/res/layout/password_accessory_sheet.xml
[modify] https://crrev.com/474ad57905c821f8deb4cb9f434f67330059386e/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetCoordinator.java
[modify] https://crrev.com/474ad57905c821f8deb4cb9f434f67330059386e/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java
[modify] https://crrev.com/474ad57905c821f8deb4cb9f434f67330059386e/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewTest.java
[modify] https://crrev.com/474ad57905c821f8deb4cb9f434f67330059386e/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetControllerTest.java

Comment 2 by fhorschig@google.com, Nov 14

Labels: Merge-Request-71 M-71
Status: Fixed (was: Started)
It would be very nice if we could merge this last fix for M71, ideally before the next Beta release. The change is a small and local, so benefits should outweigh any risk.
Project Member

Comment 3 by sheriffbot@chromium.org, Nov 14

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/979db04c46ffabdcc076d158f1c5be6b6a55accc

commit 979db04c46ffabdcc076d158f1c5be6b6a55accc
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Thu Nov 15 15:49:22 2018

[Mfil Android] Move accessory bar shadow from tab into sheet

The fix for the missing shadow in https://crrev.com/c/1331400 was
intentionally short but not very clean. This CL fixes that by moving the
shadow into the accessory sheet. This ensures that the view state is
again reflected by the model.

Bug:  903824 
Change-Id: I7a7de1cba5586997de8964551b2b4ec94cb38647
Reviewed-on: https://chromium-review.googlesource.com/c/1331587
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608378}
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/res/layout/keyboard_accessory_sheet.xml
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/res/layout/main.xml
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/res/layout/password_accessory_sheet.xml
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetCoordinator.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetMediator.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetProperties.java
[add] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetView.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetViewBinder.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryMetricsRecorder.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingCoordinator.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingMediator.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetCoordinator.java
[add] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetMediator.java
[add] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetProperties.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/java_sources.gni
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetViewTest.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingIntegrationTest.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewTest.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetControllerTest.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingControllerTest.java
[modify] https://crrev.com/979db04c46ffabdcc076d158f1c5be6b6a55accc/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetControllerTest.java

Comment 5 by fhorschig@chromium.org, Nov 15 

(The second, large CL is not part of the merge request. It's a post-fix cleanup.)

Comment 6 by benmason@chromium.org, Nov 15

Labels: -Hotlist-Merge-Review -Merge-Review-71 Merge-Approved-71
Merge approved for 71, branch 3578.

Comment 7 by aska...@chromium.org, Nov 15 

Verified fix (shadow between and accessory and password sheet) in 72.0.3610.2.
Project Member

Comment 8 by cr-audit...@appspot.gserviceaccount.com, Nov 16

Labels: -Merge-Approved-71 Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/4742a2ae21953b4d21cd2678730d4de9673859fd

Commit: 4742a2ae21953b4d21cd2678730d4de9673859fd
Author: fhorschig@chromium.org
Commiter: fhorschig@chromium.org
Date: 2018-11-16 09:33:24 +0000 UTC

[Mfill Android] Add shadow and spacing on scroll

Currently the scrolling causes the top divider to disappear. Without a
shadow separating the accessory bar from the scrollable list, this
looks wrong. Therefore, this CL adds such a shadow.

Also, some bottom spacing related to the scrolling is fixed.

Bug:  903824 
Change-Id: I31cd318cb96fa77db3ba55d8a9a61488e9d6da0b
Reviewed-on: https://chromium-review.googlesource.com/c/1331400
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#607665}(cherry picked from commit 474ad57905c821f8deb4cb9f434f67330059386e)
Reviewed-on: https://chromium-review.googlesource.com/c/1339861
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#728}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 16

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4742a2ae21953b4d21cd2678730d4de9673859fd

commit 4742a2ae21953b4d21cd2678730d4de9673859fd
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Fri Nov 16 09:33:24 2018

[Mfill Android] Add shadow and spacing on scroll

Currently the scrolling causes the top divider to disappear. Without a
shadow separating the accessory bar from the scrollable list, this
looks wrong. Therefore, this CL adds such a shadow.

Also, some bottom spacing related to the scrolling is fixed.

Bug:  903824 
Change-Id: I31cd318cb96fa77db3ba55d8a9a61488e9d6da0b
Reviewed-on: https://chromium-review.googlesource.com/c/1331400
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#607665}(cherry picked from commit 474ad57905c821f8deb4cb9f434f67330059386e)
Reviewed-on: https://chromium-review.googlesource.com/c/1339861
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#728}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/4742a2ae21953b4d21cd2678730d4de9673859fd/chrome/android/java/res/layout/password_accessory_sheet.xml
[modify] https://crrev.com/4742a2ae21953b4d21cd2678730d4de9673859fd/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetCoordinator.java
[modify] https://crrev.com/4742a2ae21953b4d21cd2678730d4de9673859fd/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java
[modify] https://crrev.com/4742a2ae21953b4d21cd2678730d4de9673859fd/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewTest.java
[modify] https://crrev.com/4742a2ae21953b4d21cd2678730d4de9673859fd/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetControllerTest.java
►

Sign in to add a comment