Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442333003:
Elide origin URLs from the left (Closed)
|
Created:
4 years, 1 month ago by jdoerrie Modified:
4 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, kolos1, Evan Stade Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionElide origin URLs from the left
This change elides long origin URLs from the left, so that the high level domain
of the domain remains visible. This is beneficial for security reasons.
BUG=620007
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/42a5543b03b6b099cc52f6d672250c16b8ace86f
Cr-Commit-Position: refs/heads/master@{#435904}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed nits, fixed types and added test. #
Total comments: 7
Patch Set 3 : Simplify Elided Origins Test #
Total comments: 1
Patch Set 4 : Pure CSS Solution #
Total comments: 3
Patch Set 5 : Reformat Comment Block #Messages
Total messages: 41 (17 generated)
Description was changed from ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. This logic is already done in the old passwords dialogue, this change makes the two dialogues consistent. BUG=620007 ========== to ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. This logic is already done in the old passwords dialogue, this change makes the two dialogues consistent. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jdoerrie@chromium.org changed reviewers: + hcarmona@chromium.org
The CQ bit was checked by jdoerrie@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
LGTM with a nit. https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:63: <div class="website-column">$i18n{editPasswordWebsiteLabel}</div> This will be returned from |this.$$('.website-column')| Just checking that this is the intended element and not the one inside the template. They should be the same size. https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:168: * @param {string} elided originUrl @return {string} Instructions for running the closure compiler locally: https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... TL;DR: `third_party/closure_compiler/run_compiler` (Might need to run `sudo apt-get install openjdk-7-jre`)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
please add tests for your binary search https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:184: } no curlies https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:196: } no curlies
https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:80: </a> This change currently breaks browser tests, the gist is the following: [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection verifySavedPasswordLength AssertionError: expected ' site1.com ' to equal 'site1.com' [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection verifyPasswordListRemove AssertionError: expected ' anotherwebsite.com ' to equal 'anotherwebsite.com' [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection verifyFilterPasswords AssertionError: expected ' one.com ' to equal 'one.com' When I join the three lines into one the tests are fixed, but I end up with a line length of 99 characters, most likely breaking the style guide. What do you suggest I do here?
On 2016/10/25 11:47:01, jdoerrie wrote: > https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html > (right): > > https://codereview.chromium.org/2442333003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:80: > </a> > This change currently breaks browser tests, the gist is the following: > > [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection > verifySavedPasswordLength > AssertionError: expected ' > http://site1.com > ' to equal 'site1.com' > > [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection > verifyPasswordListRemove > AssertionError: expected ' > http://anotherwebsite.com > ' to equal 'anotherwebsite.com' > > [46691:1287:1024/113414:ERROR:CONSOLE(48)] "Mocha test failed: PasswordsSection > verifyFilterPasswords > AssertionError: expected ' > http://one.com > ' to equal 'one.com' > > When I join the three lines into one the tests are fixed, but I end up with a > line length of 99 characters, most likely breaking the style guide. What do you > suggest I do here? If you call .trim() on the text content before comparing in the test it will get rid of whitespace.
https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:7: <link rel="import" href="/global_scroll_target_behavior.html"> I git rebase-updated, not my change. https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:75: scroll-target="[[scrollTarget]]"> Same. https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js (right): https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.js:23: behaviors: [CrScrollableBehavior, settings.GlobalScrollTargetBehavior], Same.
The CQ bit was checked by jdoerrie@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...
Thanks for tests :-) https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:7: <link rel="import" href="/global_scroll_target_behavior.html"> On 2016/10/25 16:17:13, jdoerrie wrote: > I git rebase-updated, not my change. No need to mention this, there's some cool extensions that can help with looking at diffs: http://dev.chromium.org/developers/useful-extensions https://codereview.chromium.org/2442333003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2442333003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:99: for (var i = 0; i < children.length; ++i) { iron-list will only create enough children to fill the viewable area. We've run into issues where a test would sometimes fail if not all children are created. We decided that testing the first child would be OK, because the list should* render at least 1 item. *iron-list won't render any children if it's height is 0. https://codereview.chromium.org/2442333003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:269: var passwordsSection = self.createPasswordsSection_(passwordList, []); Can we simplify the test by calling |elideOriginUrlFromLeft_| directly? like: assertEquals('\u2026website.name.com', passwordsSection.elideOriginUrlFromLeft_( 'another.very.long.and.completely.unrealistic.website.name.com')); assertEquals('name.com', passwordsSection.elideOriginUrlFromLeft_('name.com')); This way we can eliminate iterating over the iron-list. Also, we wouldn't need to maintain 2 implementations of left elide if we use known values.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2442333003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js (right): https://codereview.chromium.org/2442333003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:109: * replaces the binary search with a linear search. waiiiiiit, what? don't do this. you're not testing anything useful...
not lgtm until we figure out a way to test the real logic rather than ... writing a whole different copy for testing
On 2016/10/26 02:25:23, Dan Beam wrote: > not lgtm until we figure out a way to test the real logic rather than ... > writing a whole different copy for testing Thanks for the feedback. I believe the logic I am testing depends on the CSS styles that are loaded in that scope, especially font. This is why I did not want to hard code values, that need to be updated when the CSS changes, and rewrote the logic, where it is easier to reason about it correctness. I don't feel too strongly about this, so I will create another patch set checking against hard coded expected values, similarly to what Hector suggested.
https://codereview.chromium.org/2442333003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:27: flex: 3; A mixin might make this easier to test: .website-column { flex: 3; @apply(--website-column); } Then in your test you would need to define the mixin: <specify selector here> { --website-column: { flex: 0; width: 100px; }; } This way even if the css changes, the css in the test won't. You can also add anything else you might need to keep constant. More details about mixins in polymer: https://www.polymer-project.org/1.0/docs/devguide/styling
On 2016/10/26 14:04:36, Hector Carmona wrote: > https://codereview.chromium.org/2442333003/diff/40001/chrome/browser/resource... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html > (right): > > https://codereview.chromium.org/2442333003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:27: > flex: 3; > A mixin might make this easier to test: > > .website-column { > flex: 3; > @apply(--website-column); > } > > Then in your test you would need to define the mixin: > > <specify selector here> { > --website-column: { > flex: 0; > width: 100px; > }; > } > > This way even if the css changes, the css in the test won't. > You can also add anything else you might need to keep constant. > > More details about mixins in polymer: > https://www.polymer-project.org/1.0/docs/devguide/styling After talking to dschuyler@ yesterday we found a solution that only requires CSS. Here is a screenshot of the style sheet in action: https://bugs.chromium.org/p/chromium/issues/attachment?aid=256902&inline=1 and you can play around here: https://jsbin.com/diqurafute/2/edit?html,css,output The test now does not make sense anymore, so I removed it. Fixing CSS properties via mixins in JavaScript does not seem possible, they are stripped from the CSS before any JavaScript is run. Most likely, one would need to make use of CSSStyleSheet.insertRule() (https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule) in order to get the desired effect. On another note, usernames are currently not properly elided either and can break the design as well: https://bugs.chromium.org/p/chromium/issues/attachment?aid=256903&inline=1 But that might be a topic for another CL.
LGTM, let's just update the description to remove: This logic is already done in the old passwords dialogue, this change makes the two dialogues consistent. b/c implementation is no longer hte same as old dialog
Description was changed from ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. This logic is already done in the old passwords dialogue, this change makes the two dialogues consistent. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: direction: rtl; this is not the same, this is what previous UIs did
On 2016/10/28 22:41:43, Dan Beam wrote: > https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html > (right): > > https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: > direction: rtl; > this is not the same, this is what previous UIs did https://codereview.chromium.org/1823423002/
/cc estade@ and kolos@
On 2016/10/29 00:14:40, Dan Beam wrote: > /cc estade@ and kolos@ Friendly ping on this one. Is there anything else you would like to see tested, dbeam@? Screenshots are provided in http://crbug.com/620007#c10, copy-pasting and using screen-readers seem to work fine. However, highlighting with the mouse is not intuitive if the origin is elided from the left. https://jsbin.com/diqurafute/2/edit?html,css,output reproduces this to some regard, try highlighting text in the top box. However, I am fine with implementing a JavaScript based solution as well.
so you tried these exact steps, basically? https://bugs.chromium.org/p/chromium/issues/detail?id=595276
On 2016/11/29 01:48:34, Dan Beam wrote: > so you tried these exact steps, basically? > https://bugs.chromium.org/p/chromium/issues/detail?id=595276 Yes, that is exactly what I did. It pronounced the origins in the correct order. If you want to try yourself, I have pasted the patch for the old password settings dialog. As mentioned on the bug, ChromeVox does not work with MD settings due to shadow-dom. However, I did test it on the md settings using the screen reader built into macOS. diff --git a/chrome/browser/resources/options/password_manager.css b/chrome/browser/resources/options/password_manager.css index 33bc94b..dbdb9c0 100644 --- a/chrome/browser/resources/options/password_manager.css +++ b/chrome/browser/resources/options/password_manager.css @@ -28,6 +28,11 @@ html[dir=rtl] #password-search-column { #saved-passwords-list, #password-exceptions-list { + direction: rtl; + overflow: hidden; + text-align: left; + text-overflow: ellipsis; + white-space: nowrap; max-height: 192px; /* For performance (crbug.com/651049). */ min-height: 128px; /* Smallest value rendering enough initial list items. */ } diff --git a/chrome/browser/resources/options/password_manager.js b/chrome/browser/resources/options/password_manager.js index 6263595..abad738 100644 --- a/chrome/browser/resources/options/password_manager.js +++ b/chrome/browser/resources/options/password_manager.js @@ -246,10 +246,10 @@ cr.define('options', function() { entries = entries.filter(filter); } this.savedPasswordsList_.dataModel = new ArrayDataModel(entries); - this.updateListVisibility_(this.savedPasswordsList_); + // this.updateListVisibility_(this.savedPasswordsList_); // updateOriginsEliding_ should be called after updateListVisibility_, // otherwise updateOrigins... might be not able to read width of elements. - this.updateOriginsEliding_(this.savedPasswordsList_); + // this.updateOriginsEliding_(this.savedPasswordsList_); }, /**
ok, we can give this another shot, then... lgtm https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:30: */ /* Comments ... * should be formatted ... * like this. */
https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2442333003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:30: */ On 2016/12/01 18:18:32, Dan Beam wrote: > /* Comments ... > * should be formatted ... > * like this. */ Done.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2442333003/#ps80001 (title: "Reformat Comment Block")
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": 80001, "attempt_start_ts": 1480669871721070,
"parent_rev": "5d6bf7aa6074bcdcf6e43100c490ae32d0a2638f", "commit_rev":
"e83045eb9ed3509d4f837fe67c2c8a55e951e6cd"}
Message was sent while issue was closed.
Description was changed from ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Elide origin URLs from the left This change elides long origin URLs from the left, so that the high level domain of the domain remains visible. This is beneficial for security reasons. BUG=620007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/42a5543b03b6b099cc52f6d672250c16b8ace86f Cr-Commit-Position: refs/heads/master@{#435904} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/42a5543b03b6b099cc52f6d672250c16b8ace86f Cr-Commit-Position: refs/heads/master@{#435904} |
