ChromeOS OOBE: fix focus flow in MD-OOBE.

ChromeOS OOBE: fix focus flow in MD-OOBE.

BUG=676774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2688153004
Cr-Commit-Position: refs/heads/master@{#451434}
Committed: https://chromium.googlesource.com/chromium/src/+/bc82717e957aeaf5b865d4f4b4c3ec7ad48360c0

[Alexander Alekseev, 2017-02-11 03:42:21]

Description was changed from

==========
ChromeOS OOBE: fix focus flow in MD-OOBE.

BUG=676774
==========

to

==========
ChromeOS OOBE: fix focus flow in MD-OOBE.

BUG=676774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Alexander Alekseev, 2017-02-11 03:45:07]

alemate@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Alexander Alekseev, 2017-02-11 03:45:08]

This CL fixes accessibility focus flow and several ARIA-labels.

Please review.

[Alexander Alekseev, 2017-02-11 03:45:13]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 03:45:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 08:11:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 08:11:56]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-02-14 01:21:32]

Could you please also have someone more familiar with the login UI review this,
maybe xiyuan or achith? I'm not super familiar with the way the login pages are
structured.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_a11y_option.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_a11y_option.js:26: label: String,
Why does this need to be renamed here and elsewhere? It seemed more clear
before.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_dialog.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_dialog.js:33: var focused =
this.getElementsByClassName('focus-on-show');
nit: focusedElements

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_eula.html (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_eula.html:58:
cclass="focus-on-show">
class

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:121: if (useMDOobe)
if (loadTimeData.getString('newOobeUI') == 'on')

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:147: if
(useMDOobe)
if (loadTimeData.getString('newOobeUI') == 'on')

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: var screen =
this.$[id];
nit: assert(screen) may help other devs in the future.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome_dialog.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome_dialog.js:56: var focused =
this.$[this.focusedElement_];
focusedElement

[achuithb, 2017-02-14 09:06:44]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org, glevin@chromium.org

[achuithb, 2017-02-14 09:06:45]

Greg might be a good reviewer here?

[Alexander Alekseev, 2017-02-14 11:59:27]

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_a11y_option.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_a11y_option.js:26: label: String,
On 2017/02/14 01:21:32, stevenjb wrote:
> Why does this need to be renamed here and elsewhere? It seemed more clear
> before.

Because otherwise it will become aria-label attribute of <oobe-a11y-option> tag
itself in addition to actual input element inside it. So ChromeVox will
pronounce it twice (sometimes).

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_dialog.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_dialog.js:33: var focused =
this.getElementsByClassName('focus-on-show');
On 2017/02/14 01:21:32, stevenjb wrote:
> nit: focusedElements

Done.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_eula.html (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_eula.html:58:
cclass="focus-on-show">
On 2017/02/14 01:21:32, stevenjb wrote:
> class

This was disabled on purpose. Thank you!

Fixed.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_eula.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_eula.js:121: if (useMDOobe)
On 2017/02/14 01:21:32, stevenjb wrote:
> if (loadTimeData.getString('newOobeUI') == 'on')

Done.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:147: if
(useMDOobe)
On 2017/02/14 01:21:32, stevenjb wrote:
> if (loadTimeData.getString('newOobeUI') == 'on')

Done.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: var screen =
this.$[id];
On 2017/02/14 01:21:32, stevenjb wrote:
> nit: assert(screen) may help other devs in the future.

Done.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome_dialog.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome_dialog.js:56: var focused =
this.$[this.focusedElement_];
On 2017/02/14 01:21:32, stevenjb wrote:
> focusedElement

Done.

[Alexander Alekseev, 2017-02-14 11:59:59]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 12:00:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 13:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 13:00:39]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-02-14 17:35:35]

Generally lgtm. The changes I am not so familiar with are:
* oobe_dialog
* oobe_eula
* oobe_welcome
* oobe_welcome_dialog

Basically anything related to how dialogs work in oobe.

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_a11y_option.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_a11y_option.js:26: label: String,
On 2017/02/14 11:59:27, Alexander Alekseev wrote:
> On 2017/02/14 01:21:32, stevenjb wrote:
> > Why does this need to be renamed here and elsewhere? It seemed more clear
> > before.
> 
> Because otherwise it will become aria-label attribute of <oobe-a11y-option>
tag
> itself in addition to actual input element inside it. So ChromeVox will
> pronounce it twice (sometimes).

I see. I'm not super fond of just 'label' for something that will only be used
for aria, maybe 'labelForAria'?

[Alexander Alekseev, 2017-02-14 23:41:17]

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_a11y_option.js (right):

https://codereview.chromium.org/2688153004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_a11y_option.js:26: label: String,
On 2017/02/14 17:35:34, stevenjb wrote:
> On 2017/02/14 11:59:27, Alexander Alekseev wrote:
> > On 2017/02/14 01:21:32, stevenjb wrote:
> > > Why does this need to be renamed here and elsewhere? It seemed more clear
> > > before.
> > 
> > Because otherwise it will become aria-label attribute of <oobe-a11y-option>
> tag
> > itself in addition to actual input element inside it. So ChromeVox will
> > pronounce it twice (sometimes).
> 
> I see. I'm not super fond of just 'label' for something that will only be used
> for aria, maybe 'labelForAria'?

Done.

[Greg Levin, 2017-02-16 17:29:32]

I'm still not fluent in html/js/css, so the code looks fine as far as I can see,
except for one optional nit.  I tested out the patch, and made some
observations.  I'm not sure which fall within the scope of this CL.  There's
only one I consider a blocker, if you want to put the rest of these off till a
future CL and get this one landed.

BLOCKER -> The "Next" button on network page is broken.  It shows the click
animation, but the page doesn't advance.

Other keyboard / accessibility issues:
* No default focus on front (Welcome) page.
* No default focus on network page.  This (sometimes) includes the case where a
list selection opens a dialog, and the dialog is closed.  In test network list,
click vpn1, then close that dialog.  Now nothing has focus.  On the other hand,
click Proxy settings, then close that dialog; the list regains focus (this is
correct behavior).
* ESC key doesn't close a11y or language pages.  I would expect the ESC key to
return to the main Welcome page, and possibly discard any changes (although that
might be more effort than it's worth).  Be careful that if a menu is open, ESC
should only close the menu, not the page.

ChromeVox issues:
* When language or a11y pages opens, a bunch of stuff is read.  Most of it is
fine, but it concludes with "Welcome! Set your language and network", which
seems to be the main page prompt.  This may be difficult to fix; I often don't
understand why CV reads exactly what it does.
* Nothing is read when Network page first comes up.  Having a default focus
might fix this.

https://codereview.chromium.org/2688153004/diff/40001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/2688153004/diff/40001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:1792: Google Chrome OS Terms contents.
nit: No . after label text?

[achuithb, 2017-02-16 17:59:32]

On 2017/02/16 17:29:32, Greg Levin wrote:
> I'm still not fluent in html/js/css, so the code looks fine as far as I can
see,
> except for one optional nit.  I tested out the patch, and made some
> observations.  I'm not sure which fall within the scope of this CL.  There's
> only one I consider a blocker, if you want to put the rest of these off till a
> future CL and get this one landed.
> 
> BLOCKER -> The "Next" button on network page is broken.  It shows the click
> animation, but the page doesn't advance.

This is unrelated to this CL and should be fixed by:
https://codereview.chromium.org/2699633004/

[Greg Levin, 2017-02-16 18:36:21]

On 2017/02/16 17:59:32, achuithb wrote:
> On 2017/02/16 17:29:32, Greg Levin wrote:
> > I'm still not fluent in html/js/css, so the code looks fine as far as I can
> see,
> > except for one optional nit.  I tested out the patch, and made some
> > observations.  I'm not sure which fall within the scope of this CL.  There's
> > only one I consider a blocker, if you want to put the rest of these off till
a
> > future CL and get this one landed.
> > 
> > BLOCKER -> The "Next" button on network page is broken.  It shows the click
> > animation, but the page doesn't advance.
> 
> This is unrelated to this CL and should be fixed by:
> https://codereview.chromium.org/2699633004/

Thanks, verified that the Next button problem exists independently of this CL. 
Since alemate@ want to land this quickly, LGTM, conditional on filing new
issue(s) to address default focus and ESC key problems.

[Alexander Alekseev, 2017-02-17 05:41:34]

On 2017/02/16 17:29:32, Greg Levin wrote:
> I'm still not fluent in html/js/css, so the code looks fine as far as I can
see,
> except for one optional nit.  I tested out the patch, and made some
> observations.  I'm not sure which fall within the scope of this CL.  There's
> only one I consider a blocker, if you want to put the rest of these off till a
> future CL and get this one landed.
> 
> BLOCKER -> The "Next" button on network page is broken.  It shows the click
> animation, but the page doesn't advance.
> 
> Other keyboard / accessibility issues:
> * No default focus on front (Welcome) page.

That's strange, because I see "Language selection button" focused by default.
Both on Linux and on a device.


> * No default focus on network page.  This (sometimes) includes the case where
a
> list selection opens a dialog, and the dialog is closed.  In test network
list,
> click vpn1, then close that dialog.  Now nothing has focus.  On the other
hand,
> click Proxy settings, then close that dialog; the list regains focus (this is
> correct behavior).

Network page is different, so I postponed it to the next CL.


> * ESC key doesn't close a11y or language pages.  I would expect the ESC key to
> return to the main Welcome page, and possibly discard any changes (although
that
> might be more effort than it's worth).  Be careful that if a menu is open, ESC
> should only close the menu, not the page.

Filed https://crbug.com/693374
 
> ChromeVox issues:
> * When language or a11y pages opens, a bunch of stuff is read.  Most of it is
> fine, but it concludes with "Welcome! Set your language and network", which
> seems to be the main page prompt.  This may be difficult to fix; I often don't
> understand why CV reads exactly what it does.

I'll look into this after this CL lands. This one is important for other issues.

> * Nothing is read when Network page first comes up.  Having a default focus
> might fix this.
> 
>
https://codereview.chromium.org/2688153004/diff/40001/chrome/app/chromeos_str...
> File chrome/app/chromeos_strings.grdp (right):
> 
>
https://codereview.chromium.org/2688153004/diff/40001/chrome/app/chromeos_str...
> chrome/app/chromeos_strings.grdp:1792: Google Chrome OS Terms contents.
> nit: No . after label text?

[Alexander Alekseev, 2017-02-17 05:43:42]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2017-02-17 05:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, glevin@chromium.org
Link to the patchset: https://codereview.chromium.org/2688153004/#ps70001
(title: "Message updated after review.")

[commit-bot: I haz the power, 2017-02-17 05:44:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 05:47:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 05:47:42]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Alekseev, 2017-02-18 01:53:56]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2017-02-18 01:53:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, glevin@chromium.org
Link to the patchset: https://codereview.chromium.org/2688153004/#ps90001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-02-18 01:54:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 03:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:56:13]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Alexander Alekseev, 2017-02-18 05:26:37]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2017-02-18 05:27:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 06:56:17]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1487395597112200,
"parent_rev": "e21cbdc60d0f618166b2d53ebb306e8e79d664f0", "commit_rev":
"bc82717e957aeaf5b865d4f4b4c3ec7ad48360c0"}

[commit-bot: I haz the power, 2017-02-18 06:56:51]

Description was changed from

==========
ChromeOS OOBE: fix focus flow in MD-OOBE.

BUG=676774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
ChromeOS OOBE: fix focus flow in MD-OOBE.

BUG=676774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2688153004
Cr-Commit-Position: refs/heads/master@{#451434}
Committed:
https://chromium.googlesource.com/chromium/src/+/bc82717e957aeaf5b865d4f4b4c3...
==========

[commit-bot: I haz the power, 2017-02-18 06:56:53]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/bc82717e957aeaf5b865d4f4b4c3...