Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732003003:
Make initial focus consistent in all instances of cr-drawer. (Closed)
|
Created:
3 years, 9 months ago by hcarmona Modified:
3 years, 6 months ago CC:
chromium-reviews, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, tsergeant Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake initial focus consistent in all instances of cr-drawer.
This removes focus ring from "Chrome history" and "Extensions" when
showing cr-drawer using keyboard.
BUG=NONE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2732003003
Cr-Commit-Position: refs/heads/master@{#482044}
Committed: https://chromium.googlesource.com/chromium/src/+/c685d499f1a3b958c039c79c8456792746ce85bd
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update #
Total comments: 2
Patch Set 3 : add quotes #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== MD History: Make heading focusable in side nav. R=dbeam@chromium.org BUG=697343 ========== to ========== MD History: Make heading focusable in side nav. R=dbeam@chromium.org BUG=697343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
PTAL, similar change as settings. I can combine into 1 CL if you'd like.
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.
lgtm but you might want tsergeant@'s opinion
hcarmona@chromium.org changed reviewers: + tsergeant@chromium.org
+tsergeant@ PTAL
lgtm https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/app.html (right): https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/app.html:32: outline: none; Nit: Can this live in cr-drawer directly, to avoid duplicating it everywhere?
https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/app.html (right): https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/app.html:32: outline: none; On 2017/03/07 01:54:01, tsergeant wrote: > Nit: Can this live in cr-drawer directly, to avoid duplicating it everywhere? if we create a slot and wrap it like in cr-dialog https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog...
https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/app.html (right): https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/app.html:32: outline: none; On 2017/03/07 01:57:36, Dan Beam wrote: > On 2017/03/07 01:54:01, tsergeant wrote: > > Nit: Can this live in cr-drawer directly, to avoid duplicating it everywhere? > > if we create a slot and wrap it like in cr-dialog > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... There's already some CSS in cr-drawer for .drawer-header: https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_drawer...
https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... File chrome/browser/resources/md_history/app.html (right): https://codereview.chromium.org/2732003003/diff/1/chrome/browser/resources/md... chrome/browser/resources/md_history/app.html:32: outline: none; On 2017/03/07 01:58:43, tsergeant wrote: > On 2017/03/07 01:57:36, Dan Beam wrote: > > On 2017/03/07 01:54:01, tsergeant wrote: > > > Nit: Can this live in cr-drawer directly, to avoid duplicating it > everywhere? > > > > if we create a slot and wrap it like in cr-dialog > > > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog... > > There's already some CSS in cr-drawer for .drawer-header: > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_drawer... I kind of meant: <dom-module id="cr-drawer"> <template> <style> .header-wrapper { outline: none; } </style> <div class="header-wrapper" tabindex="-1"> <content select=".drawer-header"></content> </div> </template> </dom-module> <dialog is="cr-drawer"> <div class="drawer-header">$i18n{title}</div> </dialog> or just make a drawerHeader: String attribute and lay it out, I guess...
Description was changed from ========== MD History: Make heading focusable in side nav. R=dbeam@chromium.org BUG=697343 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Make heading focusable in side nav. Removes focus ring from "Chrome history" when showing the side nav using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
hcarmona@chromium.org changed reviewers: - dbeam@chromium.org
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...
Description was changed from ========== MD History: Make heading focusable in side nav. Removes focus ring from "Chrome history" when showing the side nav using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make cr-drawer heading consistently focusable. Removes focus ring from "Chrome history" and "Extensions" when showing the side nav using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
hcarmona@chromium.org changed reviewers: + michaelpg@chromium.org
I had forgotten about this CL, updated w/ feedback and made consistent across settings, history and extensions. Updated description since this isn't history specific anymore. +michael for ui/webui/resources/cr_elements/*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
is the CL description related to the title or an "also" thing? https://codereview.chromium.org/2732003003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html (right): https://codereview.chromium.org/2732003003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:73: <div class="drawer-header" tabindex=-1>[[title]]</div> use quotes around attribute values
still lgtm, by the way. Thanks for following up!
Description was changed from ========== Make cr-drawer heading consistently focusable. Removes focus ring from "Chrome history" and "Extensions" when showing the side nav using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make initial focus consistent in all instances of cr-drawer. This removes focus ring from "Chrome history" and "Extensions" when showing cr-drawer using keyboard. BUG=NONE 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...
Updated description to be more clear https://codereview.chromium.org/2732003003/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html (right): https://codereview.chromium.org/2732003003/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_drawer/cr_drawer.html:73: <div class="drawer-header" tabindex=-1>[[title]]</div> On 2017/06/21 00:36:08, michaelpg wrote: > use quotes around attribute values Done.
lgtm
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 dbeam@chromium.org, tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2732003003/#ps40001 (title: "add quotes")
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": 40001, "attempt_start_ts": 1498255740161650,
"parent_rev": "608a78176c1694132ba965977940b7148b2069c6", "commit_rev":
"c685d499f1a3b958c039c79c8456792746ce85bd"}
Message was sent while issue was closed.
Description was changed from ========== Make initial focus consistent in all instances of cr-drawer. This removes focus ring from "Chrome history" and "Extensions" when showing cr-drawer using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make initial focus consistent in all instances of cr-drawer. This removes focus ring from "Chrome history" and "Extensions" when showing cr-drawer using keyboard. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2732003003 Cr-Commit-Position: refs/heads/master@{#482044} Committed: https://chromium.googlesource.com/chromium/src/+/c685d499f1a3b958c039c79c8456... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c685d499f1a3b958c039c79c8456... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
