[webkit-reviews] review denied: [Bug 169947] AX: Media controls are unlabeled : [Attachment 305090] Patch

[bugzilla-daemon at webkit.org]

Antoine Quint <graouts at apple.com> has denied Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 169947: AX: Media controls are unlabeled
https://bugs.webkit.org/show_bug.cgi?id=169947

Attachment 305090: Patch

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




--- Comment #3 from Antoine Quint <graouts at apple.com> ---
Comment on attachment 305090
  --> https://bugs.webkit.org/attachment.cgi?id=305090
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.