Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2818283002:
MD-Settings: A11y - Fix password list focusability. (Closed)
|
Created:
3 years, 8 months ago by hcarmona Modified:
3 years, 7 months ago 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. |
DescriptionMD-Settings: A11y - Fix password list focusability.
BUG=705575
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2818283002
Cr-Commit-Position: refs/heads/master@{#469408}
Committed: https://chromium.googlesource.com/chromium/src/+/c84225ee9911f14d05c627a08e2b9851eb2eba24
Patch Set 1 #
Total comments: 6
Patch Set 2 : feedback #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : feedback #
Total comments: 2
Patch Set 5 : nit #Messages
Total messages: 51 (36 generated)
Description was changed from ========== MD-Settings: A11y - Fix password list focusability. BUG=705575 ========== to ========== MD-Settings: A11y - Fix password list focusability. BUG=705575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@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: linux_chromium_chromeos_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 hcarmona@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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hcarmona@chromium.org changed reviewers: + scottchen@chromium.org
PTAL
Friendly ping on this :-)
tested locally, seems to work great. LGTM with nits: https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js (right): https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:10: 'use strict'; I don't see us do this in other component definitions, should we just follow what we're doing in the code base? https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:41: { target: this.$.passwordMenu, item: this.item }); "git cl format --js" seems to want to format this differently, could you run it before uploading next patch?
The CQ bit was checked by hcarmona@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: This issue passed the CQ dry run.
Thanks for the review. I've applied your feedback. I'm going to do a rebase and land after some other changes in this area that we want to merge, having this land first will make the merge more complicated. https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js (right): https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:6: * @fileoverview TODO(hcarmona): write this. Almost missed this. https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:10: 'use strict'; On 2017/04/20 19:10:58, scottchen wrote: > I don't see us do this in other component definitions, should we just follow > what we're doing in the code base? Consistency is good. Removed. https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:41: { target: this.$.passwordMenu, item: this.item }); On 2017/04/20 19:10:58, scottchen wrote: > "git cl format --js" seems to want to format this differently, could you run it > before uploading next patch? Done.
https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js (right): https://codereview.chromium.org/2818283002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.js:6: * @fileoverview TODO(hcarmona): write this. On 2017/04/21 14:52:29, hcarmona wrote: > Almost missed this. I thought you were planning on doing this in another CL :)
The CQ bit was checked by hcarmona@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: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottchen@chromium.org Link to the patchset: https://codereview.chromium.org/2818283002/#ps60001 (title: "rebase")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
hcarmona@chromium.org changed reviewers: + dschuyler@chromium.org
+Dave PTAL, need OWNER
https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:2: <link rel="import" href="chrome://resources/html/polymer.html"> I'd thought these could be alphabetized too, but I've since learned that polymer.html should be imported first (which is an exception to the alphabetize rule). (Though in practice, something importing this file has likely imported polymer.html, so this is just a good practice rather than fixing a specific thing). https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:54: </div> Should this </div> be </template>?
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html (right): https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:2: <link rel="import" href="chrome://resources/html/polymer.html"> On 2017/05/03 21:02:33, dschuyler wrote: > I'd thought these could be alphabetized too, but I've since learned that > polymer.html should be imported first (which is an exception to the alphabetize > rule). (Though in practice, something importing this file has likely imported > polymer.html, so this is just a good practice rather than fixing a specific > thing). Done. https://codereview.chromium.org/2818283002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/password_list_item.html:54: </div> On 2017/05/03 21:02:33, dschuyler wrote: > Should this </div> be </template>? The spacing was weird here. Fixed.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2818283002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html (right): https://codereview.chromium.org/2818283002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html:26: -webkit-margin-start: 8px; tip (not required): if start and end are the same value then margin-left margin-right may be used.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@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/2818283002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html (right): https://codereview.chromium.org/2818283002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_shared_css.html:26: -webkit-margin-start: 8px; On 2017/05/03 22:32:27, dschuyler wrote: > tip (not required): if start and end are the same value then margin-left > margin-right may be used. Good catch. Changed to margin: 0 8px;
The CQ bit was unchecked by hcarmona@chromium.org
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottchen@chromium.org, dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2818283002/#ps100001 (title: "nit")
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 hcarmona@chromium.org
The CQ bit was checked by hcarmona@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": 100001, "attempt_start_ts": 1493922930171810,
"parent_rev": "319966b1a4d8d5f9d1ddf126f3500b89bfde1b33", "commit_rev":
"c84225ee9911f14d05c627a08e2b9851eb2eba24"}
Message was sent while issue was closed.
Description was changed from ========== MD-Settings: A11y - Fix password list focusability. BUG=705575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD-Settings: A11y - Fix password list focusability. BUG=705575 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2818283002 Cr-Commit-Position: refs/heads/master@{#469408} Committed: https://chromium.googlesource.com/chromium/src/+/c84225ee9911f14d05c627a08e2b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c84225ee9911f14d05c627a08e2b... |
