Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700063002:
MD Settings: focus header when dialog shows to signal to AT what's going on (Closed)
|
Created:
3 years, 10 months ago by Dan Beam Modified:
3 years, 10 months ago Reviewers:
dpapad CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: focus header when dialog shows to signal to AT what's going on
R=dpapad@chromium.org
BUG=686313
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2700063002
Cr-Commit-Position: refs/heads/master@{#452949}
Committed: https://chromium.googlesource.com/chromium/src/+/7a4e410b1926701f8c4e6df1a9735103f9fe9126
Patch Set 1 #Patch Set 2 : different day, same hack #Patch Set 3 : +assert precondition #
Total comments: 7
Patch Set 4 : dpapad@ review #
Total comments: 6
Messages
Total messages: 24 (14 generated)
Description was changed from ========== MD Settings: focus header when dialog shows to signal to AT what's going on R=dpapad@chromium.org BUG=686313 ========== to ========== MD Settings: focus header when dialog shows to signal to AT what's going on R=dpapad@chromium.org BUG=686313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
would require https://codereview.chromium.org/2695013010/
On 2017/02/17 at 06:39:51, dbeam wrote: > would require https://codereview.chromium.org/2695013010/ Ok. Also, once/if the blink CL lands, can we add a cr_dialog test to check that the focus works as expected?
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:44: attached: function() { i also considered overriding showModal() but liked this slightly better
The CQ bit was checked by dbeam@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 2017/02/17 19:05:27, dpapad wrote: > On 2017/02/17 at 06:39:51, dbeam wrote: > > would require https://codereview.chromium.org/2695013010/ > > Ok. Also, once/if the blink CL lands, can we add a cr_dialog test to check that > the focus works as expected? added a test, the blink CL is stalled
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:10: PolymerTest.clearBody(); Nit: You could also do what we do at https://cs.chromium.org/chromium/src/chrome/test/data/webui/cr_elements/cr_ac... instead of programmatically creating the elements to be tested. It seems more readable to me. https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:25: expectEquals(document.activeElement, title); Can we add one more test for a dialog that has an <input> element with autofocus and check that the autofocus wins? https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:44: attached: function() { On 2017/02/23 at 06:22:24, Dan Beam wrote: > i also considered overriding showModal() but liked this slightly better Nit: Add @override Also that looks fine. If/when we address https://bugs.chromium.org/p/chromium/issues/detail?id=668313, we will plobably have to add our own show() method (similar to the cr-action-menu's showAt()), so we could move this in that method.
https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:10: PolymerTest.clearBody(); On 2017/02/23 18:28:35, dpapad wrote: > Nit: You could also do what we do at > https://cs.chromium.org/chromium/src/chrome/test/data/webui/cr_elements/cr_ac... > instead of programmatically creating the elements to be tested. It seems more > readable to me. Done. https://codereview.chromium.org/2700063002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_dialog_test.js:25: expectEquals(document.activeElement, title); On 2017/02/23 18:28:35, dpapad wrote: > Can we add one more test for a dialog that has an <input> element with autofocus > and check that the autofocus wins? Done. https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2700063002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:44: attached: function() { On 2017/02/23 18:28:35, dpapad wrote: > On 2017/02/23 at 06:22:24, Dan Beam wrote: > > i also considered overriding showModal() but liked this slightly better > > Nit: Add @override > > Also that looks fine. If/when we address > https://bugs.chromium.org/p/chromium/issues/detail?id=668313, we will plobably > have to add our own show() method (similar to the cr-action-menu's showAt()), so > we could move this in that method. Done.
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by dbeam@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.
LGTM https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_dialog_test.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. Nit: 2017 https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_dialog_test.js:22: expectTrue(document.activeElement.matches('div.title')); I am not asking you to replace expects with asserts, but can we be at least consistent within the same tests, to use either asserts or expects but not both? https://codereview.chromium.org/2700063002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2700063002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:46: var title = this.getContentChildren('[select=".title"]'); Nit(optional): Perhaps use queryEffectiveChildren instead which returns the 1st occurrence instead of an array?
https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_dialog_test.js (right): https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_dialog_test.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/24 20:46:10, dpapad wrote: > Nit: 2017 Acknowledged. https://codereview.chromium.org/2700063002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_dialog_test.js:22: expectTrue(document.activeElement.matches('div.title')); On 2017/02/24 20:46:10, dpapad wrote: > I am not asking you to replace expects with asserts, but can we be at least > consistent within the same tests, to use either asserts or expects but not both? they're different. https://codereview.chromium.org/2700063002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2700063002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:46: var title = this.getContentChildren('[select=".title"]'); On 2017/02/24 20:46:10, dpapad wrote: > Nit(optional): Perhaps use queryEffectiveChildren instead which returns the 1st > occurrence instead of an array? that's different. that gets all the distributed children. i mean, it'd probably functionally work because the first node with class="title" would likely be in this slot and it'd be crazy to use class="title" inside of class="body", but this is specifically looking for the first child of a specific <content> slot
The CQ bit was checked by dbeam@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": 1487974116377360,
"parent_rev": "2f52fcb3a305091ded42e8fe8ed5a0c0efe03013", "commit_rev":
"7a4e410b1926701f8c4e6df1a9735103f9fe9126"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: focus header when dialog shows to signal to AT what's going on R=dpapad@chromium.org BUG=686313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: focus header when dialog shows to signal to AT what's going on R=dpapad@chromium.org BUG=686313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2700063002 Cr-Commit-Position: refs/heads/master@{#452949} Committed: https://chromium.googlesource.com/chromium/src/+/7a4e410b1926701f8c4e6df1a973... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7a4e410b1926701f8c4e6df1a973... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
