Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860423002:
Fix the alignment on chrome://settings/passwords (Closed)
|
Created:
3 years, 7 months ago by vasilii Modified:
3 years, 7 months ago Reviewers:
hcarmona CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the alignment on chrome://settings/passwords
The problem was in the "more" icon that was made independent from the password column in https://codereview.chromium.org/2805683002 because it produced interesting effects when not enough space for the password.
Now an invisible icon is a part of the header to fix the alignment.
BUG=718802
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2860423002
Cr-Commit-Position: refs/heads/master@{#473552}
Committed: https://chromium.googlesource.com/chromium/src/+/124fa61bdad949dc74f5ffb4c59332ca6ab168e7
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments #Patch Set 3 : tests #
Total comments: 4
Patch Set 4 : nits #
Messages
Total messages: 31 (20 generated)
Description was changed from ========== Fix the alignment on chrome://settings/passwords The problem was in the "more" icon that was made independent from the password column in https://codereview.chromium.org/2805683002 because it produced interesting effects when not enough space for the password. Now an invisible icon is a part of the header to fix the alignment. BUG=718802 ========== to ========== Fix the alignment on chrome://settings/passwords The problem was in the "more" icon that was made independent from the password column in https://codereview.chromium.org/2805683002 because it produced interesting effects when not enough space for the password. Now an invisible icon is a part of the header to fix the alignment. BUG=718802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by vasilii@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...
vasilii@chromium.org changed reviewers: + hcarmona@chromium.org
Hi Hector, please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:41: id="username">[[item.loginPair.username]] Let's reformat this so it's more readable: <div class="username-column selectable text-elide" id="username"> [[item.loginPair.username]] </div> https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:55: </div> We can move this div so the button is inside, then we can add a CSS rule to not shrink the button and we should be aligned again without the need for an extra invisible button :-) https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:65: <paper-icon-button class="invisible"> Let's not add a hidden paper-icon-button just to align. paper-icon-button is expensive and we're replacing them in a different CL. https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html:15: .invisible { Not needed if we remove the button.
The CQ bit was checked by vasilii@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...
https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:41: id="username">[[item.loginPair.username]] On 2017/05/09 19:23:28, hcarmona wrote: > Let's reformat this so it's more readable: > > <div class="username-column selectable text-elide" id="username"> > [[item.loginPair.username]] > </div> Done. https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:55: </div> On 2017/05/09 19:23:28, hcarmona wrote: > We can move this div so the button is inside, then we can add a CSS rule to not > shrink the button and we should be aligned again without the need for an extra > invisible button :-) Done. https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:65: <paper-icon-button class="invisible"> On 2017/05/09 19:23:28, hcarmona wrote: > Let's not add a hidden paper-icon-button just to align. > paper-icon-button is expensive and we're replacing them in a different CL. Done. https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html (right): https://codereview.chromium.org/2860423002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html:15: .invisible { On 2017/05/09 19:23:28, hcarmona wrote: > Not needed if we remove the button. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vasilii@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...
There is a warning from the WebUI test: Accessibility issues found on chrome://md-settings/ *** Begin accessibility audit results *** An accessibility audit found Errors: Error: AX_ARIA_04 (ARIA state and property values must be valid) failed on the following elements (1 - 3 of 3): #input #input #input See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. Error: AX_ARIA_02 (ARIA attributes which refer to other elements by ID should refer to elements which exist in the DOM) failed on the following elements (1 - 3 of 3): #input #input #input See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. *** End accessibility audit results ***", source: test_api.js (376) It's independent from my changes.
On 2017/05/19 16:28:25, vasilii wrote: > There is a warning from the WebUI test: > > Accessibility issues found on chrome://md-settings/ > *** Begin accessibility audit results *** > An accessibility audit found > Errors: > Error: AX_ARIA_04 (ARIA state and property values must be valid) failed on the > following elements (1 - 3 of 3): > #input > #input > #input > See > https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... > for more information. > > Error: AX_ARIA_02 (ARIA attributes which refer to other elements by ID should > refer to elements which exist in the DOM) failed on the following elements (1 - > 3 of 3): > #input > #input > #input > See > https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... > for more information. > > > *** End accessibility audit results ***", source: test_api.js (376) > > It's independent from my changes. Our a11y tests are not currently working well :-( We have a plan to update them over the summer to make sure we get good coverage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM w/ some nits https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:33: id="username">[[item.loginPair.username]]</div> Nit: Indenting is off, should be indented 4x https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:38: disabled value="[[getEmptyPassword_(item.numCharactersInPassword)]]"> Nit: indent is off, needs to be indented 4x
Thanks. https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:33: id="username">[[item.loginPair.username]]</div> On 2017/05/19 17:46:32, hcarmona wrote: > Nit: Indenting is off, should be indented 4x Done. https://codereview.chromium.org/2860423002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:38: disabled value="[[getEmptyPassword_(item.numCharactersInPassword)]]"> On 2017/05/19 17:46:32, hcarmona wrote: > Nit: indent is off, needs to be indented 4x Done.
The CQ bit was checked by vasilii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2860423002/#ps60001 (title: "nits")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vasilii@chromium.org
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": 1495451024534070,
"parent_rev": "6bd271fe88b9917fef4206aecc60b39b4f58652e", "commit_rev":
"124fa61bdad949dc74f5ffb4c59332ca6ab168e7"}
Message was sent while issue was closed.
Description was changed from ========== Fix the alignment on chrome://settings/passwords The problem was in the "more" icon that was made independent from the password column in https://codereview.chromium.org/2805683002 because it produced interesting effects when not enough space for the password. Now an invisible icon is a part of the header to fix the alignment. BUG=718802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix the alignment on chrome://settings/passwords The problem was in the "more" icon that was made independent from the password column in https://codereview.chromium.org/2805683002 because it produced interesting effects when not enough space for the password. Now an invisible icon is a part of the header to fix the alignment. BUG=718802 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2860423002 Cr-Commit-Position: refs/heads/master@{#473552} Committed: https://chromium.googlesource.com/chromium/src/+/124fa61bdad949dc74f5ffb4c593... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/124fa61bdad949dc74f5ffb4c593... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
