WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
rdar://problem/30154093
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Aaron Chu
Comment 1
2017-03-29 13:07:27 PDT
Created
attachment 305774
[details]
Patch
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
Created
attachment 305798
[details]
Patch
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
Created
attachment 307350
[details]
Patch
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
Created
attachment 307395
[details]
Patch
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
Created
attachment 307526
[details]
Patch
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
Created
attachment 307607
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug