Bug 64314 – REGRESSION(r89004): Video pauses and never resumes playing if scrubbed during playback

• Component: Media
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

This broke in r89004.

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

For ease of clickability: This broke in <http://trac.webkit.org/changeset/89004>.

[Mark Rowe (bdash)]

<rdar://problem/9757476>

[Kent Tamura]

Created attachment 100432 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 100432 [details]
Patch

Why is mousedown different?

[Kent Tamura]

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

[Mark Rowe (bdash)]

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?

[Kent Tamura]

(In reply to comment #8)

I think it's because SliderThumbElement starts mouse capturing since a mousedown event.

[Jer Noble]

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

[Steve Lacey]

FYI: This problem has broken Chromium too. Any ETA? Thx :-)

[Kent Tamura]

Jer, Steve,
Would you try my patch and tell me what's wrong please? I thought my patch fixed the bug.

[Mark Rowe (bdash)]

A code change can fix a bug yet not be correct.

[Steve Lacey]

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?

[Jer Noble]

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

[Steve Lacey]

And testing more closely, pausing/resuming after scrubbing is now buggy (with the patch).

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 101272 [details]
Patch 2

[Jer Noble]

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?

[Jer Noble]

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

[Mark Rowe (bdash)]

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?

[Kent Tamura]

Created attachment 101282 [details]
Patch 3

[Kent Tamura]

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.

[Sam Weinig]

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.

[Kent Tamura]

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

[Kent Tamura]

Committed r91333: <http://trac.webkit.org/changeset/91333>

[Antonio Gomes]

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

[Kent Tamura]

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