[webkit-reviews] review denied: [Bug 170250] AX: Modern Media Controls Timeline slider should be operable : [Attachment 305798] 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 170250: AX: Modern Media Controls Timeline slider should be operable
https://bugs.webkit.org/show_bug.cgi?id=170250

Attachment 305798: Patch

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




--- Comment #7 from Antoine Quint <graouts at apple.com> ---
Comment on attachment 305798
  --> https://bugs.webkit.org/attachment.cgi?id=305798
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:61
> +    setAccessibleLabelForInput(label)

We typically use setters, so I think this would be `set
inputAccessibleLabel(label)`.

> Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:73
> +    _formatTime(timeInSeconds)

We already have a _formattedTime() method in TimeLabel. Ideally, we could
refactor this code to be useful to both TimeLabel and Scrubber. A global
utility in main.js would be fine.

> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:63
> +	       this._timeControl.updateScrubberLabel();

Code in `commitProperty` should specifically reflect the value of a given
property in the DOM. I think you want to change this when `value` has changed,
correct? If so, we should move this under the `if (propertyName === "value")`
clause.

>
LayoutTests/media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.
html:32
> +media.addEventListener("canplay", () => {

I'm not entirely sure, but I think you won't be getting this event on iOS by
default because the "preload" attribute is to "metadata", so no actual playable
data is downloaded until there is user action. This is my guess as to why this
test is timing out on the iOS bot.