Bug 169947 – AX: Media controls are unlabeled

• Component: Accessibility
• Status: RESOLVED FIXED

[Aaron Chu]

<rdar://problem/30153323>

[Aaron Chu]

Created attachment 305090 [details]
Patch

[James Craig]

Comment on attachment 305090 [details]
Patch

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

Looks great overall. Minor comments included. Please get the r+ from Antoine since he wrote most of this.

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:3
> +    "AirPlay Placard": "AirPlay Placard",

The term "placard" may be redundant in end-user UI. Should  this be just "AirPlay"?

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6
> +    "Enter Fullscreen": "Enter Fullscreen",
> +    "Enter Picture-in-Picture": "Enter Picture-in-picture",

Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix.

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:9
> +    "Invalid Placard": "Invalid Placard",

remove "placard" if possible

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:111
> +        this.element.setAttribute("aria-label", this.label);

Is there ever a time when this could set an empty label? If so, this could be:

if (this.label)
  this.element.setAttribute("aria-label", this.label);

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:112
> +        debugger;

leftover

> LayoutTests/media/modern-media-controls/mute-button/mute-button-expected.txt:10
> +PASS muteButton.label is "Volume"

Seems like this should be "Mute"

> LayoutTests/media/modern-media-controls/mute-button/mute-button-expected.txt:14
> +PASS mutedButton.label is "Volume Mute"

And this "Unmute"

[Antoine Quint]

Comment on attachment 305090 [details]
Patch

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

Overall comment: I think we shouldn't introduce a "label" property for IconButton and instead use the single "iconName" property to set the "aria-label" attribute since the icon name and labels are coupled right now. This should also make for a smaller source change.

> Source/WebCore/ChangeLog:11
> +        modern media constrols.

Typo: "controls".

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:5
> +    "Enter Fullscreen": "Enter Fullscreen",

I think it should be "Full Screen".

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6
> +    "Enter Picture-in-Picture": "Enter Picture-in-picture",

No hyphenation, should be "Enter Picture in Picture".

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:15
> +    "Picture-in-Picture Placard": "Picture-in-Picture Placard",

No hyphenation.

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:74
> +        this._label = label;

I think we should the attribute here directly, no need to do this in `layout()` since this is not connected to rendering… although, since the icon name and labels seem coupled, could we do without a distinct `label` altogether and just get the label for the icon name?

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:111
> +        this.element.setAttribute("aria-label", this.label);

Per the previous comment, this belongs in the `label` setter I reckon.

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:112
> +        debugger;

Definitely remove that :)

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:46
> +    VolumeUp        : {name: "volume-up", label: UIString("Volume Up")}

Per WebKit style (I think!) there should be a space after the opening brace and before the closing brace:

{ name: "volume-up", label: UIString("Volume Up") }

> Source/WebCore/Modules/modern-media-controls/js-files:2
> +main.js

Why was this needed?

> LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:14
> +PASS airplayButton._image.complete is false

This shouldn't change, this test is currently broken and expected to be per TestExpectations.

> LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:-29
> -FAIL airplayButton._image.complete should be false. Was true.

This shouldn't change, this test is currently broken and expected to be per TestExpectations.

> LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:49
> +PASS airplayButton.element.getAttribute('aria-label') is "AirPlay"

That's correct though.

[Aaron Chu]

Comment on attachment 305090 [details]
Patch

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

>>> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6
>>> +    "Enter Picture-in-Picture": "Enter Picture-in-picture",
>> 
>> Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix.
> 
> No hyphenation, should be "Enter Picture in Picture".

I think "Enter" is needed at least for full screen because there is a an Exit burrow in the controls when it is in full screen mode.

>> Source/WebCore/Modules/modern-media-controls/js-files:2
>> +main.js
> 
> Why was this needed?

the main.js provided the UIString method, I moved it up to the top because now that we are using UIString to pull the loc strings, the main.js becomes necessary before all other files, the "modern-media-controls-localized-strings.js" was meant to be a symlink, but it seems like it is pulled in as a new file, but I took a second pass, the loc file is not needed. I will remove in next patch.

[Aaron Chu]

Comment on attachment 305090 [details]
Patch

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

>>>> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6
>>>> +    "Enter Picture-in-Picture": "Enter Picture-in-picture",
>>> 
>>> Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix.
>> 
>> No hyphenation, should be "Enter Picture in Picture".
> 
> I think "Enter" is needed at least for full screen because there is a an Exit burrow in the controls when it is in full screen mode.

Exit button*

[Aaron Chu]

Created attachment 305223 [details]
Patch

[chris fleizach]

Comment on attachment 305223 [details]
Patch

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

> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:28
> +    "Volume Mute": "Volume Mute",

volume mute sounds strange to me. should this just be "Mute"

[Aaron Chu]

Created attachment 305239 [details]
Patch

[Antoine Quint]

Comment on attachment 305239 [details]
Patch

Looks good! While this could land as-is, I think we might want to refactor imageForIconNameAndLayoutTraits() into imageForIconAndLayoutTraits() and pass the icon and directly and let that function look up its name instead of explicitly passing it around.

[Aaron Chu]

https://bugs.webkit.org/show_bug.cgi?id=170047

[WebKit Commit Bot]

Comment on attachment 305239 [details]
Patch

Clearing flags on attachment: 305239

Committed r214400: <http://trac.webkit.org/changeset/214400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.