Improve detection of touch events when hiding media controls.

Improve detection of touch events when hiding media controls.

We propagate synthetic state down from PlatformMouseEvent
into MouseEvent / MediaControls instead of relying on
whether the platform does not support a mouse. This allows
us to hide the controls after touch even in those platforms
that support a mouse.

We rename NotFromTouch to RealOrIndistinguishable in
preparation for adding new values, for example to replace
isSimulated.

BUG=401177
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181055

[Ignacio Solla, 2014-08-19 11:58:50]

Discussion moved from https://codereview.chromium.org/441193003/:

-> philipj noted:

"The isSimulated idea sounded promising. In
https://codereview.chromium.org/453493002/ you added derivesFromTouch which is
usually the same as isSimulated but not always. Which are the cases when
isSimulated did the wrong thing?"

I don't think that we should reuse isSimulated here. isSimulated is used for
something completely different and I would not like to overload its meaning:

- isSimulated: as far as I can tell it is true when the mouse events have been
derived from  keyboard input. See how Node::dispatchSimulatedClick is used.
- derivesFromTouch: it is true when the mouse events have been derived from
touch input

There are behavioural differences, for example isSimulated events do not have a
click position:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

As such, I do not think that it is safe to reuse isSimulated here.

[philipj_slow, 2014-08-19 12:29:02]

OK, I guess isSimulated as a more generic name than it should. It's also only
used in very few places, to the point that I wonder if it's really needed.
That's off-topic, though.

[Rick Byers, 2014-08-19 16:06:48]

Extending MouseEvent to be aware when it's derived from touch is definitely a
good idea - something I've wanted to do and event expose to the web (see
http://crbug.com/115475).  Thanks!

I think the approach should be cleaned up a little though and unified with what
we've already got (see comments).

https://codereview.chromium.org/453493002/diff/40001/Source/core/events/Mouse...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/40001/Source/core/events/Mouse...
Source/core/events/MouseEvent.h:62: PassRefPtrWillBeRawPtr<EventTarget>
relatedTarget, PassRefPtrWillBeRawPtr<DataTransfer>, bool isSimulated = false,
bool derivesFromTouch = false);
Rather than add but-yet-another bool, I think we should probably follow the
pattern already established by PlatformMouseEvent which has a
'SyntheticEventType' member which can be 'FromTouch'.  Most often that should
just be propagated down from the PlatformMouseEvent to the MouseEvent - not
specified explicitly.

https://codereview.chromium.org/453493002/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/453493002/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2241:
dispatchMouseEvent(EventTypeNames::mousemove, newHitTest.targetNode(), 0,
fakeMouseMove, true, true);
Rather than take a completely new bool here, this should be relying on
PlatformMouseEvent::fromTouch() (you can see that the line above identifies this
mouse event as being synthesized from touch).

[Ignacio Solla, 2014-08-20 14:23:40]

On 2014/08/19 16:06:48, Rick Byers wrote:
> Extending MouseEvent to be aware when it's derived from touch is definitely a
> good idea - something I've wanted to do and event expose to the web (see
> http://crbug.com/115475).  Thanks!
> 
> I think the approach should be cleaned up a little though and unified with
what
> we've already got (see comments).
> 
>
https://codereview.chromium.org/453493002/diff/40001/Source/core/events/Mouse...
> File Source/core/events/MouseEvent.h (right):
> 
>
https://codereview.chromium.org/453493002/diff/40001/Source/core/events/Mouse...
> Source/core/events/MouseEvent.h:62: PassRefPtrWillBeRawPtr<EventTarget>
> relatedTarget, PassRefPtrWillBeRawPtr<DataTransfer>, bool isSimulated = false,
> bool derivesFromTouch = false);
> Rather than add but-yet-another bool, I think we should probably follow the
> pattern already established by PlatformMouseEvent which has a
> 'SyntheticEventType' member which can be 'FromTouch'.  Most often that should
> just be propagated down from the PlatformMouseEvent to the MouseEvent - not
> specified explicitly.
> 
>
https://codereview.chromium.org/453493002/diff/40001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/453493002/diff/40001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:2241:
> dispatchMouseEvent(EventTypeNames::mousemove, newHitTest.targetNode(), 0,
> fakeMouseMove, true, true);
> Rather than take a completely new bool here, this should be relying on
> PlatformMouseEvent::fromTouch() (you can see that the line above identifies
this
> mouse event as being synthesized from touch).

I'm propagating PlatformMouseEvent::SyntheticEventType up to MouseRelatedEvent,
is this what you are proposing? Please, let me know if reusing the enum in this
way is acceptable or if we should move it somewhere else.

[Rick Byers, 2014-08-20 23:32:56]

This is much cleaner, thanks!  Can you add a test - eg. that verifies media
control scenarios involving both a touchscreen and a mouse?

https://codereview.chromium.org/453493002/diff/60001/Source/core/events/Mouse...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/60001/Source/core/events/Mouse...
Source/core/events/MouseEvent.h:63: bool isSimulated = false,
PlatformMouseEvent::SyntheticEventType = PlatformMouseEvent::NotFromTouch);
Re-using the PlatformMouseEvent::SyntheticEventType enum is fine with me.  As
far as I'm concerned the platform/core abstraction layer around events isn't
worth the extra complexity.

I looked into the isSimulated stuff.  Looks like it's used just for
Element.dispatchSimulatedClick.  I don't understand though why it isn't just a
virtual method that results false but is overriden by SimulatedMouseEvent.

I think having both "simulated" and "synthetic" is a little confusing.  Ideally
we'd collapse them into the enum and add another value called "simulated" that
is defined not to carry any position information (that's really what makes these
events special).  If you don't want to do that refactoring now, feel free to add
a FIXME and comment somewhere that at least describes the difference between
"simulated" and "synthetic".

https://codereview.chromium.org/453493002/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/453493002/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1873: return !m_nodeUnderMouse ||
m_nodeUnderMouse->dispatchMouseEvent(mouseEvent, eventType, clickCount, 0);
What's this for?

[Ignacio Solla, 2014-08-21 17:30:14]

Tests are good. When adding them I discovered a case where this did not work,
and I had to add a new value to SyntheticEventType. Basically, we should not
make the controls visible when handling synthetic mouse events generated by
Blink during re-layouts or scrolls which do not derive from real input.

I had tested this manually on Android, but this problem did not surface there
because of the early-out in EventHandler::fakeMouseMoveEventTimerFired when
deviceSupportsMouse is false.

https://codereview.chromium.org/453493002/diff/60001/Source/core/events/Mouse...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/60001/Source/core/events/Mouse...
Source/core/events/MouseEvent.h:63: bool isSimulated = false,
PlatformMouseEvent::SyntheticEventType = PlatformMouseEvent::NotFromTouch);
On 2014/08/20 23:32:56, Rick Byers wrote:
> Re-using the PlatformMouseEvent::SyntheticEventType enum is fine with me.  As
> far as I'm concerned the platform/core abstraction layer around events isn't
> worth the extra complexity.
> 
> I looked into the isSimulated stuff.  Looks like it's used just for
> Element.dispatchSimulatedClick.  I don't understand though why it isn't just a
> virtual method that results false but is overriden by SimulatedMouseEvent.
> 
> I think having both "simulated" and "synthetic" is a little confusing. 
Ideally
> we'd collapse them into the enum and add another value called "simulated" that
> is defined not to carry any position information (that's really what makes
these
> events special).  If you don't want to do that refactoring now, feel free to
add
> a FIXME and comment somewhere that at least describes the difference between
> "simulated" and "synthetic".

Yes, I completely agree that isSimulated should be collapse into the
SyntheticEventType enum. I have added the FIXME and filed crbug/405990

https://codereview.chromium.org/453493002/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/453493002/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:1873: return !m_nodeUnderMouse ||
m_nodeUnderMouse->dispatchMouseEvent(mouseEvent, eventType, clickCount, 0);
On 2014/08/20 23:32:56, Rick Byers wrote:
> What's this for?

Left-over from previous patch version.

[Ignacio Solla, 2014-08-21 17:42:31]

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html
(right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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]

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File LayoutTests/media/video-controls-hide-after-touch-on-control.html (right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-hide-after-touch-on-control.html:25:
eventSender.gestureTap(coords[0], coords[1]);
for maximum fidelity with how touch actually behaves you should really send a
sequence of events here:
gestureTapDown
gestureShowPress
gestureTap

eg. if there are any :active/:hover effects (which could interfere with the
tap), they'd show up on gestureShowPress.

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html
(right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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.

https://codereview.chromium.org/453493002/diff/80001/Source/core/events/Mouse...
File Source/core/events/MouseRelatedEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/events/Mouse...
Source/core/events/MouseRelatedEvent.h:95:
PlatformMouseEvent::SyntheticEventType m_syntheticEventType;
Adding this to MouseRelatedEvent doesn't really make sense to me.  Eg.
GestureEvent is (kind of surprisingly) a MouseRelatedEvent, and there
"synthesized from touch" doesn't make a lot of sense because gestures are ALWAYS
synthesized from touch.

It looks to me like only MouseEvent ever set isSimulated, and the only callers
that ask for isSumulated on something that may not be a MouseEvent are
MouseRelatedEvent::offsetX/Y methods.  They could just as easily use
'isMouseEvent/toMouseEvent' or be virtual methods.

So I think you should put m_syntheticEventType just in MouseEvent, not
SyntheticMouseEvent.  Then later MouseRelatedEvent::m_isSimulated can easily be
replaced with MouseEvent::m_syntheticEventType.

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:381: m_wasLastEventTouch =
event->isTouchEvent() || (event->isMouseEvent() &&
toMouseEvent(event)->fromTouch());
Do you really want to consider touch events here?  Or would you be better off
bailing out of the function when event->isMouseEvent() is false (i.e. tracking
only whether the last mouse event was from touch).

If you want to handle all types of events in this logic, then you should
probably consider gesture events too (which always come from touch input).

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
File Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:40: NotSyntheticOrIndistinguishable, //
Real mouse input events or synthetic events that behave just like real events
nit: maybe put these comments on the line above the value - we don't want to
discourage long comments here, if we need multiple lines that's fine.

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:40: NotSyntheticOrIndistinguishable, //
Real mouse input events or synthetic events that behave just like real events
nit: this name is a bit of a mouthful.  I like that you're explicit that they
may still be synthetic though.  Maybe "RealOrIndistinguishable" would be a
little clearer?

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:76: PlatformMouseEvent(const IntPoint&
position, const IntPoint& globalPosition, MouseButton button,
PlatformEvent::Type type, int clickCount, bool shiftKey, bool ctrlKey, bool
altKey, bool metaKey, double timestamp, SyntheticEventType synthesized =
NotSyntheticOrIndistinguishable)
I think we should try to keep the parameter order consistent (synthesized comes
before timestamp in the above version).  We also tend to avoid default argument
values (although I don't think they're prohibited outright like in the chromium
style guide).  I guess that means either add but-yet-another overload, or update
the handful of callers (eclipse finds 16 for me including tests).

[Ignacio Solla, 2014-08-26 13:40:53]

Thanks for the suggestions / review. I think not distinguishing NotFromIput
events is much better.

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File LayoutTests/media/video-controls-hide-after-touch-on-control.html (right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-hide-after-touch-on-control.html:25:
eventSender.gestureTap(coords[0], coords[1]);
On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> for maximum fidelity with how touch actually behaves you should really send a
> sequence of events here:
> gestureTapDown
> gestureShowPress
> gestureTap
> 
> eg. if there are any :active/:hover effects (which could interfere with the
> tap), they'd show up on gestureShowPress.

Done.

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html
(right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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?

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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.

https://codereview.chromium.org/453493002/diff/80001/Source/core/events/Mouse...
File Source/core/events/MouseRelatedEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/events/Mouse...
Source/core/events/MouseRelatedEvent.h:95:
PlatformMouseEvent::SyntheticEventType m_syntheticEventType;
On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> Adding this to MouseRelatedEvent doesn't really make sense to me.  Eg.
> GestureEvent is (kind of surprisingly) a MouseRelatedEvent, and there
> "synthesized from touch" doesn't make a lot of sense because gestures are
ALWAYS
> synthesized from touch.
> 
> It looks to me like only MouseEvent ever set isSimulated, and the only callers
> that ask for isSumulated on something that may not be a MouseEvent are
> MouseRelatedEvent::offsetX/Y methods.  They could just as easily use
> 'isMouseEvent/toMouseEvent' or be virtual methods.
> 
> So I think you should put m_syntheticEventType just in MouseEvent, not
> SyntheticMouseEvent.  Then later MouseRelatedEvent::m_isSimulated can easily
be
> replaced with MouseEvent::m_syntheticEventType.

Done.

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:381: m_wasLastEventTouch =
event->isTouchEvent() || (event->isMouseEvent() &&
toMouseEvent(event)->fromTouch());
On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> Do you really want to consider touch events here?  Or would you be better off
> bailing out of the function when event->isMouseEvent() is false (i.e. tracking
> only whether the last mouse event was from touch).
> 
> If you want to handle all types of events in this logic, then you should
> probably consider gesture events too (which always come from touch input).

Good catch, I consider gesture events now too.

I assume that the cursor will hide with both gesture events, touch events and
fromTouch mouse events, so we need to track the last event, not just the last
mouse event. Otherwise we will still be showing the controls forever without the
mouse pointer hovering them.

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
File Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:40: NotSyntheticOrIndistinguishable, //
Real mouse input events or synthetic events that behave just like real events
On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> nit: maybe put these comments on the line above the value - we don't want to
> discourage long comments here, if we need multiple lines that's fine.

Done.

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:40: NotSyntheticOrIndistinguishable, //
Real mouse input events or synthetic events that behave just like real events
On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> nit: this name is a bit of a mouthful.  I like that you're explicit that they
> may still be synthetic though.  Maybe "RealOrIndistinguishable" would be a
> little clearer?

I agree, it is a mouthful, I've changed it now to RealOrIndistinguishable.

FYI, I was on the verge, but had decided to use not synthetic because:
- synthetic is a more precise term than real
- one could argue that synthetic events are also real.

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:76: PlatformMouseEvent(const IntPoint&
position, const IntPoint& globalPosition, MouseButton button,
PlatformEvent::Type type, int clickCount, bool shiftKey, bool ctrlKey, bool
altKey, bool metaKey, double timestamp, SyntheticEventType synthesized =
NotSyntheticOrIndistinguishable)
Done. 

Added a non-optional arg and updated all callers.

[Rick Byers, 2014-08-26 15:25:46]

This LGTM (modulo changing a couple RealOr.. to FromTouch).

Please update the CL description to describe a bit more of the idea here:
pushing synthetic state down from PlatformMouseEvent into MouseEvent and
renaming NotTouch in preparation for adding other values.

I'd suggest aelias for an OWNER review of Source/web/

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html
(right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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).

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:381: m_wasLastEventTouch =
event->isTouchEvent() || (event->isMouseEvent() &&
toMouseEvent(event)->fromTouch());
On 2014/08/26 13:40:53, Ignacio Solla wrote:
> On 2014/08/22 18:11:13, Rick Byers (OOO Aug 24th) wrote:
> > Do you really want to consider touch events here?  Or would you be better
off
> > bailing out of the function when event->isMouseEvent() is false (i.e.
tracking
> > only whether the last mouse event was from touch).
> > 
> > If you want to handle all types of events in this logic, then you should
> > probably consider gesture events too (which always come from touch input).
> 
> Good catch, I consider gesture events now too.
> 
> I assume that the cursor will hide with both gesture events, touch events and
> fromTouch mouse events, so we need to track the last event, not just the last
> mouse event. Otherwise we will still be showing the controls forever without
the
> mouse pointer hovering them.

I see.  Seems fine to me.  Ideally maybe this would be more coupled to cursor
visibility state, but that's perhaps not currently reliable enough to take a
hard dependency on (I know tdanderson@ hit all sorts of edge cases on windows
where it was hard to know precisely).

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
File Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:76: PlatformMouseEvent(const IntPoint&
position, const IntPoint& globalPosition, MouseButton button,
PlatformEvent::Type type, int clickCount, bool shiftKey, bool ctrlKey, bool
altKey, bool metaKey, double timestamp, SyntheticEventType synthesized =
NotSyntheticOrIndistinguishable)
On 2014/08/26 13:40:53, Ignacio Solla wrote:
> Done. 
> 
> Added a non-optional arg and updated all callers.

Thank you!

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2290: gestureEvent.shiftKey(),
gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(),
PlatformMouseEvent::RealOrIndistinguishable, WTF::currentTime());
this should be Touch also - same argument as for the PopupContainer
(trivial/unobservable difference - let's be correct but it doesn't justify a new
test).

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2804: PlatformMouseEvent
mouseEvent(targetedEvent.event().position(),
targetedEvent.event().globalPosition(), RightButton, eventType, 1, false, false,
false, false, PlatformMouseEvent::RealOrIndistinguishable, WTF::currentTime());
also touch

https://codereview.chromium.org/453493002/diff/120001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/453493002/diff/120001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:296: PlatformMouseEvent
fakeMouseMove(gestureEvent.position(), gestureEvent.globalPosition(), NoButton,
PlatformEvent::MouseMoved, /* clickCount */ 1, gestureEvent.shiftKey(),
gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(),
PlatformMouseEvent::RealOrIndistinguishable, gestureEvent.timestamp());
These should be FromTouch (this is GestureTap handling).  I know you didn't
intend to change any behavior here, but if we're adding explicit values then we
should probably get them correct (or at least add a FIXME).  I think it's
unlikely anyone could observe the difference in this case, and since this is
pre-existing and trivial I'm OK making this change without a test.

[Ignacio Solla, 2014-08-26 17:30:43]

igsolla@chromium.org changed reviewers:
+ aelias@chromium.org, eroman@chromium.org

[Ignacio Solla, 2014-08-26 17:30:43]

Thanks for the review Rick.

aelias: Could you review the changes in Source/web please

eroman: Could you review the changes in Source/platform please

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
File
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html
(right):

https://codereview.chromium.org/453493002/diff/80001/LayoutTests/media/video-...
LayoutTests/media/video-controls-visibility-multimodal-touch-after-mouse.html:31:
// And the controls should hide after a timeout.
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.

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/453493002/diff/80001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:381: m_wasLastEventTouch =
event->isTouchEvent() || (event->isMouseEvent() &&
toMouseEvent(event)->fromTouch());
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:
> > > Do you really want to consider touch events here?  Or would you be better
> off
> > > bailing out of the function when event->isMouseEvent() is false (i.e.
> tracking
> > > only whether the last mouse event was from touch).
> > > 
> > > If you want to handle all types of events in this logic, then you should
> > > probably consider gesture events too (which always come from touch input).
> > 
> > Good catch, I consider gesture events now too.
> > 
> > I assume that the cursor will hide with both gesture events, touch events
and
> > fromTouch mouse events, so we need to track the last event, not just the
last
> > mouse event. Otherwise we will still be showing the controls forever without
> the
> > mouse pointer hovering them.
> 
> I see.  Seems fine to me.  Ideally maybe this would be more coupled to cursor
> visibility state, but that's perhaps not currently reliable enough to take a
> hard dependency on (I know tdanderson@ hit all sorts of edge cases on windows
> where it was hard to know precisely).

Acknowledged.

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
File Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/453493002/diff/80001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:76: PlatformMouseEvent(const IntPoint&
position, const IntPoint& globalPosition, MouseButton button,
PlatformEvent::Type type, int clickCount, bool shiftKey, bool ctrlKey, bool
altKey, bool metaKey, double timestamp, SyntheticEventType synthesized =
NotSyntheticOrIndistinguishable)
On 2014/08/26 15:25:46, Rick Byers wrote:
> On 2014/08/26 13:40:53, Ignacio Solla wrote:
> > Done. 
> > 
> > Added a non-optional arg and updated all callers.
> 
> Thank you!

Acknowledged.

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2290: gestureEvent.shiftKey(),
gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(),
PlatformMouseEvent::RealOrIndistinguishable, WTF::currentTime());
On 2014/08/26 15:25:46, Rick Byers wrote:
> this should be Touch also - same argument as for the PopupContainer
> (trivial/unobservable difference - let's be correct but it doesn't justify a
new
> test).

Done.

https://codereview.chromium.org/453493002/diff/120001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:2804: PlatformMouseEvent
mouseEvent(targetedEvent.event().position(),
targetedEvent.event().globalPosition(), RightButton, eventType, 1, false, false,
false, false, PlatformMouseEvent::RealOrIndistinguishable, WTF::currentTime());
On 2014/08/26 15:25:46, Rick Byers wrote:
> also touch

Done.

https://codereview.chromium.org/453493002/diff/120001/Source/web/PopupContain...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/453493002/diff/120001/Source/web/PopupContain...
Source/web/PopupContainer.cpp:296: PlatformMouseEvent
fakeMouseMove(gestureEvent.position(), gestureEvent.globalPosition(), NoButton,
PlatformEvent::MouseMoved, /* clickCount */ 1, gestureEvent.shiftKey(),
gestureEvent.ctrlKey(), gestureEvent.altKey(), gestureEvent.metaKey(),
PlatformMouseEvent::RealOrIndistinguishable, gestureEvent.timestamp());
On 2014/08/26 15:25:46, Rick Byers wrote:
> These should be FromTouch (this is GestureTap handling).  I know you didn't
> intend to change any behavior here, but if we're adding explicit values then
we
> should probably get them correct (or at least add a FIXME).  I think it's
> unlikely anyone could observe the difference in this case, and since this is
> pre-existing and trivial I'm OK making this change without a test.

Done.

[aelias_OOO_until_Jul13, 2014-08-26 17:59:34]

Source/web lgtm

[Ignacio Solla, 2014-08-27 13:18:28]

igsolla@chromium.org changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org

[Ignacio Solla, 2014-08-27 13:19:40]

abarth or eseidel or eroman: could anyone of you help me review the changes to
Source/platform/PlatformMouseEvent.h. Thanks!!

[eseidel, 2014-08-28 15:38:08]

lgtm

The names seem a bit funny to me.  The previous axis was touch or not touch. 
This is now synthesized or not.  But with values which are "real" vs. "touch". 
It's not immediately clear to my brain how "touch" relates to synthesized from
just reading the enum type name and values (without the comments).

[Ignacio Solla, 2014-08-28 16:00:47]

On 2014/08/28 15:38:08, eseidel wrote:
> lgtm
> 
> The names seem a bit funny to me.  The previous axis was touch or not touch. 
> This is now synthesized or not.  But with values which are "real" vs. "touch".

> It's not immediately clear to my brain how "touch" relates to synthesized from
> just reading the enum type name and values (without the comments).

NotFromTouch will not work once we unify the different ways in which we track
synthesized events and add a new value for isSimulated (more context in the
previous comments). My intention is to simplify things, not to make them more
complex. Real == NotSynthetic, but we decided to pick the first one because
NotSyntheticOrIndistinguishable would be a mouthful.

[Ignacio Solla, 2014-08-28 16:01:11]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-08-28 16:02:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/igsolla@chromium.org/453493002/140001

[commit-bot: I haz the power, 2014-08-28 16:14:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[commit-bot: I haz the power, 2014-08-28 16:27:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-28 16:27:32]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[abarth-chromium, 2014-08-28 17:10:25]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-08-28 17:11:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/igsolla@chromium.org/453493002/140001

[commit-bot: I haz the power, 2014-08-28 17:24:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[commit-bot: I haz the power, 2014-08-28 17:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-28 17:37:58]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[Ignacio Solla, 2014-08-28 18:14:48]

On 2014/08/28 17:37:58, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_dbg on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

rbyers: could you take a look at path set 9 (delta 8 to EventHandler.cpp and
PopupContainer.cpp)? I have reverted patch set 8 because that is a nice to have
but those functional changes are not required to fix bug 401177.

The reason why I reverted it is because it made some tests crash. Before we set
the type of those events to FromTouch we need to make sure that we don't
re-rerun hit-tests again for them. I know that you have worked previously in
this area (https://codereview.chromium.org/353893002/) so I have filed a new bug
and assign it to you
(https://code.google.com/p/chromium/issues/detail?id=408660). Let me know if
that's ok. Thanks!!

[Rick Byers, 2014-08-28 18:20:55]

On 2014/08/28 18:14:48, Ignacio Solla wrote:
> On 2014/08/28 17:37:58, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_dbg on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
> 
> rbyers: could you take a look at path set 9 (delta 8 to EventHandler.cpp and
> PopupContainer.cpp)? I have reverted patch set 8 because that is a nice to
have
> but those functional changes are not required to fix bug 401177.
> 
> The reason why I reverted it is because it made some tests crash. Before we
set
> the type of those events to FromTouch we need to make sure that we don't
> re-rerun hit-tests again for them. I know that you have worked previously in
> this area (https://codereview.chromium.org/353893002/) so I have filed a new
bug
> and assign it to you
> (https://code.google.com/p/chromium/issues/detail?id=408660). Let me know if
> that's ok. Thanks!!

Ah, sorry about that - I forgot about that ASSERT (I recently added it to cover
some cases I had fixed, but clearly others escaped my notice).  Yes this still
LGTM - I'll land a follow-up ASAP.

[Ignacio Solla, 2014-08-28 18:22:23]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-08-28 18:23:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/igsolla@chromium.org/453493002/160001

[commit-bot: I haz the power, 2014-08-28 19:03:35]

Committed patchset #9 (id:160001) as 181055