New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Feature

Blocked on:
issue 916199



Sign in to add a comment
link

Issue 890118: Show website favicon in password manager

Reported by rsgingerrs@chromium.org, Sep 28 Project Member

Issue description

Now we only show the website URL in chrome://settings/passwords, but not the favicon. We would like to add the favicon before the address.
 
Proposed_mock.png
16.7 KB View Download

Comment 1 by battre@chromium.org, Sep 28

Cc: jdoerrie@chromium.org

Comment 2 by vasi...@chromium.org, Sep 28

Cc: maxwalker@chromium.org
Can you make a screeshot of the whole page (= whole content area)? It's difficult to assess only one column.

Comment 3 by rsgingerrs@chromium.org, Oct 4

Sure.
Screenshot from 2018-10-04 11-23-11.png
31.9 KB View Download

Comment 4 by bugdroid1@chromium.org, Oct 23

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4cb312703d70ef1fd7f4379d971ff848b5977b8a

commit 4cb312703d70ef1fd7f4379d971ff848b5977b8a
Author: Yue Cen <rsgingerrs@chromium.org>
Date: Tue Oct 23 21:18:59 2018

[Settings] Add a component site-favicon to display site favicon

This is a refactor CL and it should not change any existing UI.

The affected pages are as follows:
- On startup site entry in chrome://settings
- chrome://settings/siteData
- chrome://settings/searchEngines
- chrome://settings/content/zoomLevels
- chrome://settings/handlers
- chrome://settings/content/usbDevices
- chrome://settings/content/all
- All the pages in "Content settings" that use site-list-entry, such as
  chrome://settings/content/paymentHandler

Bug: 890118
Change-Id: I35e00406999cf30012db557b10dd418cef67a69d
Reviewed-on: https://chromium-review.googlesource.com/c/1277995
Commit-Queue: Yue Cen <rsgingerrs@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602099}
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/on_startup_page/startup_url_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/on_startup_page/startup_url_entry.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/search_engines_page/omnibox_extension_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/search_engines_page/omnibox_extension_entry.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/search_engines_page/search_engine_entry_css.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/settings_shared_css.html
[add] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_favicon.html
[add] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_favicon.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/protocol_handlers.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_data.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_data_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_data_entry.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_entry.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_list_entry.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_settings_behavior.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/site_settings_behavior.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/usb_devices.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/browser/resources/settings/site_settings/zoom_levels.html
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/test/data/webui/settings/cr_settings_browsertest.js
[add] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/chrome/test/data/webui/settings/site_favicon_test.js
[modify] https://crrev.com/4cb312703d70ef1fd7f4379d971ff848b5977b8a/testing/buildbot/filters/webui_polymer2_browser_tests.filter

Comment 6 Deleted

Comment 7 by rsgingerrs@chromium.org, Nov 14

Per UX's request, we are making the following changes:
- Increase the padding between the icon and the domain like in chrome://history.
- Make the URLS for the domain black (but still clickable).

Please see the expected UI as follows.
Screenshot from 2018-11-14 13-38-33.png
37.5 KB View Download

Comment 8 by vasi...@chromium.org, Nov 15

Max, is this exactly what you wanted?

Comment 9 by dpa...@chromium.org, Nov 15

Cc: namratakannan@chromium.org
Repeating my comment from https://chromium-review.googlesource.com/c/chromium/src/+/1332258/1#message-bf5cf488e0f27b88cb92bcb5d40ad885eb39a973.


Changing the font color makes this UI inconsistent with other places in Settings (see screenshot). We put a lot of effort on keeping those consistent see  issue 849857  during the recent MD Refresh.

Is there a good reason to only change the color here? Can we revert this to keep consistency with the original MD refresh specs?

@namratakannan: Any thoughts?
other_lists.png
32.2 KB View Download

Comment 10 by maxwalker@chromium.org, Nov 20

Adding a mini spec. Please use Google Grey 900 (#202124, primary text color) for the URL text.
V1 - Favicons Spec.png
51.4 KB View Download

Comment 11 by rsgingerrs@chromium.org, Nov 30

It seems by removing the ellipsis in masked state, the long password will show one more character than the shorter ones. I assume that's not the expected behavior?
MG6HZb8RpjE.png
37.9 KB View Download

Comment 12 by maxwalker@chromium.org, Dec 3

Hi! Passwords should show a bullet for each character until the max number of bullets (which I think is 16) is reached:

- Password with length x < max: show x characters
- Password with length x = max: show max characters
- Password with length x > max: show max characters

Examples:
- •••• (x = 4)
- •••••• (x = 6)
- •••••••••••••• (x = max)
- •••••••••••••• (x > max)

Comment 13 by rsgingerrs@chromium.org, Dec 5

Thanks for the reply.

There is a tricky issue that if we remove the ellipsis, the long passwords show one more character than the short one.

Since showing a ellipsis on a long password in masked state is the original behavior and it doesn't block the favicon feature, I would suggest we land the UI changes and create a separate bug for the ellipsis issue.

Comment 14 by vasi...@chromium.org, Dec 5

I guess what Max means is to edit getPassword_() function and instead repeat(this.item.entry.numCharactersInPassword) have something like repeat(min(this.item.entry.numCharactersInPassword, 16)). That magic const doesn't exist today. Therefore I agree that it's a separate feature, trivial though.

Comment 15 by maxwalker@chromium.org, Dec 5

Thanks! Creating a separate bug SGTM2.

Comment 16 by rsgingerrs@chromium.org, Dec 5

Bug created: crbug.com/912321

Comment 17 by rsgingerrs@chromium.org, Dec 6

Labels: Merge-Request-72 OS-Chrome OS-Linux OS-Mac OS-Windows
For some reason the last commit does not show up here.

We would like to request a merge to M72 for this commit:
https://chromium.googlesource.com/chromium/src/+/8b974f8e7a514acf61ed018d55565731ff5d02e5

Comment 18 by sheriffbot@chromium.org, Dec 6

Project Member
Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: There is .grd file changes and we are only 53 days from stable.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

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

Comment 19 by bugdroid1@chromium.org, Dec 7

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

commit 8b974f8e7a514acf61ed018d55565731ff5d02e5
Author: Yue Cen <rsgingerrs@chromium.org>
Date: Thu Dec 06 21:20:15 2018

Password manager: Style changes in the website column

- Increase the padding between the icon and the domain like in
  chrome://history.
- Make the URLS for the domain black (but still clickable).

Bug: 890118
Change-Id: If63006dced601b2891e9b0864b788808acc1faa8
Reviewed-on: https://chromium-review.googlesource.com/c/1332258
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Yue Cen <rsgingerrs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614491}
[modify] https://crrev.com/8b974f8e7a514acf61ed018d55565731ff5d02e5/chrome/browser/resources/settings/autofill_page/passwords_shared_css.html

Comment 20 by rsgingerrs@chromium.org, Dec 7

Oops, now it shows up.

Request to merge comment#19 in M72.

Comment 21 by srinivassista@google.com, Dec 7

Labels: -Merge-Review-72 Merge-Approved-72
Approving the CL in Comment 19 for 72. Branch 3626

Comment 22 by bugdroid1@chromium.org, Dec 7

Project Member
Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/632693796875df389ea693c8dbfeeab44b16cc2d

commit 632693796875df389ea693c8dbfeeab44b16cc2d
Author: Yue Cen <rsgingerrs@chromium.org>
Date: Fri Dec 07 19:48:19 2018

[Merge to M72] Password manager: Style changes in the website column

- Increase the padding between the icon and the domain like in
  chrome://history.
- Make the URLS for the domain black (but still clickable).

TBR=rsgingerrs@chromium.org

(cherry picked from commit 8b974f8e7a514acf61ed018d55565731ff5d02e5)

Bug: 890118
Change-Id: If63006dced601b2891e9b0864b788808acc1faa8
Reviewed-on: https://chromium-review.googlesource.com/c/1332258
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Yue Cen <rsgingerrs@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614491}
Reviewed-on: https://chromium-review.googlesource.com/c/1368444
Reviewed-by: Yue Cen <rsgingerrs@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#146}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/632693796875df389ea693c8dbfeeab44b16cc2d/chrome/browser/resources/settings/autofill_page/passwords_shared_css.html

Comment 23 by vasi...@chromium.org, Dec 18

Blockedon: 916199

Comment 24 by cr-audit...@appspot.gserviceaccount.com, Dec 19

Project Member
Labels: CommitLog-Audit-Violation Merge-Without-Approval M-72
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 632693796875df389ea693c8dbfeeab44b16cc2d was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required --

Comment 25 by cr-audit...@appspot.gserviceaccount.com, Dec 19

Project Member
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/632693796875df389ea693c8dbfeeab44b16cc2d

Commit: 632693796875df389ea693c8dbfeeab44b16cc2d
Author: rsgingerrs@chromium.org
Commiter: rsgingerrs@chromium.org
Date: 2018-12-07 19:48:19 +0000 UTC

[Merge to M72] Password manager: Style changes in the website column

- Increase the padding between the icon and the domain like in
  chrome://history.
- Make the URLS for the domain black (but still clickable).

TBR=rsgingerrs@chromium.org

(cherry picked from commit 8b974f8e7a514acf61ed018d55565731ff5d02e5)

Bug: 890118
Change-Id: If63006dced601b2891e9b0864b788808acc1faa8
Reviewed-on: https://chromium-review.googlesource.com/c/1332258
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Yue Cen <rsgingerrs@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614491}
Reviewed-on: https://chromium-review.googlesource.com/c/1368444
Reviewed-by: Yue Cen <rsgingerrs@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#146}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Comment 26 by abdulsyed@google.com, Dec 26

Labels: -CommitLog-Audit-Violation -Merge-Without-Approval

Comment 27 by rsgingerrs@chromium.org, Dec 27

Just noticed this. So the merge violation was a false alarm, correct?

Sign in to add a comment