Bug 163328 – [Modern Media Controls] Sliders: scrubber and volume

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

Media controls need some sliders, specifically to change the video current time (scrubber) and the volume.

[Radar WebKit Bug Importer]

<rdar://problem/28733838>

[Antoine Quint]

Created attachment 291350 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

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?

[Antoine Quint]

(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.

[Antoine Quint]

I've filed https://bugs.webkit.org/show_bug.cgi?id=163357 to skip the failing test on iOS.

[Antoine Quint]

Created attachment 291395 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291395 [details]
Patch for landing

Clearing flags on attachment: 291395

Committed r207245: <http://trac.webkit.org/changeset/207245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

(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.

[Antoine Quint]

(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.