Fix accessibility issues in AudioPlayer and VideoPlayer.

Fix accessibility issues in AudioPlayer and VideoPlayer.

This CL includes:
- Make each control focusable.
- Implement blue outline for focused-by-keyboard buttons.
- Add aria-label for buttons and sliders.
- Extracted common behaviors of icon buttons as files-icon-button.

BUG=551267, 533604
TEST=manually tested using ChromeVox.

Committed: https://crrev.com/11e80965386a3488f6ef9d4bb94722a4f5ba83f9
Cr-Commit-Position: refs/heads/master@{#359041}

[fukino, 2015-11-05 18:54:22]

Description was changed from

==========
Fix accessibility issues in AudioPlayer and VideoPlayer.

BUG=
==========

to

==========
Fix accessibility issues in AudioPlayer and VideoPlayer.

This CL includes:
- Make each control focusable.
- Implement blue outline for focused-by-keyboard buttons.
- Add aria-label for buttons and sliders.
- Extracted common behaviors of icon buttons as files-icon-button.

BUG=551267,533604
TEST=manually tested using ChromeVox.
==========

[fukino, 2015-11-05 18:55:21]

fukino@chromium.org changed reviewers:
+ yawano@chromium.org

[fukino, 2015-11-05 18:55:21]

yawano@, could you take a look?
Sorry for including multiple fixes in one CL.
Please let me know if you want separated CLs.

[yawano, 2015-11-06 05:38:37]

Thank you! I really like files-icon-button. It makes the code much simpler and
clean.

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/elements/control_panel.css (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.css:131: background-image:
-webkit-image-set(
If you set icon as a background image of files-icon-button, icon will go behind
ripple effect. Is this intentional?

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/elements/control_panel.js (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.js:176: ariaLabelsChanged_:
function(ariaLabels) {
small nit: assert(ariaLabels)?

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.js:178:
this.$.volumeSlider.setAttribute('aria-label', ariaLabels.volumeSlider);
Is it difficult to use i18nTemplate here as we do in File Manager and Gallery? -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga...

Using i18n-values properties seem cleaner for me.

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/video_playe...
File ui/file_manager/video_player/js/media_controls.js (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/video_playe...
ui/file_manager/video_player/js/media_controls.js:18: function
MediaControls(containerElement, onMediaError, stringFunction) {
Is it difficult to use strf global function in video player? -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...

In File Manager and Gallery, we are migrating from passing string function to
use strf global function.

[fukino, 2015-11-06 10:13:21]

Thank you!

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/elements/control_panel.css (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.css:131: background-image:
-webkit-image-set(
On 2015/11/06 05:38:37, yawano wrote:
> If you set icon as a background image of files-icon-button, icon will go
behind
> ripple effect. Is this intentional?

It is intentional, since it is same as paper-icon-button.
However, it might be better to make it possible to show icon on top of ripple
when we use this element for Files app.
Let me leave it as is for now. I added a TODO comment.

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/elements/control_panel.js (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.js:176: ariaLabelsChanged_:
function(ariaLabels) {
On 2015/11/06 05:38:37, yawano wrote:
> small nit: assert(ariaLabels)?

Done.

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/elements/control_panel.js:178:
this.$.volumeSlider.setAttribute('aria-label', ariaLabels.volumeSlider);
On 2015/11/06 05:38:37, yawano wrote:
> Is it difficult to use i18nTemplate here as we do in File Manager and Gallery?
-
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga...
> 
> Using i18n-values properties seem cleaner for me.

I pass translated labels to the custom element because I don't want to make the
element dependent with VideoPlayer or cr.ui library.

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/video_playe...
File ui/file_manager/video_player/js/media_controls.js (right):

https://codereview.chromium.org/1415953006/diff/1/ui/file_manager/video_playe...
ui/file_manager/video_player/js/media_controls.js:18: function
MediaControls(containerElement, onMediaError, stringFunction) {
On 2015/11/06 05:38:37, yawano wrote:
> Is it difficult to use strf global function in video player? -
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...
> 
> In File Manager and Gallery, we are migrating from passing string function to
> use strf global function.

Yes, str/strf looks better and succinct.
I upddated the CL to use str().

[yawano, 2015-11-06 10:22:26]

lgtm. Thank you!

[fukino, 2015-11-06 10:53:08]

On 2015/11/06 10:22:26, yawano wrote:
> lgtm. Thank you!

Thank you!

[fukino, 2015-11-06 10:53:12]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2015-11-06 10:53:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415953006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415953006/20001

[commit-bot: I haz the power, 2015-11-06 11:03:13]

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

[commit-bot: I haz the power, 2015-11-06 11:03:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fukino, 2015-11-10 01:57:36]

Awano-san, I fixed browser tests. Could you take another look?

[yawano, 2015-11-10 04:08:50]

still lgtm. Thank you!

[fukino, 2015-11-10 04:10:02]

fukino@chromium.org changed reviewers:
+ mtomasz@chromium.org

[fukino, 2015-11-10 04:10:03]

Tomasz, could you take a look at
chrome/browser/chromeos/extensions/file_manager/private_api_strings.cc?

[fukino, 2015-11-11 02:29:52]

Friendly ping. > Tomasz

[mtomasz, 2015-11-11 02:32:23]

lgtm. sorry for late, missed it!

[fukino, 2015-11-11 03:38:35]

On 2015/11/11 02:32:23, mtomasz wrote:
> lgtm. sorry for late, missed it!

Thank you!

[fukino, 2015-11-11 03:38:43]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2015-11-11 03:39:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415953006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415953006/60001

[commit-bot: I haz the power, 2015-11-11 04:05:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-11 04:06:29]

Patchset 4 (id:??) landed as
https://crrev.com/11e80965386a3488f6ef9d4bb94722a4f5ba83f9
Cr-Commit-Position: refs/heads/master@{#359041}

[fukino, 2015-11-12 08:15:49]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1441603002/ by fukino@chromium.org.

The reason for reverting is: We decided to punt redesign of audio player to
M49..