Summary: | [EFL][GTK] Volume slider only changes volume when thumb is released, not while dragging | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hunseop Jeong <hs85.jeong> | ||||||
Component: | WebKit EFL | Assignee: | Hunseop Jeong <hs85.jeong> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | calvaris, commit-queue, gyuyoung.kim, lucas.de.marchi | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Hunseop Jeong
2016-04-24 23:13:06 PDT
'mediaControlsApple.js' already fixed bug in https://bugs.webkit.org/attachment.cgi?id=240041. Created attachment 277224 [details]
Patch
(In reply to comment #0) > Some media tests didn't invoke the 'volumechange' event when dragging the > volume slider in built-in media controls. Could you please let us know which are the affected tests? (In reply to comment #3) > (In reply to comment #0) > > Some media tests didn't invoke the 'volumechange' event when dragging the > > volume slider in built-in media controls. > > Could you please let us know which are the affected tests? Not some, only media/video-volume-slider-drag.html. (In reply to comment #4) > Not some, only media/video-volume-slider-drag.html. Ok, let me do some tests and then I'll review. Comment on attachment 277224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277224&action=review > Source/WebCore/ChangeLog:3 > + [EFL] Volume slider only changes volume when thumb is released, not while dragging GTK is affected too so you should mark [GTK] as well. > Source/WebCore/ChangeLog:23 > + * Modules/mediacontrols/mediaControlsBase.js: > + (Controller.prototype.createControls): > + (Controller.prototype.handlePlayButtonClicked): > + (Controller.prototype.handleTimelineInput): > + (Controller.prototype.handleTimelineChange): > + (Controller.prototype.handleTimelineDown): > + (Controller.prototype.handleTimelineMouseUp): > + (Controller.prototype.handleMuteButtonClicked): > + (Controller.prototype.handleMaxButtonClicked): > + (Controller.prototype.handleVolumeSliderInput): > + (Controller.prototype.handleVolumeSliderChange): Deleted. We would need a more complete general explanation or explain the details here. > Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:343 > - this.listenFor(timeline, 'input', this.handleTimelineChange); > + this.listenFor(timeline, 'input', this.handleTimelineInput); > + this.listenFor(timeline, 'change', this.handleTimelineChange); These changes seem unrelated to volume. Why are they here? > Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:737 > - handleTimelineChange: function(event) > + handleTimelineInput: function(event) > { > this.video.fastSeek(this.controls.timeline.value); > }, > > + handleTimelineChange: function(event) > + { > + this.video.currentTime = this.controls.timeline.value; > + }, > + Ditto. > Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:-793 > - > - // Do a precise seek when we lift the mouse: > - this.video.currentTime = this.controls.timeline.value; Ditto Created attachment 277438 [details]
Patch
Comment on attachment 277224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277224&action=review >> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:343 >> + this.listenFor(timeline, 'change', this.handleTimelineChange); > > These changes seem unrelated to volume. Why are they here? You are right. That is related with timeline. I just moved the codes(https://bugs.webkit.org/show_bug.cgi?id=137805). Comment on attachment 277438 [details]
Patch
Good that you removed the failing expectations too. Nice!
Comment on attachment 277438 [details]
Patch
Thanks for the review.
Comment on attachment 277438 [details] Patch Clearing flags on attachment: 277438 Committed r200126: <http://trac.webkit.org/changeset/200126> All reviewed patches have been landed. Closing bug. |