AX: Video/Audio Player Controls missing group container.

[Aaron Chu]

<rdar://problem/32008994>

[Aaron Chu]

Created attachment 309320 [details]
Patch

[James Craig]

Comment on attachment 309320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309320&action=review

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:67
> +        if (value)
> +            this.controlsBar.element.setAttribute("aria-label", UIString("Audio Player"));
> +        else
> +            this.controlsBar.element.setAttribute("aria-label", UIString("Video Controls"));
> +    }

This is the place where I think it's appropriate to use aria-roledescription instead of aria-label. Since this is WebKit UI only, VoiceOver users are going to learn it quickly, and there is little benefit to hearing the generic "group" on predictable UI. The label could (eventually) come from a user-generated attribute on the shadow root <video> element. e.g. "foobar, video controls" 

Nit: the strings should be "… Player" or "… Controls" but not a mix of both.

[James Craig]

(In reply to James Craig from comment #2)

> Nit: the strings should be "… Player" or "… Controls" but not a mix of both.

Looking back, I see those inconsistent strings came from me. ;-)

[Aaron Chu]

Created attachment 312138 [details]
Patch

[chris fleizach]

Comment on attachment 312138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312138&action=review

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:4
> +    "Audio Player": "Audio Controls",

these should be the same

[Aaron Chu]

Created attachment 312139 [details]
Patch

[Antoine Quint]

Comment on attachment 312139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312139&action=review

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:64
> +    set isAudio(value)

There already is a "shouldUseAudioLayout" property defined on InlineMediaControls, which is set from AudioSupport. We should promote it to MediaControls and augment it to perform the work you're doing here.

[Aaron Chu]

Created attachment 312283 [details]
Patch

[Antoine Quint]

Comment on attachment 312283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312283&action=review

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:71
> +            this.bottomControlsBar.element.setAttribute("aria-label", UIString("Audio Controls"));

I don't quite follow these changes. Why do we do anything when the value doesn't change? Is it to set the aria-label for the default case? If so, I think this should be handled in the constructor. The code here would fail to set the "Video Controls" string if "shouldUseAudioLayout" goes from true to false.

[Aaron Chu]

Created attachment 312318 [details]
Patch

[Build Bot]

Comment on attachment 312318 [details]
Patch

Attachment 312318 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3895159

New failing tests:
fast/mediastream/getUserMedia-grant-persistency.html

[Build Bot]

Created attachment 312329 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Antoine Quint]

Comment on attachment 312318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312318&action=review

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:69
> +            this.bottomControlsBar.element.setAttribute("aria-label", UIString("Audio Controls"));

You should update the "aria-label" regardless of the value of `flag`:

this.bottomControlsBar.element.setAttribute("aria-label", flag ? UIString("Audio Controls") : UIString("Video Controls"));

I think the easiest thing is to split this in a dedicated method like so:

_updateBottomControlsBarLabel()
{
    this.bottomControlsBar.element.setAttribute("aria-label", this._shouldUseAudioLayout ? UIString("Audio Controls") : UIString("Video Controls"));
}

and call this in the constructor and in the `shouldUseAudioLayout` setter once the ivar has been set.

[Aaron Chu]

Created attachment 312436 [details]
Patch

[Antoine Quint]

Comment on attachment 312436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312436&action=review

> Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js:245
> +    updateBottomControlsBarLabel() {

This should be a private method, please move it down to a Private block and add a `_` prefix.

[Aaron Chu]

Created attachment 312701 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 312701 [details]
Patch for landing

Clearing flags on attachment: 312701

Committed r218145: <http://trac.webkit.org/changeset/218145>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.