AX: Modern Media Controls Timeline slider should be operable

[Aaron Chu]

rdar://problem/30154093

[Aaron Chu]

Created attachment 305774 [details]
Patch

[Build Bot]

Comment on attachment 305774 [details]
Patch

Attachment 305774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3435623

New failing tests:
media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html

[Build Bot]

Created attachment 305795 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Aaron Chu]

Created attachment 305798 [details]
Patch

[Build Bot]

Comment on attachment 305798 [details]
Patch

Attachment 305798 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3436201

New failing tests:
media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html

[Build Bot]

Created attachment 305805 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Antoine Quint]

Comment on attachment 305798 [details]
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.

[Aaron Chu]

Created attachment 307350 [details]
Patch

[Build Bot]

Comment on attachment 307350 [details]
Patch

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

New failing tests:
media/modern-media-controls/time-label/time-label.html

[Build Bot]

Created attachment 307354 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 307350 [details]
Patch

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

New failing tests:
media/modern-media-controls/time-label/time-label.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 307350 [details]
Patch

Attachment 307350 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3555640

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307356 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Build Bot]

Comment on attachment 307350 [details]
Patch

Attachment 307350 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3555639

New failing tests:
media/modern-media-controls/time-label/time-label.html

[Build Bot]

Created attachment 307359 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Aaron Chu]

Created attachment 307395 [details]
Patch

[Antoine Quint]

Comment on attachment 307395 [details]
Patch

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

> Source/WebCore/ChangeLog:55
> +        (formatTimeByUnit):

Make sure this double ChangeLog entry gets fixed.

> Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:85
> +    _unitizeTime(value, unit)

While it's OK as-is, this method really needn't be on the Scrubber prototype, it could just be a global function.

> Source/WebCore/Modules/modern-media-controls/controls/time-control.js:93
> +        if (this.scrubber)

Is this ever not true?

> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:59
> +            if (this._timeControl)

Is this ever not true?

> Source/WebCore/Modules/modern-media-controls/main.js:57
> +    let result = {};
> +    const time = value || 0;
> +    const absTime = Math.abs(time);
> +    result.intSeconds = Math.floor(absTime % 60).toFixed(0);
> +    result.intMinutes = Math.floor((absTime / 60) % 60).toFixed(0);
> +    result.intHours = Math.floor(absTime / (60 * 60)).toFixed(0);
> +    return result;

Probably more idiomatic to write this as:

return {
    seconds: …,
    minutes: …,
    hours: …
};

> LayoutTests/ChangeLog:20
> +

Double ChangeLog entry here as well.

[Aaron Chu]

Comment on attachment 307395 [details]
Patch

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

>> Source/WebCore/Modules/modern-media-controls/controls/time-control.js:93
>> +        if (this.scrubber)
> 
> Is this ever not true?

This is extra.

>> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:59
>> +            if (this._timeControl)
> 
> Is this ever not true?

This one is needed to pass the time-label test. The test only instantiates a time-label. It does not have a time control.

[Aaron Chu]

Created attachment 307526 [details]
Patch

[Antoine Quint]

Comment on attachment 307526 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:64
> +        this._inputAccessibleLabel = this._formatTime(timeValue);

Doesn't look like we need this ivar at all.

> Source/WebCore/Modules/modern-media-controls/controls/slider.js:81
>              this._handleInputEvent();

We should change the name of `_handleInputEvent()` now that it's called for "change" events as well. We probably want it to be called something like `_valueDidChange()`.

> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:65
> +        

Remove those extra lines.

[Aaron Chu]

Created attachment 307607 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 307607 [details]
Patch

Clearing flags on attachment: 307607

Committed r215572: <http://trac.webkit.org/changeset/215572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.