LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html - Issue 453493002: Improve detection of touch events when hiding media controls.

Side by Side Diff: LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html

Issue 453493002: Improve detection of touch events when hiding media controls. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@autoHideControls

Patch Set: Added multimodal tests and NotFromInput synthetic type. Created 6 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« LayoutTests/media/video-controls-hide-after-touch-on-control.html ('K') | « LayoutTests/media/video-controls-visibility-multimodal-mouse-after-touch-expected.txt ('k') | LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse-expected.txt » ('j') | Source/core/events/MouseRelatedEvent.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <html>	2 <html>

3 <style>	3 <style>

4 #no-video-media {	4 #no-video-media {

5 width: 320px;	5 width: 320px;

6 height: 240px;	6 height: 240px;

7 }	7 }

8 </style>	8 </style>

9 <script src=video-test.js></script>	9 <script src=video-test.js></script>

10 <script src=media-file.js></script>	10 <script src=media-file.js></script>

11 <script src=media-controls.js></script>	11 <script src=media-controls.js></script>

12 <script>	12 <script>

13 var controls;	13 var controls;

14	14

15 function runTest()	15 function runTest()

16 {	16 {

17 video = document.getElementById("no-video-media");	17 video = document.getElementById("no-video-media");

18	18

19 testExpected("video.paused", true);	19 testExpected("video.paused", true);

20 if (!window.testRunner)	20 if (!window.testRunner)

21 return;	21 return;

22	22

23 // Click the play button.	23 // Hover the control with the mouse.

24 var playCoords = mediaControlsButtonCoordinates(video, "play-button");	24 var coords = mediaControlsButtonCoordinates(video, "play-button");

25 var clickX = playCoords[0];	25 eventSender.mouseMoveTo(coords[0], coords[1]);

26 var clickY = playCoords[1];	26

27 eventSender.mouseMoveTo(clickX, clickY);	27 // And then tap (touch input) the play button.

28 eventSender.mouseDown();	28 eventSender.gestureTap(coords[0], coords[1]);

29 eventSender.mouseUp();

30 testExpected("video.paused", false);	29 testExpected("video.paused", false);

31	30

	31 // And the controls should hide after a timeout.
	Ignacio Solla 2014/08/21 17:42:31 BTW, this is the test that failed before I added N BTW, this is the test that failed before I added NotFromInput. This is why: 1. The timer to hide the controls fires 2. The controls become hidden 3. The layout changes and blink generates fake mouse events 4. MediaControls receives the fake mouse events and makes the controls visible again The solution is to detect that the events are fake at 4, and do not make them visible. Rick Byers 2014/08/22 18:11:13 Nice, good catch! Do you really never want to con Show quoted text On 2014/08/21 17:42:31, Ignacio Solla wrote: > BTW, this is the test that failed before I added NotFromInput. > > This is why: > 1. The timer to hide the controls fires > 2. The controls become hidden > 3. The layout changes and blink generates fake mouse events > 4. MediaControls receives the fake mouse events and makes the controls visible > again > > The solution is to detect that the events are fake at 4, and do not make them > visible. Nice, good catch! Do you really never want to consider these timer-based events though? Eg. the user moving their mouse over the control area should show the controls. What if (on a desktop with just a mouse) the user drags the window such that the control area is now under the mouse cursor? Or what if the page layout changes and moves the video so it's under the mouse cursor? In general we treat relative movement between the content and mouse cursor as the same thing - regardless of which is actually moving in an absolute sense. But maybe it makes sense in this case to care only about physical mouse input - I can see an argument for that. In the real world you shouldn't actually hit this case because when the user does a tap on the touchscreen we hide the mouse cursor, which will later result in an early-out from EventHandler::fakeMouseMoveEventTimerFired due to the isCursorVisible check. So another option is to just update your test to cause isCursorVisible to be set false after the tap. Ignacio Solla 2014/08/26 13:40:53 Done. I have added the internals.setCursorVisible Show quoted text On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote: > On 2014/08/21 17:42:31, Ignacio Solla wrote: > > BTW, this is the test that failed before I added NotFromInput. > > > > This is why: > > 1. The timer to hide the controls fires > > 2. The controls become hidden > > 3. The layout changes and blink generates fake mouse events > > 4. MediaControls receives the fake mouse events and makes the controls visible > > again > > > > The solution is to detect that the events are fake at 4, and do not make them > > visible. > > Nice, good catch! > > Do you really never want to consider these timer-based events though? Eg. the > user moving their mouse over the control area should show the controls. What if > (on a desktop with just a mouse) the user drags the window such that the control > area is now under the mouse cursor? Or what if the page layout changes and > moves the video so it's under the mouse cursor? > > In general we treat relative movement between the content and mouse cursor as > the same thing - regardless of which is actually moving in an absolute sense. > > But maybe it makes sense in this case to care only about physical mouse input - > I can see an argument for that. > > In the real world you shouldn't actually hit this case because when the user > does a tap on the touchscreen we hide the mouse cursor, which will later result > in an early-out from EventHandler::fakeMouseMoveEventTimerFired due to the > isCursorVisible check. > > So another option is to just update your test to cause isCursorVisible to be set > false after the tap. Done. I have added the internals.setCursorVisible to false now and removed the NotFromInput synthetic value. In real life (I have tested with Android) I can see that the cursor gets hidden after a tap. Do you know why we need to hide manually in the test? Is it because the test eventSender executes a slightly different path? Ignacio Solla 2014/08/26 13:40:53 Acknowledged. Show quoted text On 2014/08/21 17:42:31, Ignacio Solla wrote: > BTW, this is the test that failed before I added NotFromInput. > > This is why: > 1. The timer to hide the controls fires > 2. The controls become hidden > 3. The layout changes and blink generates fake mouse events > 4. MediaControls receives the fake mouse events and makes the controls visible > again > > The solution is to detect that the events are fake at 4, and do not make them > visible. Acknowledged. Rick Byers 2014/08/26 15:25:46 Yeah, LayoutTests are really designed just test bl Show quoted text On 2014/08/26 13:40:53, Ignacio Solla wrote: > On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote: > > On 2014/08/21 17:42:31, Ignacio Solla wrote: > > > BTW, this is the test that failed before I added NotFromInput. > > > > > > This is why: > > > 1. The timer to hide the controls fires > > > 2. The controls become hidden > > > 3. The layout changes and blink generates fake mouse events > > > 4. MediaControls receives the fake mouse events and makes the controls > visible > > > again > > > > > > The solution is to detect that the events are fake at 4, and do not make > them > > > visible. > > > > Nice, good catch! > > > > Do you really never want to consider these timer-based events though? Eg. the > > user moving their mouse over the control area should show the controls. What > if > > (on a desktop with just a mouse) the user drags the window such that the > control > > area is now under the mouse cursor? Or what if the page layout changes and > > moves the video so it's under the mouse cursor? > > > > In general we treat relative movement between the content and mouse cursor as > > the same thing - regardless of which is actually moving in an absolute sense. > > > > But maybe it makes sense in this case to care only about physical mouse input > - > > I can see an argument for that. > > > > In the real world you shouldn't actually hit this case because when the user > > does a tap on the touchscreen we hide the mouse cursor, which will later > result > > in an early-out from EventHandler::fakeMouseMoveEventTimerFired due to the > > isCursorVisible check. > > > > So another option is to just update your test to cause isCursorVisible to be > set > > false after the tap. > > Done. > > I have added the internals.setCursorVisible to false now and removed the > NotFromInput synthetic value. In real life (I have tested with Android) I can > see that the cursor gets hidden after a tap. Do you know why we need to hide > manually in the test? Is it because the test eventSender executes a slightly > different path? Yeah, LayoutTests are really designed just test blink, so eventSender just injects events into blink. That logic lives in chromium. To test that end-to-end you'd have to write a browsertest (and cross blink/browser async behavior is notoriously difficult to reliably test with browser tests). By the way, I just did a quick search and it looks to me like only Aura informs blink of the cursor visibility (that's the only call I see to RenderWidgetHostImpl::SendCursorVisibilityState). So and edge case probably remains on Android here. Feel free to file a bug to me for getting SendCusrorVisibilityState implemented on Android (mouse scenarios on Android aren't well covered). Ignacio Solla 2014/08/26 17:30:42 I had manually tested with Android and a keyboard, Show quoted text On 2014/08/26 15:25:46, Rick Byers wrote: > On 2014/08/26 13:40:53, Ignacio Solla wrote: > > On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote: > > > On 2014/08/21 17:42:31, Ignacio Solla wrote: > > > > BTW, this is the test that failed before I added NotFromInput. > > > > > > > > This is why: > > > > 1. The timer to hide the controls fires > > > > 2. The controls become hidden > > > > 3. The layout changes and blink generates fake mouse events > > > > 4. MediaControls receives the fake mouse events and makes the controls > > visible > > > > again > > > > > > > > The solution is to detect that the events are fake at 4, and do not make > > them > > > > visible. > > > > > > Nice, good catch! > > > > > > Do you really never want to consider these timer-based events though? Eg. > the > > > user moving their mouse over the control area should show the controls. > What > > if > > > (on a desktop with just a mouse) the user drags the window such that the > > control > > > area is now under the mouse cursor? Or what if the page layout changes and > > > moves the video so it's under the mouse cursor? > > > > > > In general we treat relative movement between the content and mouse cursor > as > > > the same thing - regardless of which is actually moving in an absolute > sense. > > > > > > But maybe it makes sense in this case to care only about physical mouse > input > > - > > > I can see an argument for that. > > > > > > In the real world you shouldn't actually hit this case because when the user > > > does a tap on the touchscreen we hide the mouse cursor, which will later > > result > > > in an early-out from EventHandler::fakeMouseMoveEventTimerFired due to the > > > isCursorVisible check. > > > > > > So another option is to just update your test to cause isCursorVisible to be > > set > > > false after the tap. > > > > Done. > > > > I have added the internals.setCursorVisible to false now and removed the > > NotFromInput synthetic value. In real life (I have tested with Android) I can > > see that the cursor gets hidden after a tap. Do you know why we need to hide > > manually in the test? Is it because the test eventSender executes a slightly > > different path? > > Yeah, LayoutTests are really designed just test blink, so eventSender just > injects events into blink. That logic lives in chromium. To test that > end-to-end you'd have to write a browsertest (and cross blink/browser async > behavior is notoriously difficult to reliably test with browser tests). > > By the way, I just did a quick search and it looks to me like only Aura informs > blink of the cursor visibility (that's the only call I see to > RenderWidgetHostImpl::SendCursorVisibilityState). So and edge case probably > remains on Android here. Feel free to file a bug to me for getting > SendCusrorVisibilityState implemented on Android (mouse scenarios on Android > aren't well covered). I had manually tested with Android and a keyboard, and the cursor does also become invisible after touch. I'm not sure what the code path is though.
32 runAfterHideMediaControlsTimerFired(function()	32 runAfterHideMediaControlsTimerFired(function()

33 {	33 {

34 controls = mediaControlsButton(video, "panel");	34 controls = mediaControlsButton(video, "panel");

35 testExpected("getComputedStyle(controls).opacity", 0);	35 testExpected("getComputedStyle(controls).opacity", 0);

36

37 consoleWrite("");

38 endTest();	36 endTest();

39 }, video);	37 }, video);

	38

40 }	39 }

41 </script>	40 </script>

42 <body>	41 <body>

43 <p>Test video element control visibility after touch on control. After a del ay the	42 <p>Test video element control visibility with multimodal input. The controls

44 controls must be hidden.</p>	43 should hide after a timeout if the last input event was a tap.</p>

45 <p>This test only runs in DRT!</p>	44 <p>This test only runs in DRT!</p>

46	45

47 <video id="no-video-media" controls loop oncanplaythrough="runTest()"></vide o>	46 <video id="no-video-media" controls loop oncanplaythrough="runTest()"></vide o>

48 <script>	47 <script>

49 window.internals.settings.setDeviceSupportsMouse(false);

50 setSrcById("no-video-media", findMediaFile("video", "content/test"));	48 setSrcById("no-video-media", findMediaFile("video", "content/test"));

51 </script>	49 </script>

52 </body>	50 </body>

53 </html>	51 </html>

OLD	NEW