Bug 112548 – Clicking on the volume slider of HTML5 elements is pausing sometimes

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xabier Rodríguez Calvar]

Clicking on the volume slider should not pause the video but it's doing it sometimes.

[Xabier Rodríguez Calvar]

Created attachment 193584 [details]
Patch

When dealing with the volume click, the slider was not marking the event as handled.

[Jer Noble]

Comment on attachment 193584 [details]
Patch

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

Goo fix and testcase, with one nit:

> Source/WebCore/ChangeLog:12
> +        * html/shadow/MediaControlElementTypes.cpp:
> +        (WebCore::MediaControlVolumeSliderElement::defaultEventHandler):
> +        Setting setDefaultHandled to the event.

I'd like to see more description here, something along the lines of:

"Mouse click events were propagating from the volume button up to the MediaDocument, causing the video to pause. Call the event's setDefaultHandled() method to prevent the event from being handled twice."

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 193584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193584&action=review
> 
> Goo fix and testcase, with one nit:

Goo => Good.

[Eric Carlson]

Comment on attachment 193584 [details]
Patch

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

Normally, clicking on a playing <video> element will not make it pause. The only situation that I can think of where a video will pause because of a click is when it is open in a MediaDocument, where we have a click handler, so I don't think this test case will ever fail as written.

I think a better test for this change would be to have a click handler on the <video> element and fail if that fires, rather than if 'pause' fires.

> Source/WebCore/ChangeLog:3
> +        [GTK] Clicking on the volume slider of HTML5 elements is pausing sometimes

This change is not GTK-specific, please remove "[GTK]" from the title.

> LayoutTests/media/click-volume-bar-not-pausing.html:44
> +            setTimeout(finalTest(), 1000);

This makes the test take more than a full second. I a much shorter timeout should work because the 'pause' event is sent immediately, can you please see if something like 100 will work?

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> > Source/WebCore/ChangeLog:12
> > +        * html/shadow/MediaControlElementTypes.cpp:
> > +        (WebCore::MediaControlVolumeSliderElement::defaultEventHandler):
> > +        Setting setDefaultHandled to the event.
> 
> I'd like to see more description here, something along the lines of:
> 
> "Mouse click events were propagating from the volume button up to the MediaDocument, causing the video to pause. Call the event's setDefaultHandled() method to prevent the event from being handled twice."

Roger that.

(In reply to comment #4)
> Normally, clicking on a playing <video> element will not make it pause. The only situation that I can think of where a video will pause because of a click is when it is open in a MediaDocument, where we have a click handler, so I don't think this test case will ever fail as written.

Yes, I had realized that the test was not failing without the fix but I didn't know why exactly. I hope that somebody could tell me because I was thinking that maybe the testing fw had some flaw preventing that to work properly, but the answer was simpler :)

> I think a better test for this change would be to have a click handler on the <video> element and fail if that fires, rather than if 'pause' fires.

Yes, I'll try to do that.

> > Source/WebCore/ChangeLog:3
> > +        [GTK] Clicking on the volume slider of HTML5 elements is pausing sometimes
> 
> This change is not GTK-specific, please remove "[GTK]" from the title.

Done.

> > LayoutTests/media/click-volume-bar-not-pausing.html:44
> > +            setTimeout(finalTest(), 1000);
> 
> This makes the test take more than a full second. I a much shorter timeout should work because the 'pause' event is sent immediately, can you please see if something like 100 will work?

I tried 1s because I preferred to be more conservative, but I'll try to look for a shorter period.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> I think a better test for this change would be to have a click handler on the <video> element and fail if that fires, rather than if 'pause' fires.

I just tried:

-        <video controls/>
+        <video controls onclick="failTest();"/>

and the problem is that we are always getting the click as I assume it's handled to the <video> first. Any other ideas?

> > LayoutTests/media/click-volume-bar-not-pausing.html:44
> > +            setTimeout(finalTest(), 1000);
> 
> This makes the test take more than a full second. I a much shorter timeout should work because the 'pause' event is sent immediately, can you please see if something like 100 will work?

It works with 100, so I am sticking to that.

[Eric Carlson]

(In reply to comment #6)
> (In reply to comment #4)
> > I think a better test for this change would be to have a click handler on the <video> element and fail if that fires, rather than if 'pause' fires.
> 
> I just tried:
> 
> -        <video controls/>
> +        <video controls onclick="failTest();"/>
> 
> and the problem is that we are always getting the click as I assume it's handled to the <video> first. Any other ideas?
> 

Attach the handler to the <body>?

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> Attach the handler to the <body>?

Same result.

Given this, I would either let the test as it was (regarding that), which would not be testing exactly what we'd like, but at least it's another regression test or just forget about it.

[Xabier Rodríguez Calvar]

(In reply to comment #8)
> (In reply to comment #7)
> > Attach the handler to the <body>?
> 
> Same result.

It looks like the volume gets changed in mouseDown event. If add the onclick to the video element, I don't get the volume change but the effect of the javacript function I use. If I use "alert(0)" as call, the event goes thru and I see both the volume change and the alert.

I'll try to keep debugging this but if you have any idea, please let me know. I am not sure if the test worths it though.

[Xabier Rodríguez Calvar]

Premise: for the test, no more code should be needed as the bug is fixed as it is now, so forgetting about stopping propagation and those things.
Idea: I should connect a handler to something that would work in bubble phase. That was the body as the event in the video element was handled in target phase.

The idea was having a look at the event exported attributes and see if any of them changed if I apply or remove my patch. The thing is that they don't and one of the reasons might be because the event bubbles up to the body before it was passed down to the shadow dom.

I don't really see a way of testing that the bug is fixed without adding more code only for testing that would be quite nasty.

Any ideas before I suggest to leave the test as it is even if we are not testing exactly that?

[Xabier Rodríguez Calvar]

(In reply to comment #10)
> The idea was having a look at the event exported attributes and see if any of them changed if I apply or remove my patch. The thing is that they don't and one of the reasons might be because the event bubbles up to the body before it was passed down to the shadow dom.

I even tried to keep a reference to the event and check at the timeout callback, but the object is dead by that moment.

[Eric Carlson]

(In reply to comment #10)
> Premise: for the test, no more code should be needed as the bug is fixed as it is now, so forgetting about stopping propagation and those things.
> Idea: I should connect a handler to something that would work in bubble phase. That was the body as the event in the video element was handled in target phase.
> 
> The idea was having a look at the event exported attributes and see if any of them changed if I apply or remove my patch. The thing is that they don't and one of the reasons might be because the event bubbles up to the body before it was passed down to the shadow dom.
> 
> I don't really see a way of testing that the bug is fixed without adding more code only for testing that would be quite nasty.
> 
> Any ideas before I suggest to leave the test as it is even if we are not testing exactly that?

I don't understand how the changes in the patch can have any effect on the behavior of the element if it is impossible to detect in a test. Are you certain that this fixes the described behavior?

[Xabier Rodríguez Calvar]

(In reply to comment #12)
> I don't understand how the changes in the patch can have any effect on the behavior of the element if it is impossible to detect in a test. Are you certain that this fixes the described behavior?

Sorry if I didn't explain it correctly, but I meant that the patch was that very line and that fixes the bug, answering to your question. What I didn't want to do was adding anything else to the events management to be able to detect if the patch was working or not.

As you mentioned, the problem is that the bug only affected to MediaDocuments and the document I was creating was not a MediaDocument so we were trying to find a way of detecting the fix in document that didn't contain the bug and one of the problems is that I didn't know how to force the creation of a MediaDocument.I found some examples so I think it should work this time.

[Xabier Rodríguez Calvar]

Created attachment 195142 [details]
Patch

Used a MediaDocument in the test and now the test fails before the patch. I'll set the cq? in case I can watch the bots properly

[Eric Carlson]

Comment on attachment 195142 [details]
Patch

Nicely done, thanks for taking the extra time to figure out how to create this test!

[Xabier Rodríguez Calvar]

(In reply to comment #15)
> (From update of attachment 195142 [details])
> Nicely done, thanks for taking the extra time to figure out how to create this test!

You are welcome :)

It looks like mac-ews and cr-linux are not happy. I'll have a look at this in order not to break the bots.

[WebKit Review Bot]

Comment on attachment 195142 [details]
Patch

Attachment 195142 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17144899

New failing tests:
media/click-volume-bar-not-pausing.html

[WebKit Review Bot]

Created attachment 195161 [details]
Archive of layout-test-results from gce-cr-linux-08 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Build Bot]

Comment on attachment 195142 [details]
Patch

Attachment 195142 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17290726

New failing tests:
media/click-volume-bar-not-pausing.html

[Build Bot]

Created attachment 195165 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 195142 [details]
Patch

Attachment 195142 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17329161

[Xabier Rodríguez Calvar]

Created attachment 197248 [details]
Patch

I fixed the problem caused in Apple WK2 tests. Now it works in all cases.

[Eric Carlson]

Comment on attachment 197248 [details]
Patch

Thank you for keeping at this!

[Xabier Rodríguez Calvar]

(In reply to comment #23)
> (From update of attachment 197248 [details])
> Thank you for keeping at this!

You are welcome :)

[WebKit Commit Bot]

Comment on attachment 197248 [details]
Patch

Clearing flags on attachment: 197248

Committed r148131: <http://trac.webkit.org/changeset/148131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.