Hotword: Make the opt-in flow tab ordering more intuitive

Hotword: Make the opt-in flow tab ordering more intuitive

Makes the primary buttons first in tab order.

BUG=466390
Committed: https://crrev.com/f1df20df06365f2c8e41872c567e4801a8cf9392
Cr-Commit-Position: refs/heads/master@{#321501}

[kcarattini, 2015-03-18 01:11:02]

kcarattini@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-03-18 01:14:50]

i don't think this is a good idea.  tab order should be byte-stream order, and
that should make left -> right, top -> down (in LTR)

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
File
chrome/browser/resources/hotword_audio_verification/steps/finished_step.html
(right):

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword_audio_verification/steps/finished_step.html:24:
<div class="right">
why is this needed, then?

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword_audio_verification/steps/start_step.html
(right):

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword_audio_verification/steps/start_step.html:14:
<button id="hotword-start" tabindex="1" i18n-content="introStart"></button>
80 col wrap

[kcarattini, 2015-03-18 01:20:13]

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
File
chrome/browser/resources/hotword_audio_verification/steps/finished_step.html
(right):

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword_audio_verification/steps/finished_step.html:24:
<div class="right">
On 2015/03/18 01:14:50, Dan Beam wrote:
> why is this needed, then?

Wow, you were quick! This was a mistake. Added back in.

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
File chrome/browser/resources/hotword_audio_verification/steps/start_step.html
(right):

https://codereview.chromium.org/1011283003/diff/1/chrome/browser/resources/ho...
chrome/browser/resources/hotword_audio_verification/steps/start_step.html:14:
<button id="hotword-start" tabindex="1" i18n-content="introStart"></button>
On 2015/03/18 01:14:50, Dan Beam wrote:
> 80 col wrap

Done.

[Dan Beam, 2015-03-18 01:21:14]

On 2015/03/18 01:14:50, Dan Beam wrote:
> i don't think this is a good idea.  tab order should be byte-stream order, and
> that should make left -> right, top -> down (in LTR)

^

[kcarattini, 2015-03-18 01:21:38]

On 2015/03/18 01:14:50, Dan Beam wrote:
> i don't think this is a good idea.  tab order should be byte-stream order, and
> that should make left -> right, top -> down (in LTR)

The idea was that it should be easier to navigate through the flow with the
keyboard. In the natural flow, the close button is always the first thing tabbed
to (see bug).

[Dan Beam, 2015-03-18 01:38:27]

On 2015/03/18 01:21:38, kcarattini wrote:
> On 2015/03/18 01:14:50, Dan Beam wrote:
> > i don't think this is a good idea.  tab order should be byte-stream order,
and
> > that should make left -> right, top -> down (in LTR)
> 
> The idea was that it should be easier to navigate through the flow with the
> keyboard. In the natural flow, the close button is always the first thing
tabbed
> to (see bug).

how is the close button styled/where is the markup?

btw, we don't put close buttons in the tab order at all in most webui. 
http://crbug.com/457754

[kcarattini, 2015-03-18 01:40:03]

On 2015/03/18 01:38:27, Dan Beam wrote:
> On 2015/03/18 01:21:38, kcarattini wrote:
> > On 2015/03/18 01:14:50, Dan Beam wrote:
> > > i don't think this is a good idea.  tab order should be byte-stream order,
> and
> > > that should make left -> right, top -> down (in LTR)
> > 
> > The idea was that it should be easier to navigate through the flow with the
> > keyboard. In the natural flow, the close button is always the first thing
> tabbed
> > to (see bug).
> 
> how is the close button styled/where is the markup?
> 
> btw, we don't put close buttons in the tab order at all in most webui. 
> http://crbug.com/457754

It's the span with class "close" in all of the files. We were told to make it
navigatable by keyboard for accessibility.

[Dan Beam, 2015-03-18 01:52:21]

dbeam@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Dan Beam, 2015-03-18 01:52:21]

+dmazzoni@

On 2015/03/18 01:40:03, kcarattini wrote:
> On 2015/03/18 01:38:27, Dan Beam wrote:
> > On 2015/03/18 01:21:38, kcarattini wrote:
> > > On 2015/03/18 01:14:50, Dan Beam wrote:
> > > > i don't think this is a good idea.  tab order should be byte-stream
order,
> > and
> > > > that should make left -> right, top -> down (in LTR)
> > > 
> > > The idea was that it should be easier to navigate through the flow with
the
> > > keyboard. In the natural flow, the close button is always the first thing
> > tabbed
> > > to (see bug).
> > 
> > how is the close button styled/where is the markup?
> > 
> > btw, we don't put close buttons in the tab order at all in most webui. 
> > http://crbug.com/457754
> 
> It's the span with class "close" in all of the files. We were told to make it
> navigatable by keyboard for accessibility.

if this is the only close button, I'd recommend leaving the tab orders without
explicit values and doing something like:
- calling cr.ui.setInitialFocus[1]
- using a <form> maybe?
- or writing/using some custom code to click the default button on enter with no
focus[2].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[kcarattini, 2015-03-18 04:31:23]

On 2015/03/18 01:52:21, Dan Beam wrote:
> +dmazzoni@
> 
> On 2015/03/18 01:40:03, kcarattini wrote:
> > On 2015/03/18 01:38:27, Dan Beam wrote:
> > > On 2015/03/18 01:21:38, kcarattini wrote:
> > > > On 2015/03/18 01:14:50, Dan Beam wrote:
> > > > > i don't think this is a good idea.  tab order should be byte-stream
> order,
> > > and
> > > > > that should make left -> right, top -> down (in LTR)
> > > > 
> > > > The idea was that it should be easier to navigate through the flow with
> the
> > > > keyboard. In the natural flow, the close button is always the first
thing
> > > tabbed
> > > > to (see bug).
> > > 
> > > how is the close button styled/where is the markup?
> > > 
> > > btw, we don't put close buttons in the tab order at all in most webui. 
> > > http://crbug.com/457754
> > 
> > It's the span with class "close" in all of the files. We were told to make
it
> > navigatable by keyboard for accessibility.
> 
> if this is the only close button, I'd recommend leaving the tab orders without
> explicit values and doing something like:
> - calling cr.ui.setInitialFocus[1]
> - using a <form> maybe?
> - or writing/using some custom code to click the default button on enter with
no
> focus[2].
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

Thanks for the pointers! I opted for setInitialFocus so that it's clear what
pressing 'enter' next will do.

[Dan Beam, 2015-03-19 02:38:47]

https://codereview.chromium.org/1011283003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/1011283003/diff/60001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:556:
cr.ui.setInitialFocus(currentStep.querySelector('.buttonbar'));
this kinda defeats the purpose.  you should be passing |currentStep| instead

[kcarattini, 2015-03-19 02:59:39]

https://codereview.chromium.org/1011283003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/1011283003/diff/60001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:556:
cr.ui.setInitialFocus(currentStep.querySelector('.buttonbar'));
On 2015/03/19 02:38:47, Dan Beam wrote:
> this kinda defeats the purpose.  you should be passing |currentStep| instead

Done.

[Dan Beam, 2015-03-19 18:48:58]

lgtm

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/flow.js (right):

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/flow.js:555: 
btw, you're likely to have problems trying to immediately focus after unhiding
:( might want to put this into a setTimeout() if it's not working for you
locally

[Dan Beam, 2015-03-19 18:50:06]

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.html (right):

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.html:11: <script
src="chrome://resources/js/cr/ui/node_utils.js"></script>
i can also fix node_utils.js to export the cr.ui namespace if you don't want to
include ui.js (I think)

[Dan Beam, 2015-03-19 18:52:24]

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/hotword_audio_verification/main.html (right):

https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
chrome/browser/resources/hotword_audio_verification/main.html:10: <script
src="chrome://resources/js/cr/ui.js"></script>
actually, you shouldn't need ui.js at all, as far as i can tell

[kcarattini, 2015-03-20 00:50:59]

On 2015/03/19 18:52:24, Dan Beam wrote:
>
https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
> File chrome/browser/resources/hotword_audio_verification/main.html (right):
> 
>
https://codereview.chromium.org/1011283003/diff/80001/chrome/browser/resource...
> chrome/browser/resources/hotword_audio_verification/main.html:10: <script
> src="chrome://resources/js/cr/ui.js"></script>
> actually, you shouldn't need ui.js at all, as far as i can tell

Removed ui.js, and I haven't seen an issue with focusing immediately after
unhiding; it seems to work well locally. Thanks!

[kcarattini, 2015-03-20 00:51:20]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2015-03-20 00:51:21]

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/1011283003/#ps100001
(title: "Review Comments")

[commit-bot: I haz the power, 2015-03-20 00:51:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011283003/100001

[commit-bot: I haz the power, 2015-03-20 02:29:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-20 02:31:03]

Patchset 6 (id:??) landed as
https://crrev.com/f1df20df06365f2c8e41872c567e4801a8cf9392
Cr-Commit-Position: refs/heads/master@{#321501}