Bug 64314

Summary: REGRESSION(r89004): Video pauses and never resumes playing if scrubbed during playback
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe>
Component: MediaAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, hyatt, jer.noble, sam, sjl, tkent, tonikitoo
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://media.w3.org/2010/05/bunny/movie.mp4
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3 sam: review+

Description Mark Rowe (bdash) 2011-07-11 13:32:20 PDT
Steps to reproduce:
1) Load <http://media.w3.org/2010/05/bunny/movie.mp4> and let the video begin to play.
2) Click in the playback timeline to move ahead.
3) Wait for a few moments to see whether the video continues to play from that point.

Results:
Video stops playing after 2 and never resumes.  Pressing pause then play again does not unstick it.
Comment 1 Mark Rowe (bdash) 2011-07-11 13:33:51 PDT
This broke in r89004.
Comment 2 Mark Rowe (bdash) 2011-07-11 13:35:32 PDT
The video gets stuck because after r89004 MediaControlTimelineElement::defaultEventHandler no longer sees the mouse up event.  This means that endScrubbing is no longer called on the MediaElement.
Comment 3 Mark Rowe (bdash) 2011-07-11 13:37:35 PDT
For ease of clickability: This broke in <http://trac.webkit.org/changeset/89004>.
Comment 4 Mark Rowe (bdash) 2011-07-11 17:10:29 PDT
<rdar://problem/9757476>
Comment 5 Kent Tamura 2011-07-11 20:10:22 PDT
Created attachment 100432 [details]
Patch
Comment 6 Mark Rowe (bdash) 2011-07-11 20:27:37 PDT
Comment on attachment 100432 [details]
Patch

Why is mousedown different?
Comment 7 Kent Tamura 2011-07-11 20:33:00 PDT
(In reply to comment #6)
> (From update of attachment 100432 [details])
> Why is mousedown different?

setDefaultHandled() for mousedown is required to fix Bug 62683, and mousedown is unrelated to this media control issue.
Comment 8 Mark Rowe (bdash) 2011-07-11 20:35:28 PDT
I understand that changing it is not required to fix the bug.  What I’m asking is why does calling setDefaultHandled cause a problem for mousemove / mouseup but not for mousedown?  Why is there an inconsistency in behavior?
Comment 9 Kent Tamura 2011-07-11 21:13:13 PDT
(In reply to comment #8)

I think it's because SliderThumbElement starts mouse capturing since a mousedown event.
Comment 10 Jer Noble 2011-07-14 11:21:24 PDT
(In reply to comment #9)
> (In reply to comment #8)
> 
> I think it's because SliderThumbElement starts mouse capturing since a mousedown event.

MediaControlTimelineElement::defaultEventHandler() needs to receive a mouseDown event to initiate scrubbing.   Calling setDefaultHandled() in SliderThumbElement means that event will never be handled by MediaControlTimelineElement.  The setDefaultHandled() call for mouseDown must be removed as well to fix media scrubbing, so this patch is not complete.

Is there another way to fix the Qt layout test that doesn't involve calling setDefaultHandled() on mouseDown events?
Comment 11 Steve Lacey 2011-07-18 16:07:59 PDT
FYI: This problem has broken Chromium too. Any ETA? Thx :-)
Comment 12 Kent Tamura 2011-07-18 18:24:40 PDT
Jer, Steve,
Would you try my patch and tell me what's wrong please? I thought my patch fixed the bug.
Comment 13 Mark Rowe (bdash) 2011-07-18 18:31:07 PDT
A code change can fix a bug yet not be correct.
Comment 14 Steve Lacey 2011-07-18 18:46:03 PDT
The patch does fix the symptom (i.e. media timeline now works, for chromium), but I don't understand why the mouseDown case should be different?
Comment 15 Jer Noble 2011-07-18 22:52:39 PDT
(In reply to comment #12)
> Jer, Steve,
> Would you try my patch and tell me what's wrong please? I thought my patch fixed the bug.

Your patch sets defaultHandled for mouseDown events.  This means that MediaControlTimelineElement will never see the mouseDown event, and HTMLMediaElement::beginScrubbing() will never be called.

HTMLMediaElement::beginScrubbing() sets some internal flags and pauses the movie if necessary.  HTMLMediaElement::endScrubbing() checks those flags, and begins to play the movie if it was paused in beginScrubbing().

With your patch, beginScrubbing() will never get called, and endScrubbing() will.  While your patch fixes the obvious bug, it creates another less obvious one.
Comment 16 Steve Lacey 2011-07-18 23:06:27 PDT
And testing more closely, pausing/resuming after scrubbing is now buggy (with the patch).
Comment 17 Kent Tamura 2011-07-18 23:22:04 PDT
Thank you for the comments.
It seems I tested with a wrong way.  I thought I clicked on the thumb of the timeline slider but probably I clicked on the slider track near the thumb.  My patch doesn't pause the media by mouse clicking.
Comment 18 Kent Tamura 2011-07-18 23:26:50 PDT
Created attachment 101272 [details]
Patch 2
Comment 19 Jer Noble 2011-07-18 23:42:10 PDT
Comment on attachment 101272 [details]
Patch 2

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

This patch looks much better. Just one comment:

> Source/WebCore/html/RangeInputType.cpp:164
> +    // Mouse click on the thumb was already handled in SliderThumbElement::defaultEventHandler().
> +    if (targetNode == thumb)
> +        return;
>      thumb->dragFrom(event->absoluteLocation());
>      event->setDefaultHandled();

Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314?
Comment 20 Jer Noble 2011-07-18 23:44:32 PDT
(In reply to comment #19)
> Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314?

Whoops, wrong bug number.  Is that line still needed to fix bug #62683?
Comment 21 Mark Rowe (bdash) 2011-07-18 23:44:44 PDT
Comment on attachment 101272 [details]
Patch 2

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

> Source/WebCore/html/shadow/SliderThumbElement.cpp:283
> +    // We don't call event->setDefaultHandled() here intentionally.
> +    // http://webkit.org/b/64314

We typically don’t put a comment at every location that we don’t call a given function.  Is the lack of call to setDefaultHandled here so surprising that the comment is necessary?
Comment 22 Kent Tamura 2011-07-19 00:13:28 PDT
Created attachment 101282 [details]
Patch 3
Comment 23 Kent Tamura 2011-07-19 00:17:56 PDT
Comment on attachment 101272 [details]
Patch 2

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

>> Source/WebCore/html/RangeInputType.cpp:164
>>      event->setDefaultHandled();
> 
> Now that you return early here, is the "event->setDefaultHandled()" line still necessary to fix bug #64314?

It is unnecessary.  Removed.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:283
>> +    // http://webkit.org/b/64314
> 
> We typically don’t put a comment at every location that we don’t call a given function.  Is the lack of call to setDefaultHandled here so surprising that the comment is necessary?

The lack surprised me and I added setDefaultHandled() blindly in r89004.  Almost all of event handling code of HTMLInputElement and InputTypes call setDefaultHandled() if they do something for events.
Comment 24 Sam Weinig 2011-07-19 19:14:23 PDT
Comment on attachment 101282 [details]
Patch 3

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

> Source/WebCore/html/shadow/SliderThumbElement.cpp:283
> +    // We don't call event->setDefaultHandled() here intentionally.
> +    // http://webkit.org/b/64314

A better way to phrase this would be "We intentionally do not call event->setDefaultHandled() here. It would probably be of more use to explain why we don't call it rather than pointing to a bug.
Comment 25 Kent Tamura 2011-07-19 21:43:30 PDT
(In reply to comment #24)
> (From update of attachment 101282 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101282&action=review
> 
> > Source/WebCore/html/shadow/SliderThumbElement.cpp:283
> > +    // We don't call event->setDefaultHandled() here intentionally.
> > +    // http://webkit.org/b/64314
> 
> A better way to phrase this would be "We intentionally do not call event->setDefaultHandled() here. It would probably be of more use to explain why we don't call it rather than pointing to a bug.

Thanks!
I'll change the comment as follows and will land the patch.

    // We intentionally do not call event->setDefaultHandled() here because
    // MediaControlTimelineElement::defaultEventHandler() wants to handle these
    // mouse events.
Comment 26 Kent Tamura 2011-07-19 21:46:49 PDT
Committed r91333: <http://trac.webkit.org/changeset/91333>
Comment 27 Antonio Gomes 2011-10-06 07:30:55 PDT
(In reply to comment #26)
> Committed r91333: <http://trac.webkit.org/changeset/91333>

Kent, is this really the right thing to do for "input type=range" case. Now no events get handled at all on this case for range input.

More specially, getting the mouse-move set as 'defaultHandled' when draggin the slider is specially usefull for touch-based input devices, if they want to make this slider draggable.
Comment 28 Kent Tamura 2011-10-11 01:44:41 PDT
(In reply to comment #27)
> (In reply to comment #26)
> > Committed r91333: <http://trac.webkit.org/changeset/91333>
> 
> Kent, is this really the right thing to do for "input type=range" case. Now no events get handled at all on this case for range input.
> 
> More specially, getting the mouse-move set as 'defaultHandled' when draggin the slider is specially usefull for touch-based input devices, if they want to make this slider draggable.

I think the current behavior is same as the behavior before r89004.
Would you explain the detail of the use-case in another bug please?