Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949613003:
Clear initial focus in Sync Confirmation dialog. (Closed)
DescriptionClear initial focus in Sync Confirmation dialog.
BUG=601658
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/89613e4d7d6c50af391960ba679689ebedb9bb77
Cr-Commit-Position: refs/heads/master@{#391818}
Patch Set 1 #Patch Set 2 : Clear initial focus after initial display. #
Total comments: 2
Patch Set 3 : Added TODO #Patch Set 4 : Fix unit tests #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Focus "OK, Got it" initially in Sync Confirmation dialog. BUG=601658 ========== to ========== Focus "OK, Got it" initially in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
anthonyvd@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, can you please take a look at this small change? Here's the rationale: when the dialog is shown on Mac, the "settings" link is automatically focused as it's the first focusable element on the page. On other platforms, that's not the case (there is no visible focused element initially). This change ensures "OK, Got it" is initially focused on mac since it should be the default action. I'd like to get your opinion about this. Specifically, is there a way that you know of to make sure the Mac dialog would not have anything focused initially? That would allow consistency with other platforms. Thanks!
adding a tabindex >= 0 means something /can be focused/ it's possible your dialog attempts to set initial focus and chooses that button because of this change, BUT a) you typically should only use tabindex=0 to make something focusable (not 1) b) you probably just want to handle Enter without changing the focus have you ever used a screenreader?
On 2016/05/04 18:05:11, Dan Beam wrote: > adding a tabindex >= 0 means something /can be focused/ > > it's possible your dialog attempts to set initial focus and chooses that button > because of this change, BUT My code doesn't do anything fancy to set initial focus, so I'm thinking that it might be some behavior of constrained_window/webcontents/something like that. I'm not too familiar with the internals though, so I have no idea how to figure out exactly why. Any insight would be appreciated. > > a) you typically should only use tabindex=0 to make something focusable (not 1) > b) you probably just want to handle Enter without changing the focus We do handle Enter as well, it's the initial focus that's wrong in this case. Ideally I'd want nothing to be focused initially like on other platforms but that doesn't seem to be the behavior of the webview. > > have you ever used a screenreader? Only for some testing, why?
On 2016/05/04 18:20:16, anthonyvd wrote: > On 2016/05/04 18:05:11, Dan Beam wrote: > > adding a tabindex >= 0 means something /can be focused/ > > > > it's possible your dialog attempts to set initial focus and chooses that > button > > because of this change, BUT also, if you add this tabindex=1 it'll mess up the natural tab order :( > > My code doesn't do anything fancy to set initial focus, so I'm thinking that it > might be some behavior of constrained_window/webcontents/something like that. > I'm not too familiar with the internals though, so I have no idea how to figure > out exactly why. Any insight would be appreciated. that's unfortunate. can we turn it off? maybe consult an OWNER. > > > > > a) you typically should only use tabindex=0 to make something focusable (not > 1) > > b) you probably just want to handle Enter without changing the focus > > We do handle Enter as well, it's the initial focus that's wrong in this case. > Ideally I'd want nothing to be focused initially like on other platforms but > that doesn't seem to be the behavior of the webview. maybe we should just be calling document.activeElement.blur() in the contents then? > > > > > have you ever used a screenreader? > > Only for some testing, why? on starting the dialog, the screenreader's position will likely start at the focused button rather than the top of the dialog (which is probably preferred because a blind user may not know what they're submitting)
On 2016/05/04 18:27:16, Dan Beam wrote: > On 2016/05/04 18:20:16, anthonyvd wrote: > > On 2016/05/04 18:05:11, Dan Beam wrote: > > > adding a tabindex >= 0 means something /can be focused/ > > > > > > it's possible your dialog attempts to set initial focus and chooses that > > button > > > because of this change, BUT > > also, if you add this tabindex=1 it'll mess up the natural tab order :( > > > > > My code doesn't do anything fancy to set initial focus, so I'm thinking that > it > > might be some behavior of constrained_window/webcontents/something like that. > > I'm not too familiar with the internals though, so I have no idea how to > figure > > out exactly why. Any insight would be appreciated. > > that's unfortunate. can we turn it off? maybe consult an OWNER. I'll be trying to track down who to talk to about this, but it's unclear to me which part of the UI stack doing it just yet. Since the flow is launching in M51, we're looking into a fix for this specific instance to merge back while looking into the more generic case. > > > > > > > > > a) you typically should only use tabindex=0 to make something focusable (not > > 1) > > > b) you probably just want to handle Enter without changing the focus > > > > We do handle Enter as well, it's the initial focus that's wrong in this case. > > Ideally I'd want nothing to be focused initially like on other platforms but > > that doesn't seem to be the behavior of the webview. > > maybe we should just be calling document.activeElement.blur() in the contents > then? That's a good idea. I went ahead and implemented that. It has to happen after displaying the dialog, hence my current implementation. > > > > > > > > > have you ever used a screenreader? > > > > Only for some testing, why? > > on starting the dialog, the screenreader's position will likely start at the > focused button rather than the top of the dialog (which is probably preferred > because a blind user may not know what they're submitting) I see how that's far from ideal. Thanks for the explanation.
lgtm https://codereview.chromium.org/1949613003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right): https://codereview.chromium.org/1949613003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:127: web_ui()->CallJavascriptFunction("sync.confirmation.clearFocus"); please put a TODO about hunting down the answer to "how do we start unfocused by default?"
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1949613003/#ps40001 (title: "Added TODO")
https://codereview.chromium.org/1949613003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right): https://codereview.chromium.org/1949613003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:127: web_ui()->CallJavascriptFunction("sync.confirmation.clearFocus"); On 2016/05/04 20:55:25, Dan Beam wrote: > please put a TODO about hunting down the answer to "how do we start unfocused by > default?" Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949613003/40001
The CQ bit was unchecked by anthonyvd@chromium.org
Description was changed from ========== Focus "OK, Got it" initially in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear initial focus in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by anthonyvd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949613003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by anthonyvd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1949613003/#ps60001 (title: "Fix unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949613003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949613003/60001
Message was sent while issue was closed.
Description was changed from ========== Clear initial focus in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear initial focus in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clear initial focus in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Clear initial focus in Sync Confirmation dialog. BUG=601658 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/89613e4d7d6c50af391960ba679689ebedb9bb77 Cr-Commit-Position: refs/heads/master@{#391818} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/89613e4d7d6c50af391960ba679689ebedb9bb77 Cr-Commit-Position: refs/heads/master@{#391818} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
