Bug 163328 - [Modern Media Controls] Sliders: scrubber and volume
Summary: [Modern Media Controls] Sliders: scrubber and volume
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks: 163356
  Show dependency treegraph
 
Reported: 2016-10-12 05:29 PDT by Antoine Quint
Modified: 2016-10-13 00:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch (38.11 KB, patch)
2016-10-12 05:33 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2016-10-12 06:49 PDT, Build Bot
no flags Details
Patch for landing (38.96 KB, patch)
2016-10-12 14:18 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-12 05:29:28 PDT
Media controls need some sliders, specifically to change the video current time (scrubber) and the volume.
Comment 1 Radar WebKit Bug Importer 2016-10-12 05:30:06 PDT
<rdar://problem/28733838>
Comment 2 Antoine Quint 2016-10-12 05:33:31 PDT
Created attachment 291350 [details]
Patch
Comment 3 Build Bot 2016-10-12 06:49:15 PDT
Comment on attachment 291350 [details]
Patch

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

New failing tests:
media/modern-media-controls/volume-slider/volume-slider-value.html
Comment 4 Build Bot 2016-10-12 06:49:18 PDT
Created attachment 291354 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 5 Dean Jackson 2016-10-12 12:32:07 PDT
Comment on attachment 291350 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/slider.js:35
> +        this._input = new LayoutNode(`<input type="range" min="0" max="1" step="${1 / 1000}">`);

Why not just 0.001?

> Source/WebCore/Modules/modern-media-controls/controls/slider.js:37
> +        this._input.element.addEventListener("input", this);

I wonder if you're often doing someLayoutNode.element.addEventListenter(whatever, this) you should add a listenFor method to LayoutNode

listenFor(type, target) {
  this.element.addEventListener(type, target);
}

> Source/WebCore/Modules/modern-media-controls/controls/slider.js:101
> +        if (!this._valueIsChanging && this.uiDelegate && typeof this.uiDelegate.controlValueWillStartChanging === "function")

isFunction() helper?

> LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:33
> +        if (rgba.length == 4) {
> +            [r, g, b] = rgba;

Wow. Didn't know that worked.

> LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46
> +    const expectedRGBA = rgba(expectedColor);
> +    shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`);
> +    shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`);
> +    shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`);
> +    shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001);

Add a local variable.

let rgbaValue = rgba(expr);

Also, this function is for testing, and won't need to be included in the running production code. I suggest a separate media-control-test-utils.js file.

> LayoutTests/media/modern-media-controls/volume-slider/volume-slider-value.html:42
> +function dragSlider(fromPx, toPx) {

Isn't there a drag event sender?

Or am I thinking of the iOS-only touch UIScripts?
Comment 6 Antoine Quint 2016-10-12 13:18:52 PDT
(In reply to comment #5)
> Comment on attachment 291350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291350&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/slider.js:35
> > +        this._input = new LayoutNode(`<input type="range" min="0" max="1" step="${1 / 1000}">`);
> 
> Why not just 0.001?

No good reason. I'll change it.

> > Source/WebCore/Modules/modern-media-controls/controls/slider.js:37
> > +        this._input.element.addEventListener("input", this);
> 
> I wonder if you're often doing
> someLayoutNode.element.addEventListenter(whatever, this) you should add a
> listenFor method to LayoutNode
> 
> listenFor(type, target) {
>   this.element.addEventListener(type, target);
> }

I thought about that, but discarded it since it was not essential to the LayoutNode feature of grouping DOM updates.

> > Source/WebCore/Modules/modern-media-controls/controls/slider.js:101
> > +        if (!this._valueIsChanging && this.uiDelegate && typeof this.uiDelegate.controlValueWillStartChanging === "function")
> 
> isFunction() helper?

This is how we've written those things in the Web Inspector code. It's verbose but it's straightforward.

> > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:33
> > +        if (rgba.length == 4) {
> > +            [r, g, b] = rgba;
> 
> Wow. Didn't know that worked.

Yup, ES6 restructuring is the bomb.

> > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46
> > +    const expectedRGBA = rgba(expectedColor);
> > +    shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`);
> > +    shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`);
> > +    shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`);
> > +    shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001);
> 
> Add a local variable.
> 
> let rgbaValue = rgba(expr);

I don't think this can work, I think the `shouldBe()` family of functions only work when variables you refer to are in the global scope since eval() is used.

> Also, this function is for testing, and won't need to be included in the
> running production code. I suggest a separate media-control-test-utils.js
> file.

This file is in the LayoutTests directory.

> > LayoutTests/media/modern-media-controls/volume-slider/volume-slider-value.html:42
> > +function dragSlider(fromPx, toPx) {
> 
> Isn't there a drag event sender?
> 
> Or am I thinking of the iOS-only touch UIScripts?

Not that I'm aware. This was inspired by fast/form/range/range-slow-drag-to-edge.html.
Comment 7 Antoine Quint 2016-10-12 14:14:45 PDT
I've filed https://bugs.webkit.org/show_bug.cgi?id=163357 to skip the failing test on iOS.
Comment 8 Antoine Quint 2016-10-12 14:18:33 PDT
Created attachment 291395 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2016-10-12 14:52:39 PDT
Comment on attachment 291395 [details]
Patch for landing

Clearing flags on attachment: 291395

Committed r207245: <http://trac.webkit.org/changeset/207245>
Comment 10 WebKit Commit Bot 2016-10-12 14:52:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Dean Jackson 2016-10-12 16:39:55 PDT
(In reply to comment #6)

> > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46
> > > +    const expectedRGBA = rgba(expectedColor);
> > > +    shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`);
> > > +    shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`);
> > > +    shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`);
> > > +    shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001);
> > 
> > Add a local variable.
> > 
> > let rgbaValue = rgba(expr);
> 
> I don't think this can work, I think the `shouldBe()` family of functions
> only work when variables you refer to are in the global scope since eval()
> is used.

Except I think you show how to do it in the second parameter :)

shouldBe(`${rgbaValue.r}`, `${expectedRGBA.r}`)

Now you're passing a string that has a single value, but the eval should work.
Comment 12 Antoine Quint 2016-10-13 00:02:34 PDT
(In reply to comment #11)
> (In reply to comment #6)
> 
> > > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46
> > > > +    const expectedRGBA = rgba(expectedColor);
> > > > +    shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`);
> > > > +    shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`);
> > > > +    shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`);
> > > > +    shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001);
> > > 
> > > Add a local variable.
> > > 
> > > let rgbaValue = rgba(expr);
> > 
> > I don't think this can work, I think the `shouldBe()` family of functions
> > only work when variables you refer to are in the global scope since eval()
> > is used.
> 
> Except I think you show how to do it in the second parameter :)
> 
> shouldBe(`${rgbaValue.r}`, `${expectedRGBA.r}`)
> 
> Now you're passing a string that has a single value, but the eval should
> work.

Right, except the point of those `shouldBe()` functions is that they show the eval'd string in the test results, and in your example I would see the actual r value, not the expression that would generate it.