New issue
Advanced search Search tips

Issue 856180 link

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

Blocked on:
issue 887258
issue 900178
Blocking:
issue 853794
issue 865352



Sign in to add a comment

Keyboard accessory deviates from specs

Project Member Reported by fhorschig@chromium.org, Jun 25 2018 
What needs to be adjusted:
- Apply 38% Opacity to non-interactive list items (see spec)
- Blue tone of Recolored key icons should be GBlue 600: #1A73E8
- Padding for tabs in keyboard accessory bar (is 8dp/4dp, should be 6dp)
- Drop shadow is missing above keyboard (see i856138)
- Favicons next to entries (see i853748)


(@maxwalker: if you find something else, please collect it here)

Comment 1 by fhorschig@chromium.org, Jun 25 2018

Blocking: 853794

Comment 2 by fhorschig@chromium.org, Jun 25 2018

Description: Show this description

Comment 3 by maxwalker@chromium.org, Jun 25 2018 

Thanks, Friedrich - super useful! Some additional things I found:

- List items should have standard feedback state (light grey background) on tap (like Chrome menu, Recent tabs, etc.)
- Add 8dp margin-top above first list item
- Text in list items should be vertically aligned
Bugs.png
562 KB View Download

Comment 4 by nepper@chromium.org, Jun 27 2018

Labels: -Pri-2 Pri-1

Comment 5 by ioanap@chromium.org, Jul 18

Cc: fhorschig@chromium.org
Owner: ioanap@chromium.org
Status: Started (was: Assigned)

Comment 6 by fhorschig@chromium.org, Jul 18 

Thanks for tackling this!
Please note that https://crrev.com/c/1138319 (which introduces favicons) affects the suggestions layout quite a lot - it should fix the padding for single suggestions.

Comment 7 by fhorschig@chromium.org, Jul 19

Blocking: 865352
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 8 

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

commit 21ee86ff76372f64585a4b6ead244883025dd342
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Wed Aug 08 15:14:46 2018

[Android] Resize keyboard accessory sheet according to keyboard

With this CL, the accessory will be resized according to the last known
height of the keyboard.

This CL should (hopefully) not have any effect on how UiUtils work
around the keyboard.
It will publicize the approximation it uses to determine the keyboard
height which I then use to resize the keyboard accessory.

Bug:  856180 
Change-Id: I0726ace04db8d4d095b899aab9354bae307215e7
Reviewed-on: https://chromium-review.googlesource.com/1158573
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581563}
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetCoordinator.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetMediator.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetModel.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetViewBinder.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryMetricsRecorder.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingMediator.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetViewTest.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/chrome/android/junit/src/org/chromium/chrome/browser/autofill/keyboard_accessory/AccessorySheetControllerTest.java
[modify] https://crrev.com/21ee86ff76372f64585a4b6ead244883025dd342/ui/android/java/src/org/chromium/ui/UiUtils.java
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 23 

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

commit 04b3700b290ad2e658c9201135cb6eb8546a6352
Author: Ioana Pandele <ioanap@chromium.org>
Date: Thu Aug 23 13:53:29 2018

Update keyboard accessory style to match specs

This CL changes:
- colors
- spacing
- tap animation on suggestions in the accessory sheet
- keyboard accessory dividers

Bug:  856180 
Change-Id: Ieb0809f5c175a481bb5d9d0b0e0e8187c300f542
Reviewed-on: https://chromium-review.googlesource.com/1163612
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Friedrich Horschig [CEST] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585472}
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/layout/keyboard_accessory.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/layout/keyboard_accessory_action.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/layout/password_accessory_sheet_label.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/layout/password_accessory_sheet_option.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/layout/password_accessory_sheet_suggestion.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/values-v17/styles.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryView.java
[modify] https://crrev.com/04b3700b290ad2e658c9201135cb6eb8546a6352/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java

Comment 10 by ioanap@chromium.org, Aug 27 

Issues still to be solved:
  - Drop shadow
  - Tab width and spacing

Comment 11 by fhorschig@chromium.org, Sep 17 

Issue 865352 has been merged into this issue.

Comment 12 by maxwalker@chromium.org, Sep 17 

Additional findings:
- Align text vertically (Manage passwords...)
- Favicons are blurry (use same in History)
- Add divider below accessory
- Add 8dp below accessory divider
- Add 8dp above and below section divider

Thanks!
Bugs.png
652 KB View Download

Comment 13 by ioanap@chromium.org, Sep 20

Blockedon: 887258
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 20 

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

commit 8e6b81dba717bd2c4b9947754609570b5c21ac80
Author: Ioana Pandele <ioanap@chromium.org>
Date: Thu Sep 20 09:02:52 2018

Update keyboard accessory style to match specs (part 2)

Changes in this CL:
- added a drop shadow
- added a divider at the top of the accessory sheet
- centered text for accessory options (e.g. Manage passwords...)

Bug:  856180 ,  887258 
Change-Id: I7b71670ef551d8d5f86501ba47011ae590323676
Reviewed-on: https://chromium-review.googlesource.com/1225803
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Friedrich Horschig [CEST] <fhorschig@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592727}
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/layout/keyboard_accessory.xml
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/layout/main.xml
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/layout/password_accessory_sheet_label.xml
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/layout/password_accessory_sheet_option.xml
[add] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/layout/password_accessory_sheet_top_divider.xml
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryData.java
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessoryBridge.java
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/browser/password_manager/password_accessory_controller.cc
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/browser/password_manager/password_accessory_controller_unittest.cc
[modify] https://crrev.com/8e6b81dba717bd2c4b9947754609570b5c21ac80/chrome/browser/password_manager/password_accessory_view_interface.h
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 11 

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

commit 9ce367473d16d509bb8de611cf035611aa873a86
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Thu Oct 11 09:51:00 2018

[Android] Use correctly-scaled icons for keyboard accessory

With this CL, the keyboard accessory controller fetches icons using
GetRawFaviconForPageURL with the desired size in pixels which results
in sharp favicons that scale with Android UI elements.

Before this CL, |GetFaviconImageForPageUrl| was used which assumes the
default size (16dp, as opposed to 20dp) for icons and doesn't provide
scale representation which are suitable for hdpi scales of mobile icons.
(The provided scale was 1x to 2x, but modern phones use scales >= 3.5x)

Bug:  870346 ,  856180 
Change-Id: I4371c43ccdf6e7c7b39be805d82a17c8bdb6c661
Reviewed-on: https://chromium-review.googlesource.com/c/1273497
Commit-Queue: Friedrich Horschig [CEST] <fhorschig@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598710}
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryData.java
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessoryBridge.java
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessorySheetViewBinder.java
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/browser/android/password_manager/password_accessory_view_android.cc
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/browser/android/password_manager/password_accessory_view_android.h
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/browser/password_manager/password_accessory_controller.cc
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/browser/password_manager/password_accessory_controller.h
[modify] https://crrev.com/9ce367473d16d509bb8de611cf035611aa873a86/chrome/browser/password_manager/password_accessory_controller_unittest.cc

Comment 16 by fhorschig@chromium.org, Oct 30

Blockedon: 900178
Project Member

Comment 17 by bugdroid1@chromium.org, Oct 30 

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

commit 9855219e2fa70f37292b258c13e22861d0f85068
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Tue Oct 30 17:44:43 2018

[Android] Add margin to accessory for divider and multi-line labels

The padding in divider elements hasn't had any effect which is fixed by
using the margin instead which shows the desired spacing.
Additionally, the line height wouldn't affect the height of the title
label and negate any existing padding which is fixed by using the
correct lineSpacingMultiplier.

Bug:  856180 ,  900178 
Change-Id: Ie4e9e1df1081da9fa5dc5e6aeb1bcb578f22d7d0
Reviewed-on: https://chromium-review.googlesource.com/c/1307399
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Friedrich Horschig [EDT] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603951}
[modify] https://crrev.com/9855219e2fa70f37292b258c13e22861d0f85068/chrome/android/java/res/layout/password_accessory_sheet_divider.xml
[modify] https://crrev.com/9855219e2fa70f37292b258c13e22861d0f85068/chrome/android/java/res/layout/password_accessory_sheet_label.xml
[modify] https://crrev.com/9855219e2fa70f37292b258c13e22861d0f85068/chrome/android/java/res/layout/password_accessory_sheet_top_divider.xml

Comment 18 by fhorschig@chromium.org, Oct 30

Status: Fixed (was: Started)
Most UI changes are incorporated now. If one is missing, please add as a new bug since this bug becomes quite crowded.
Some newer changes will need to happen for the UI rework.
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 5

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

commit 468367a0a902941bc45a25eeb17fd1671079c235
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Mon Nov 05 11:36:17 2018

[Android] Add margin to accessory for divider and multi-line labels

The padding in divider elements hasn't had any effect which is fixed by
using the margin instead which shows the desired spacing.
Additionally, the line height wouldn't affect the height of the title
label and negate any existing padding which is fixed by using the
correct lineSpacingMultiplier.

Bug:  856180 ,  900178 
Change-Id: Ie4e9e1df1081da9fa5dc5e6aeb1bcb578f22d7d0
Reviewed-on: https://chromium-review.googlesource.com/c/1307399
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Friedrich Horschig [EDT] <fhorschig@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#603951}(cherry picked from commit 9855219e2fa70f37292b258c13e22861d0f85068)
Reviewed-on: https://chromium-review.googlesource.com/c/1317626
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#493}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/468367a0a902941bc45a25eeb17fd1671079c235/chrome/android/java/res/layout/password_accessory_sheet_divider.xml
[modify] https://crrev.com/468367a0a902941bc45a25eeb17fd1671079c235/chrome/android/java/res/layout/password_accessory_sheet_label.xml
[modify] https://crrev.com/468367a0a902941bc45a25eeb17fd1671079c235/chrome/android/java/res/layout/password_accessory_sheet_top_divider.xml
Project Member

Comment 20 by cr-audit...@appspot.gserviceaccount.com, Nov 5

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

Commit: 468367a0a902941bc45a25eeb17fd1671079c235
Author: fhorschig@chromium.org
Commiter: fhorschig@chromium.org
Date: 2018-11-05 11:36:17 +0000 UTC

[Android] Add margin to accessory for divider and multi-line labels

The padding in divider elements hasn't had any effect which is fixed by
using the margin instead which shows the desired spacing.
Additionally, the line height wouldn't affect the height of the title
label and negate any existing padding which is fixed by using the
correct lineSpacingMultiplier.

Bug:  856180 ,  900178 
Change-Id: Ie4e9e1df1081da9fa5dc5e6aeb1bcb578f22d7d0
Reviewed-on: https://chromium-review.googlesource.com/c/1307399
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Friedrich Horschig [EDT] <fhorschig@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#603951}(cherry picked from commit 9855219e2fa70f37292b258c13e22861d0f85068)
Reviewed-on: https://chromium-review.googlesource.com/c/1317626
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#493}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
►

Sign in to add a comment