Clicking on the volume slider should not pause the video but it's doing it sometimes.
Created attachment 193584 [details] Patch When dealing with the volume click, the slider was not marking the event as handled.
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."
(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.
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?
(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.
(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.
(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>?
(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.
(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.
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?
(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.
(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?
(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.
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
Comment on attachment 195142 [details] Patch Nicely done, thanks for taking the extra time to figure out how to create this test!
(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.
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
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
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
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
Comment on attachment 195142 [details] Patch Attachment 195142 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17329161
Created attachment 197248 [details] Patch I fixed the problem caused in Apple WK2 tests. Now it works in all cases.
Comment on attachment 197248 [details] Patch Thank you for keeping at this!
(In reply to comment #23) > (From update of attachment 197248 [details]) > Thank you for keeping at this! You are welcome :)
Comment on attachment 197248 [details] Patch Clearing flags on attachment: 197248 Committed r148131: <http://trac.webkit.org/changeset/148131>
All reviewed patches have been landed. Closing bug.