Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567693002:
Elide origins for passwords settings in CSS instead of JS (Closed)
|
Created:
4 years ago by vabr (Chromium) Modified:
4 years ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, gcasto+watchlist_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionElide 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
Committed: https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f
Cr-Commit-Position: refs/heads/master@{#438469}
Patch Set 1 #Patch Set 2 : Elide through CSS #
Total comments: 4
Patch Set 3 : Tune CSS better #
Total comments: 3
Patch Set 4 : Add a note about direction #
Messages
Total messages: 34 (18 generated)
Description was changed from ========== Speed up suffix length estimate in password_manager.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 informatino about the hostname are the top (right-most) parts of the domain. So the eliding needs to be done by password_manager.js, instead of relying on the default eliding done by the rendering engine. Part of the eliding is computing how long is the maximal suffix of the origin which still fits into the cell. Currently, this is done by shaving the leading characters from the origin one by one until it fits. This CL speeds that up by making an educated guess: computing how many "i"s would fit into the cell, cutting the suffix of that length, and then cutting it further until it fits. BUG=667130 ========== to ========== Speed up suffix length estimate in password_manager.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 informatino about the hostname are the top (right-most) parts of the domain. So the eliding needs to be done by password_manager.js, instead of relying on the default eliding done by the rendering engine. Part of the eliding is computing how long is the maximal suffix of the origin which still fits into the cell. Currently, this is done by shaving the leading characters from the origin one by one until it fits. This CL speeds that up by making an educated guess: computing how many "i"s would fit into the cell, cutting the suffix of that length, and then cutting it further until it fits. BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vabr@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, Could you please review? I was considering doing a binary search to find the maximal suffix, but not sure if the additional code complexity is worth it. With my test data I had seen the estimate on the maximal fitting suffix length to be about 20 higher than the actual suffix length. Cheers, Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can you just switch to css? the screen reader bug seen before is fixed. just flip direction and use text-overflow: ellipsis
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/10 00:40:12, Dan Beam wrote: > can you just switch to css? the screen reader bug seen before is fixed. just > flip direction and use text-overflow: ellipsis Thanks, Dan, I completely missed this option! Please have a look at patch set 2. I tested with the data from https://crbug.com/667130#c5 that it looks as expected both in English and in Hebrew (as representatives of LTR and RTL languages, respectively). Please see the screenshots in https://crbug.com/667130#c8. I also checked that ChromeVox reads the origins correctly (to avoid repeating https://crbug.com/595276). (It took me a while to figure out how to get ChromeVox running, so I documented that in https://crbug.com/667130#c7). Cheers, Vaclav https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: -webkit-margin-end: 7px; These numbers seem a bit random. But I stole them from what has been in before https://codereview.chromium.org/2567693002/ and they seem to work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Speed up suffix length estimate in password_manager.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 informatino about the hostname are the top (right-most) parts of the domain. So the eliding needs to be done by password_manager.js, instead of relying on the default eliding done by the rendering engine. Part of the eliding is computing how long is the maximal suffix of the origin which still fits into the cell. Currently, this is done by shaving the leading characters from the origin one by one until it fits. This CL speeds that up by making an educated guess: computing how many "i"s would fit into the cell, cutting the suffix of that length, and then cutting it further until it fits. BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Speed up suffix length estimate in password_manager.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. 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. BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
vabr@chromium.org changed reviewers: + kolos@chromium.org
Hi Maxim, In addition to Dan's review, could you also please have a look and let me know if there are any issues with this change? Cheers, Vaclav
Thanks Vaclav for fixing that. LGTM
https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: -webkit-margin-end: 7px; The links points to this CL.
https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:78: -webkit-margin-end: 7px; On 2016/12/12 09:55:57, kolos1 wrote: > The links points to this CL. Thanks! The correct link should have been https://codereview.chromium.org/1823423002.
https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:82: text-align: left; don't we have to set direction: ltr in the RTL case as well? can you send me screenshots of both directionalities?
On 2016/12/13 04:07:33, Dan Beam wrote: > https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/options/password_manager_list.css (right): > > https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... > chrome/browser/resources/options/password_manager_list.css:82: text-align: left; > don't we have to set direction: ltr in the RTL case as well? can you send me > screenshots of both directionalities? Hi Dan, As mentioned in #11, screenshots are in http://crbug.com/667130#c8. It appears that setting ltr in the RTL case is not needed. Cheers, Vaclav
On 2016/12/13 06:17:09, vabr (Chromium) wrote: > On 2016/12/13 04:07:33, Dan Beam wrote: > > > https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/options/password_manager_list.css (right): > > > > > https://codereview.chromium.org/2567693002/diff/20001/chrome/browser/resource... > > chrome/browser/resources/options/password_manager_list.css:82: text-align: > left; > > don't we have to set direction: ltr in the RTL case as well? can you send me > > screenshots of both directionalities? > > Hi Dan, > > As mentioned in #11, screenshots are in http://crbug.com/667130#c8. It appears > that setting ltr in the RTL case is not needed. > > Cheers, > Vaclav the screenshots show that you DO need a direction: ltr; the ... should always be closer to the favicons (so it should be on the right in RTL). also, increase the magical margin constants because the screenshots don't currently look that great. possibly base them on em units
Hi Dan, Please take a look at patch set 3, with screenshots at https://crbug.com/667130#c11. Changing the direction for other classes than "url" in the previous patchset was a mistake (fixed now). > the screenshots show that you DO need a direction: ltr; > > the ... should always be closer to the favicons (so it should be on the right in > RTL). This is not correct. The favicons have nothing to do with the ellipsis. As described in the CL description, the ellipsis needs to always come from the left to keep the right-most parts of the hostname visible. Note that the hostname is always LTR even if the rest of the UI is RTL. This has been a security requirement, to avoid cases like shortening www.google.com.evil.com to www.google.c... > > also, increase the magical margin constants because the screenshots don't > currently look that great. possibly base them on em units I tried to address this. I don't have a feeling for what looks great, so I appreciate your feedback here. Cheers, Vaclav https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:91: direction: rtl; Not sure if this is reduntant, give the [dir='rtl']?
Description was changed from ========== Speed up suffix length estimate in password_manager.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. 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. BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== 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-Nam... BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
there's probably some magical hack somewhere to make only the end of the hostname/origin/whatever show up while moving the ... to the right in RTL, but I understand what you're saying now. lgtm thanks for sticking with it https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:91: direction: rtl; On 2016/12/13 09:23:38, vabr (Chromium) wrote: > Not sure if this is reduntant, give the [dir='rtl']? it is, but it's probably fine to keep it here and add a note as it's uncommon (I would probably assume it was accidentally omitted)
Thanks, Dan! > there's probably some magical hack somewhere to make only the end of the > hostname/origin/whatever show up while moving the ... to the right in RTL, but I > understand what you're saying now. lgtm thanks for sticking with it Now I see what you mean. I was too stuck at considering the ... to be part of the LTR URL, while IIUC, your point is that the ... are actually a part of the surrounding RTL text. Give your approval, and for consistency with what I currently see in the MD version of the passwords settings, I'll keep the ... on the left-hand site. But I will check at least with a teammate of mine, who is a native speaker of a RTL written language. Perhaps he will have some suggestions. If you find some interesting pointers, please share, I'll be happy to fix the UI. Thanks, Vaclav https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/password_manager_list.css (right): https://codereview.chromium.org/2567693002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/password_manager_list.css:91: direction: rtl; On 2016/12/14 07:45:33, Dan Beam wrote: > On 2016/12/13 09:23:38, vabr (Chromium) wrote: > > Not sure if this is reduntant, give the [dir='rtl']? > > it is, but it's probably fine to keep it here and add a note as it's uncommon (I > would probably assume it was accidentally omitted) Good idea about the note, I added it now.
The CQ bit was checked by vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2567693002/#ps60001 (title: "Add a note about direction")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481702780948040,
"parent_rev": "7e2483a049191d5547ec8f74839d7d93f2364283", "commit_rev":
"f66078876a1b99841c36887acb1455ce56347404"}
Message was sent while issue was closed.
Description was changed from ========== 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-Nam... BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== 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-Nam... BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567693002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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-Nam... BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2567693002 ========== to ========== 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-Nam... BUG=667130 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f Cr-Commit-Position: refs/heads/master@{#438469} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7f5230e3d8213f33ba0f1014b1e682f75e36028f Cr-Commit-Position: refs/heads/master@{#438469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
