Bug 170250 - AX: Modern Media Controls Timeline slider should be operable
Summary: AX: Modern Media Controls Timeline slider should be operable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari 10
Hardware: All All
: P2 Normal
Assignee: Aaron Chu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-29 12:18 PDT by Aaron Chu
Modified: 2017-04-20 12:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.11 KB, patch)
2017-03-29 13:07 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (14.68 MB, application/zip)
2017-03-29 14:41 PDT, Build Bot
no flags Details
Patch (10.10 KB, patch)
2017-03-29 14:57 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (deleted)
2017-03-29 16:24 PDT, Build Bot
no flags Details
Patch (12.22 KB, patch)
2017-04-17 23:20 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.09 MB, application/zip)
2017-04-18 00:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.27 MB, application/zip)
2017-04-18 00:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.67 MB, application/zip)
2017-04-18 00:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (882.55 KB, application/zip)
2017-04-18 01:00 PDT, Build Bot
no flags Details
Patch (13.92 KB, patch)
2017-04-18 11:11 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2017-04-19 17:25 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2017-04-20 12:00 PDT, Aaron Chu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Chu 2017-03-29 12:18:51 PDT
rdar://problem/30154093
Comment 1 Aaron Chu 2017-03-29 13:07:27 PDT
Created attachment 305774 [details]
Patch
Comment 2 Build Bot 2017-03-29 14:41:17 PDT
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
Comment 3 Build Bot 2017-03-29 14:41:19 PDT
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
Comment 4 Aaron Chu 2017-03-29 14:57:40 PDT
Created attachment 305798 [details]
Patch
Comment 5 Build Bot 2017-03-29 16:24:28 PDT
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
Comment 6 Build Bot 2017-03-29 16:24:30 PDT
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
Comment 7 Antoine Quint 2017-03-30 01:51:18 PDT
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.
Comment 8 Aaron Chu 2017-04-17 23:20:19 PDT
Created attachment 307350 [details]
Patch
Comment 9 Build Bot 2017-04-18 00:08:04 PDT
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
Comment 10 Build Bot 2017-04-18 00:08:05 PDT
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
Comment 11 Build Bot 2017-04-18 00:20:31 PDT
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
Comment 12 Build Bot 2017-04-18 00:20:32 PDT
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
Comment 13 Build Bot 2017-04-18 00:52:08 PDT
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
Comment 14 Build Bot 2017-04-18 00:52:09 PDT
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
Comment 15 Build Bot 2017-04-18 01:00:26 PDT
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
Comment 16 Build Bot 2017-04-18 01:00:28 PDT
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
Comment 17 Aaron Chu 2017-04-18 11:11:39 PDT
Created attachment 307395 [details]
Patch
Comment 18 Antoine Quint 2017-04-19 06:30:32 PDT
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.
Comment 19 Aaron Chu 2017-04-19 17:17:50 PDT
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.
Comment 20 Aaron Chu 2017-04-19 17:25:36 PDT
Created attachment 307526 [details]
Patch
Comment 21 Antoine Quint 2017-04-20 01:40:51 PDT
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.
Comment 22 Aaron Chu 2017-04-20 12:00:04 PDT
Created attachment 307607 [details]
Patch
Comment 23 WebKit Commit Bot 2017-04-20 12:45:22 PDT
Comment on attachment 307607 [details]
Patch

Clearing flags on attachment: 307607

Committed r215572: <http://trac.webkit.org/changeset/215572>
Comment 24 WebKit Commit Bot 2017-04-20 12:45:24 PDT
All reviewed patches have been landed.  Closing bug.