New issue
Advanced search Search tips

Issue 831603 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Feature

Blocking:
issue 908486



Sign in to add a comment

Post-launch cleanup for "Autofill Dropdown Views Refactor"

Project Member Reported by ftirelo@chromium.org, Apr 11 2018

Issue description

Feature description:
Post-launch cleanup for "Autofill Dropdown Views Refactor" (crbug.com/824470)

Eng owner: tmartino

Once this is launched, we can delete the AutofillExpandedPopupViews
feature and delete all the code that is no longer executed.
 
Labels: OS-Linux OS-Mac
One task is to do a later pass and replace some of the values we use for colors with those shared with other UI. For instance, we may wish to use values from the omnibox for selected rows, while the footer may wish to borrow from the Bubble Frame View's footnote: https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_frame_view.cc?rcl=eac951a28b63f21617c8f8e35505be8f77143f93&l=48

This depends on other parts of Chrome having more clarity on how the refresh UI will interact with native themes.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 6

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

commit 19eaae68c82b00edc2a2bafbafe94961f2e9852f
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Thu Sep 06 14:07:41 2018

[AF] Enable dropdown refresh by default

Difference in tests are due to separators between suggestions
and footer are not sent in the suggestions list (we already did
that on Android).

A follow-up CL will delete the feature and all the code that
depends on it.

Bug: 831603
Change-Id: If2415782182ea7c0deb6d848ed7ae4f4964216f1
Reviewed-on: https://chromium-review.googlesource.com/1204835
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589155}
[modify] https://crrev.com/19eaae68c82b00edc2a2bafbafe94961f2e9852f/components/autofill/core/browser/autofill_external_delegate_unittest.cc
[modify] https://crrev.com/19eaae68c82b00edc2a2bafbafe94961f2e9852f/components/autofill/core/common/autofill_features.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 6

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

commit 8d0997778c8cbfb19908fec9c8fc3a7aa9805c42
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Thu Sep 06 21:26:25 2018

[AF] Remove native popup views test param from interactive ui tests

This is to prepare the field for removing feature
kAutofillExpandedPopupViews.

Bug: 831603
Change-Id: I3f0d8ee5359be4c89b5284aef6bbdca0edf86da1
Reviewed-on: https://chromium-review.googlesource.com/1210545
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589305}
[modify] https://crrev.com/8d0997778c8cbfb19908fec9c8fc3a7aa9805c42/chrome/browser/autofill/autofill_interactive_uitest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 6

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

commit 7bf6f28e80d89c2510f65efb3494fb3f46e36fb8
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Thu Sep 06 21:33:53 2018

[AF] Remove test param from DevTools sanity browser tests

This is to prepare the field for removing feature
kAutofillExpandedPopupViews.

Bug: 831603
Change-Id: I7db276de2da430634e146ad7641669e139fc6fa1
Reviewed-on: https://chromium-review.googlesource.com/1210387
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589308}
[modify] https://crrev.com/7bf6f28e80d89c2510f65efb3494fb3f46e36fb8/chrome/browser/devtools/devtools_sanity_browsertest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 7

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

commit bfe4b735a728de7e553641217a63f1f1283a165b
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Fri Sep 07 16:02:42 2018

[AF] Remove native popup views test params in PWM tests

This is to prepare the field for removing feature
kAutofillExpandedPopupViews.

Bug: 831603
Change-Id: Ib748de088912221c3af18435286904046befb58f
Reviewed-on: https://chromium-review.googlesource.com/1210165
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589531}
[modify] https://crrev.com/bfe4b735a728de7e553641217a63f1f1283a165b/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/bfe4b735a728de7e553641217a63f1f1283a165b/chrome/browser/password_manager/password_manager_interactive_uitest.cc

Any update on c#1? I'm working on dark mode (Issue 850098) and am happy to do this if you've reached a conclusion. Otherwise, I'll parametrize the current hardcoded colors.
Hmm, I would hope that we can do this in a way that dark mode happens "for free."

Do you have mocks or UX documentation for how the popup should work in Dark Mode that you could pass along? I'd like to take a look before suggesting an approach.
The UX spec is https://docs.google.com/presentation/d/1kJoBzf_HGYK-_FAJPAjD67TW8224dCPLA6dAhlDmysk/edit#slide=id.g471d243f08_0_78 though it doesn't cover menus.

If you want an idea with temp colors, launch Canary with "--force-dark-mode" on Mojave and open the wrench menu.

Re: "for free", this is what we would get if autofill popup used NativeTheme. Right now, this is one of the only surfaces that's not covered since it uses hardcoded colors.
I will try to mail a CL updating these to native theme colors by EOW. I'll cc you.
Blocking: 908486
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 10

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

commit 05eef22b41776e01ee7f3fa4f121a92fe7c32546
Author: Tommy Martino <tmartino@chromium.org>
Date: Mon Dec 10 23:46:07 2018

[Autofill Cleanup] Use NativeTheme Colors

This CL addreses one of the post-launch cleanup items for the Autofill
Views rewrite. Specifically, it replaces hardcoded colors with colors
taken from the NativeTheme.

The new colors are all defined in common_theme and, with one exception,
are equal to the hardcoded ones they are replacing. The exception is
kColorId_MenuSeparatorColor, which this CL updates from a neutral grey
to the corresponding GoogleGrey value. This is consistent with the spec
for Autofill, and I believewe should be using it Chrome-wide as well.

This will also allow work to proceed on macOS Dark Mode implementation.

Bug: 831603
Change-Id: I6755285cff9096daef3e468a407214dc5dd770bc
Reviewed-on: https://chromium-review.googlesource.com/c/1367936
Commit-Queue: Tommy Martino <tmartino@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615326}
[modify] https://crrev.com/05eef22b41776e01ee7f3fa4f121a92fe7c32546/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
[modify] https://crrev.com/05eef22b41776e01ee7f3fa4f121a92fe7c32546/chrome/browser/ui/views/autofill/autofill_popup_base_view.h
[modify] https://crrev.com/05eef22b41776e01ee7f3fa4f121a92fe7c32546/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc
[modify] https://crrev.com/05eef22b41776e01ee7f3fa4f121a92fe7c32546/chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc
[modify] https://crrev.com/05eef22b41776e01ee7f3fa4f121a92fe7c32546/ui/native_theme/common_theme.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 9

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

commit f2d26633cbd50735ac2af30436888b71ac0abad3
Author: Mathieu Perreault <mathp@chromium.org>
Date: Wed Jan 09 23:11:26 2019

[Autofill] Remove AutofillPopupViewViews and associated feature.

Bug: 906135,831603
Change-Id: I3c982f8b3ffb4928c7c878e74e10113999106499
Reviewed-on: https://chromium-review.googlesource.com/c/1387124
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621360}
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/about_flags.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/views/autofill/autofill_popup_base_view.h
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.h
[delete] https://crrev.com/1b9e54bfbe066e986efe150e1596112dbaab6652/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
[delete] https://crrev.com/1b9e54bfbe066e986efe150e1596112dbaab6652/chrome/browser/ui/views/autofill/autofill_popup_view_views.h
[delete] https://crrev.com/1b9e54bfbe066e986efe150e1596112dbaab6652/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/chrome/test/BUILD.gn
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/components/autofill/core/browser/autofill_external_delegate.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/components/autofill/core/browser/autofill_external_delegate_unittest.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/components/autofill/core/common/autofill_features.cc
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/components/autofill/core/common/autofill_features.h
[modify] https://crrev.com/f2d26633cbd50735ac2af30436888b71ac0abad3/testing/variations/fieldtrial_testing_config.json

Sign in to add a comment