New issue
Advanced search Search tips

Issue 800318 link

Starred by 1 user
Status: Fixed
Owner:
kolos@chromium.org
Closed: Aug 27
Cc:
vasi...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocking:
issue 797475



Sign in to add a comment

[Password Manager] Merge save and update bubbles, and implement username/password editing

Project Member Reported by kolos@chromium.org, Jan 9 2018 
1. Remove |kEnableUsernameCorrection| and |kEnablePasswordSelection| flags as the corresponding features are stabilized. 
2. Introduce |is_update_bubble| flag to |ManagePasswordsBubbleView::PendingView|. Merge the code of |PendingView| and |UpdatePendingView| into |PendingView|. I.e. simply use |if (is_update_bubble) { /* code for update case*/} else { /* code for save case */}|.
3. Make username editable for update cases when there is only one credential (i.e. |CredentialsSelectionView| is not needed).
4. Introduce the password selector and the eye icon for update cases. 
5. Update the backend logic, if needed.
6. Implement an editable combobox for the password selector. 
7. Introduce an editable username selector that is available for both save and update cases. 
8. Decide whether we should change title and buttons if username editing switches the state from save to update or back?
9. Move the eye icon inside the password selector (aka ZigZag problem).
10. Add metrics.
11. Add tests for views

Comment 1 by kolos@chromium.org, Jan 9 2018

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 10 2018 

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

commit 8fbaaa49a274c3dd9bf54b4b299414c4a64950e8
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Wed Jan 10 14:35:08 2018

[Password Manager] Remove flags for username correction and password selection in a prompt

Bug:  800318 
Change-Id: I20ec8e9db99c4b2611a55c0664b3933b0a0140c0
Reviewed-on: https://chromium-review.googlesource.com/856998
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528308}
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/about_flags.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_unittest.mm
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller_unittest.mm
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/components/autofill/content/renderer/password_form_conversion_utils.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/components/password_manager/core/common/password_manager_features.cc
[modify] https://crrev.com/8fbaaa49a274c3dd9bf54b4b299414c4a64950e8/components/password_manager/core/common/password_manager_features.h
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 26 2018 

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

commit 578da4353a91968bfdfa6f1d01c7f1a42d453268
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Mon Feb 26 14:13:22 2018

[Password Manager] Fix multi-account update case

In |PasswordUpdatePendingView::Accept|, |selection_view_| is checked, but |selection_view_| is never set.

There is no tests, because we have no tests for views. This issue shows that we need tests for views.

Bug:  800318 
Change-Id: I2b8faaf943be5ecd2792bd8e7d2092e30dec7b57
Reviewed-on: https://chromium-review.googlesource.com/931762
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539124}
[modify] https://crrev.com/578da4353a91968bfdfa6f1d01c7f1a42d453268/chrome/browser/ui/views/passwords/password_update_pending_view.cc

Comment 4 by kolos@chromium.org, Feb 27 2018

update_prompt.png
14.5 KB View Download
update_prompt_many_passwords.png
13.1 KB View Download
update_prompt_many_passwords2.png
13.4 KB View Download
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 2 2018 

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

commit c7fd77da7749bb1b48baf583889acdd4467811d1
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Fri Mar 02 14:57:02 2018

[Password Manager] Merge code of |PasswordUpdatePendingView| and |PasswordPendingView| into |PasswordPendingView|

This CL is Step 2 from the description of  crbug.com/800318 .

Bug:  800318 
Change-Id: I8b71209a8ab820c54a5a122ce0b4b907abd036bb
Reviewed-on: https://chromium-review.googlesource.com/926601
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540516}
[modify] https://crrev.com/c7fd77da7749bb1b48baf583889acdd4467811d1/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/c7fd77da7749bb1b48baf583889acdd4467811d1/chrome/browser/ui/views/passwords/password_bubble_view_base.cc
[modify] https://crrev.com/c7fd77da7749bb1b48baf583889acdd4467811d1/chrome/browser/ui/views/passwords/password_pending_view.cc
[modify] https://crrev.com/c7fd77da7749bb1b48baf583889acdd4467811d1/chrome/browser/ui/views/passwords/password_pending_view.h
[delete] https://crrev.com/f21542cdf145cbfc53681b2a63838d6bb6d2b839/chrome/browser/ui/views/passwords/password_update_pending_view.cc
[delete] https://crrev.com/f21542cdf145cbfc53681b2a63838d6bb6d2b839/chrome/browser/ui/views/passwords/password_update_pending_view.h
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 2 2018 

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

commit 6362c7aeab66147cffb476d6e3bd0b1b6c65a890
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Fri Mar 02 15:04:05 2018

[Password Manager] Introduce editable username, password selector and eye icon for update cases

This CL is Step 3 & Step 4 from the description of  crbug.com/800318 .

Bug:  800318 
Change-Id: I50d8936f80d193044c159a7d0f12285aaecfdeb6
Reviewed-on: https://chromium-review.googlesource.com/927361
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540518}
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/passwords/manage_passwords_bubble_model.h
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[delete] https://crrev.com/ebfec448c139cc96b78a86464ad020465dc19401/chrome/browser/ui/views/passwords/credentials_selection_view.cc
[delete] https://crrev.com/ebfec448c139cc96b78a86464ad020465dc19401/chrome/browser/ui/views/passwords/credentials_selection_view.h
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/views/passwords/password_items_view.cc
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/views/passwords/password_items_view.h
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/views/passwords/password_pending_view.cc
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/chrome/browser/ui/views/passwords/password_pending_view.h
[modify] https://crrev.com/6362c7aeab66147cffb476d6e3bd0b1b6c65a890/components/password_manager/core/browser/password_form_manager.cc

Comment 7 by battre@chromium.org, Mar 2 2018 

We have the following two UKMs:
- User.Action.CorrectedUsernameInForm
- User.Action.SelectedDifferentPasswordInBubble
Can you make sure that we always record something meaningful in the light of the recent and upcoming changes?

Comment 8 by kolos@chromium.org, Mar 2 2018 

I've added metrics to our plan about the bubble. But let's do it when the bubble has reached the ultimate state (i.e. editable comboboxes for username and password)

Comment 9 by kolos@chromium.org, Mar 2 2018

Description: Show this description
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 29 2018 

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

commit b8bc1145d84b2f9c20d13d2ea49ca68438dd2573
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Mar 29 15:00:11 2018

The password bubble dynamically switches between Save and Update state.

The change is implemented on Views only. The title stays as well as the "Nope"/"Never" button.
The save button changes the title dynamically depending on the current username.

Bug:  800318 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: If17071782dc0736a39c61e843e1c456c6e4427ee
Reviewed-on: https://chromium-review.googlesource.com/983962
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546832}
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/password_manager/save_password_infobar_delegate_android.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/password_manager/update_password_infobar_delegate_android.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.mm
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/cocoa/passwords/passwords_bubble_cocoa_unittest.mm
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/manage_passwords_bubble_model.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/manage_passwords_bubble_model.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/manage_passwords_ui_controller.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/passwords_model_delegate.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/passwords/passwords_model_delegate_mock.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/views/passwords/password_pending_view.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/chrome/browser/ui/views/passwords/password_pending_view.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/components/password_manager/core/browser/password_manager_metrics_util.cc
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/components/password_manager/core/browser/password_manager_metrics_util.h
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.mm
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.mm
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.mm
[modify] https://crrev.com/b8bc1145d84b2f9c20d13d2ea49ca68438dd2573/tools/metrics/histograms/histograms.xml

Comment 11 by kolos@chromium.org, May 14 2018

Blocking: 797475

Comment 12 by kolos@chromium.org, Aug 27

Status: Fixed (was: Started)
►

Sign in to add a comment