Summary: | [Modern Media Controls] Sliders: scrubber and volume | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
Component: | Media | Assignee: | Antoine Quint <graouts> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dino, graouts, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari 10 | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 163356 | ||||||||||
Attachments: |
|
Description
Antoine Quint
2016-10-12 05:29:28 PDT
Created attachment 291350 [details]
Patch
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 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 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? (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. I've filed https://bugs.webkit.org/show_bug.cgi?id=163357 to skip the failing test on iOS. Created attachment 291395 [details]
Patch for landing
Comment on attachment 291395 [details] Patch for landing Clearing flags on attachment: 291395 Committed r207245: <http://trac.webkit.org/changeset/207245> All reviewed patches have been landed. Closing bug. (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. (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. |