RESOLVED FIXED 170250
AX: Modern Media Controls Timeline slider should be operable
https://bugs.webkit.org/show_bug.cgi?id=170250
Summary AX: Modern Media Controls Timeline slider should be operable
Aaron Chu
Reported 2017-03-29 12:18:51 PDT
Attachments
Patch (10.11 KB, patch)
2017-03-29 13:07 PDT, Aaron Chu
no flags
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
Patch (10.10 KB, patch)
2017-03-29 14:57 PDT, Aaron Chu
no flags
Archive of layout-test-results from ews124 for ios-simulator-wk2 (deleted)
2017-03-29 16:24 PDT, Build Bot
no flags
Patch (12.22 KB, patch)
2017-04-17 23:20 PDT, Aaron Chu
no flags
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
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
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
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
Patch (13.92 KB, patch)
2017-04-18 11:11 PDT, Aaron Chu
no flags
Patch (12.16 KB, patch)
2017-04-19 17:25 PDT, Aaron Chu
no flags
Patch (12.30 KB, patch)
2017-04-20 12:00 PDT, Aaron Chu
no flags
Aaron Chu
Comment 1 2017-03-29 13:07:27 PDT
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Aaron Chu
Comment 4 2017-03-29 14:57:40 PDT
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Antoine Quint
Comment 7 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.
Aaron Chu
Comment 8 2017-04-17 23:20:19 PDT
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Build Bot
Comment 11 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
Build Bot
Comment 12 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
Build Bot
Comment 13 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
Build Bot
Comment 14 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
Build Bot
Comment 15 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
Build Bot
Comment 16 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
Aaron Chu
Comment 17 2017-04-18 11:11:39 PDT
Antoine Quint
Comment 18 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.
Aaron Chu
Comment 19 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.
Aaron Chu
Comment 20 2017-04-19 17:25:36 PDT
Antoine Quint
Comment 21 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.
Aaron Chu
Comment 22 2017-04-20 12:00:04 PDT
WebKit Commit Bot
Comment 23 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>
WebKit Commit Bot
Comment 24 2017-04-20 12:45:24 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.