Password Manager lags excessively when list contains entries whose hostnames require shortening
Reported by
taylorve...@gmail.com,
Nov 20 2016
|
||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36 Steps to reproduce the problem: 1. Visit chrome://settings/passwords, and have saved password entries whose hostnames exceed width of first hostname column 2. Search, delete, select an entry What is the expected behavior? Constant time String replacement, no lag What went wrong? Lag. password_manager.js brute forces the length into compliance - see https://cs.chromium.org/chromium/src/chrome/browser/resources/options/password_manager.js?q=updateOriginsEliding_&sq=package:chromium&l=215&dr=C Did this work before? N/A Chrome version: 54.0.2840.99 Channel: stable OS Version: 6.3 Flash Version: Shockwave Flash 23.0 r0 My list of ~680 passwords takes between 10 and 15 seconds to load completely after first initialization, searches, deletions - even selections seem to have a 1-2 second lag accompanying them. Column width does not seem to change depending on window size, nor can the user modify it - why not set a hard limit to the hostname length (that is string length, not rendered width) and make a single call to substring? If user fonts and monospacing vary, maybe instead of relying on ctx.measureText(textContent), change the container to something like a textbox that doesn't wrap?
,
Nov 21 2016
That seems to relate to passwords being auto-filled at login pages, since OP mentions specific websites (e.g. amazon.com). To clarify, this bug relates solely to the chrome://settings/passwords dialog
,
Dec 9 2016
This is related to bug 651049 , which should be fixed in version 55. However, the issue with long hostnames was not studied there. I'm reopening this for investigation.
,
Dec 9 2016
Thanks again for the report and the spot-on code pointer! Will try to fix soon.
,
Dec 9 2016
Just to document the reproduction case: using chrome://flags/#password-import-export I imported the attached passwords. ToT chrome is unusable displaying chrome://settings/passwords after that, the reason being exactly what is described by the reporter.
,
Dec 9 2016
The fix is in https://codereview.chromium.org/2567693002/
,
Dec 10 2016
Quick update: The reviewer pointed out that the JS workaround for the ellipsis is no longer needed. Simply using "direction: rtl" CSS for the URL text results in the ellipsis applied from left. There is a minor tweak needed to fix the alignment of the result in otherwise LTR UI, but it is all basically what has been removed in https://codereview.chromium.org/1817053002/. I will soon update the patch from #6. Notes on testing: Installing and running ChromeVox on self-built Chromium on Linux: * Need to install both the ChromeVox extension [1] and the Lois voice extension * Need to build nacl support (enable_nacl = true in gn args) * Need to locally disable the CHECK in NaClSandbox::CheckForExpectedNumberOfOpenFds Testing Chrome UI in RTL: * Change LANGUAGE env variable appropriately, e.g., to run in Hebrew: LANGUAGE=he ./chrome ... [1] https://chrome.google.com/webstore/detail/chromevox/kgejglhpjiefppelpmljglcjbhoiplfn [2] https://chrome.google.com/webstore/detail/lois-tts-us-english/jcabofbhfighebggomnamjankeaplmhn
,
Dec 12 2016
Also just realising that what I'm trying to do in the CL of #6 has already been done for the MD version of the settings (chrome://md-settings/passwords) in https://codereview.chromium.org/2442333003. I spoke to the author of that CL (jdoerrie@) as well as to the author of recent changes to eliding in the old settings (kolos@), and given that screen reader seems to work fine with the new solution, none of us sees a concern with going CSS only.
,
Dec 12 2016
Thank you for looking into this, vabr@! I addressed part of the issue in http://crrev.com/2439453005 which went into stable with M55. However, I still see a bit of lag with the test case you provided. Having the pure CSS implementation here as well looks like a perfect solution to me.
,
Dec 13 2016
Screenshots corresponding to https://codereview.chromium.org/2567693002/#ps40001
,
Dec 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f5230e3d8213f33ba0f1014b1e682f75e36028f commit 7f5230e3d8213f33ba0f1014b1e682f75e36028f Author: vabr <vabr@chromium.org> Date: Wed Dec 14 09:10:50 2016 Elide origins for passwords settings in CSS instead of JS Displayed origins of stored passwords are elided to fit the cell they are in. The ellipsis needs to be applied from the left, because the most important security information about the hostname are the top (right-most) parts of the domain [1]. In the past this needed to be done by password_manager.js, because changing the text direction in CSS to allow eliding from left left to issues with screen readers. This CL drops the JS code for computing the elided string, and instead specifies the text direction for the hostname to be RTL. This applies the ellipsis from left, while working as expected with the screen reader, and not interfering with the text direction of the Chrome UI. [1] https://www.chromium.org/Home/chromium-security/enamel#TOC-Eliding-Origin-Names-And-Hostnames BUG= 667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567693002 Cr-Commit-Position: refs/heads/master@{#438469} [modify] https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f/chrome/browser/resources/options/password_manager.js [modify] https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f/chrome/browser/resources/options/password_manager_list.css [modify] https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f/chrome/browser/resources/options/password_manager_list.js
,
Dec 14 2016
r438469 should be fixing this issue on trunk. I will wait until it reaches Canary and then request a merge to 56.
,
Dec 15 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a7c201888f97779886fc374eeffaafcc385e43c commit 2a7c201888f97779886fc374eeffaafcc385e43c Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Dec 16 10:39:25 2016 Elide origins for passwords settings in CSS instead of JS Displayed origins of stored passwords are elided to fit the cell they are in. The ellipsis needs to be applied from the left, because the most important security information about the hostname are the top (right-most) parts of the domain [1]. In the past this needed to be done by password_manager.js, because changing the text direction in CSS to allow eliding from left left to issues with screen readers. This CL drops the JS code for computing the elided string, and instead specifies the text direction for the hostname to be RTL. This applies the ellipsis from left, while working as expected with the screen reader, and not interfering with the text direction of the Chrome UI. [1] https://www.chromium.org/Home/chromium-security/enamel#TOC-Eliding-Origin-Names-And-Hostnames BUG= 667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567693002 Cr-Commit-Position: refs/heads/master@{#438469} (cherry picked from commit 7f5230e3d8213f33ba0f1014b1e682f75e36028f) Review-Url: https://codereview.chromium.org/2579283002 . Cr-Commit-Position: refs/branch-heads/2924@{#525} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/2a7c201888f97779886fc374eeffaafcc385e43c/chrome/browser/resources/options/password_manager.js [modify] https://crrev.com/2a7c201888f97779886fc374eeffaafcc385e43c/chrome/browser/resources/options/password_manager_list.css [modify] https://crrev.com/2a7c201888f97779886fc374eeffaafcc385e43c/chrome/browser/resources/options/password_manager_list.js
,
Dec 16 2016
Keeping an eye on the betabuilders, but otherwise this should be fixed. The change is already in Canary. It has just been merged to 56 as well, although it might take time until 56 is re-spun; in particular it might no longer happen this year.
,
Dec 23 2016
Just an update to say that this issue should be fixed for any Chrome with version at least 56.0.2924.31.
,
Jan 4 2017
Tested on windows 10 using chrome M56 #56.0.2924.51 and followed steps mentioned in comment #5. 1. Enabled flag "password import and export" from chrome://flags . 2. navigated to chrome://settings/passwords and imported the attached file in comment #6. 3. Observed that all the passwords and accounts are seen in saved passwords. Attached screenshot for reference. @vabr -- Could you please check and let us know is this the expected behavior and issue is fixed. Thanks!
,
Jan 12 2017
The issue was about the time it took to load the passwords page. It is not possible to tell from a static screenshot, how long it took to load it. Hence I cannot say if #19 observed the expected behaviour. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by ligim...@chromium.org
, Nov 21 2016Status: Duplicate (was: Unconfirmed)