Hide video controls after touch when hideMediaControlsTimerFired fires.

Hide video controls after touch from hideMediaControlsTimerFired.

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

[Ignacio Solla, 2014-08-06 19:05:56]

https://codereview.chromium.org/441193003/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-controls-always-visible-when-control-hovered.html
(right):

https://codereview.chromium.org/441193003/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-always-visible-when-control-hovered.html:32:
runAfterHideMediaControlsTimerFired(function()
Note that this is a new file, but for some reason a diff is shown against
video-controls-hide-on-move-outside-controls.html

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

https://codereview.chromium.org/441193003/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-hide-after-touch-on-control.html:32:
runAfterHideMediaControlsTimerFired(function()
Note that this is a new file, but for some reason a diff is shown against
video-controls-hide-on-move-outside-controls.html

[qinmin, 2014-08-06 20:16:06]

this will impact desktop behavior.
I think we should set the mouseEvent as simulated if it is converted from a
touch event, and then hide the control if mouse up is a simulated event.

MouseRelatedEvent class already has a isSimulated member there, i think we can
reuse it for the purpose of converted touch event

[Ignacio Solla, 2014-08-07 18:17:46]

On 2014/08/06 20:16:06, qinmin wrote:
> this will impact desktop behavior.
> I think we should set the mouseEvent as simulated if it is converted from a
> touch event, and then hide the control if mouse up is a simulated event.
> 
> MouseRelatedEvent class already has a isSimulated member there, i think we can
> reuse it for the purpose of converted touch event

Thanks for the pointer about isSimulated, I hadn't noticed that it was there.
I'm working on a patch to replace the check
(https://codereview.chromium.org/453493002/), but I would like to keep both
changes separate if possible.

Why could this impact desktop behaviour?

This change only changes the behaviour if deviceSupportsMouse is false, and that
should not be the case for desktops as they support mouses, but please let me
know otherwise. So in all the platforms with mouses the behaviour will not
change.

In summary, this is what the changes do:
1. This patch fixes crbug/401159 in platforms that do not support a mouse, for
example Android.
2. https://codereview.chromium.org/453493002/ will also fix crbug/401159 in
platforms that support both touch and a mouse.

2 is more complex and I suspect that it will require a lengthier review (and I
still need to figure out how to make the Layout Test pass). In order to get the
fix for 1 as soon as possible for WebView and schedule another round of testing
I'd like to keep them separate. Please let me know if that's ok.


(Extra details. If the following is possible, then this change is not safe.
- mouseEvents can be derived from touch, real mouse inputs, and some other type
of input call X.
- the controls should remain visible after real mouse inputs or X, but not
touch.
- some platforms only support touch and X, but not real mouse inputs
- then this patch will break things on those platforms, but I am not aware of
any)

[Ignacio Solla, 2014-08-07 18:24:16]

On 2014/08/07 18:17:46, Ignacio Solla wrote:
> On 2014/08/06 20:16:06, qinmin wrote:
> > this will impact desktop behavior.
> > I think we should set the mouseEvent as simulated if it is converted from a
> > touch event, and then hide the control if mouse up is a simulated event.
> > 
> > MouseRelatedEvent class already has a isSimulated member there, i think we
can
> > reuse it for the purpose of converted touch event
> 
> Thanks for the pointer about isSimulated, I hadn't noticed that it was there.
> I'm working on a patch to replace the check
> (https://codereview.chromium.org/453493002/), but I would like to keep both
> changes separate if possible.
> 
> Why could this impact desktop behaviour?
> 
> This change only changes the behaviour if deviceSupportsMouse is false, and
that
> should not be the case for desktops as they support mouses, but please let me
> know otherwise. So in all the platforms with mouses the behaviour will not
> change.
> 
> In summary, this is what the changes do:
> 1. This patch fixes crbug/401159 in platforms that do not support a mouse, for
> example Android.
> 2. https://codereview.chromium.org/453493002/ will also fix crbug/401159 in
> platforms that support both touch and a mouse.
> 
> 2 is more complex and I suspect that it will require a lengthier review (and I
> still need to figure out how to make the Layout Test pass). In order to get
the
> fix for 1 as soon as possible for WebView and schedule another round of
testing
> I'd like to keep them separate. Please let me know if that's ok.
> 
> 
> (Extra details. If the following is possible, then this change is not safe.
> - mouseEvents can be derived from touch, real mouse inputs, and some other
type
> of input call X.
> - the controls should remain visible after real mouse inputs or X, but not
> touch.
> - some platforms only support touch and X, but not real mouse inputs
> - then this patch will break things on those platforms, but I am not aware of
> any)

Also, it might be helpful, I had filed this bug with more details:
https://code.google.com/p/chromium/issues/detail?id=401177

[philipj_slow, 2014-08-11 07:55:53]

Is this still waiting for review, or does
https://codereview.chromium.org/453493002/ make this obsolete?

[Ignacio Solla, 2014-08-11 08:39:29]

On 2014/08/11 07:55:53, philipj wrote:
> Is this still waiting for review, or does
> https://codereview.chromium.org/453493002/ make this obsolete?

This is still waiting for review. This change in isolation already fixes
http://www.crbug.com/401159, and the subsequent
https://codereview.chromium.org/453493002/ is just an improvement.

[philipj_slow, 2014-08-13 15:34:09]

+fs +acolwell who looked at this code recently.

The problem: touching fires mouse* events in addition to the touch* evenst, and
that makes it seem like the video is being hovered.

How would one do it using only Web-exposed APIs, or with Blink-in-JS?
https://codereview.chromium.org/456323002/

It seems to me that setting m_isMouseOverControls to false and starting the hide
timer in a touchend event handler should do the right thing, why doesn't that
work?

[fs, 2014-08-15 18:29:07]

On 2014/08/13 15:34:09, philipj wrote:
> +fs +acolwell who looked at this code recently.
> 
> The problem: touching fires mouse* events in addition to the touch* evenst,
and
> that makes it seem like the video is being hovered.
> 
> How would one do it using only Web-exposed APIs, or with Blink-in-JS?
> https://codereview.chromium.org/456323002/

By actually handling the touch events themselves? (And hence preventing the
"legacy conversion".)

> It seems to me that setting m_isMouseOverControls to false and starting the
hide
> timer in a touchend event handler should do the right thing, why doesn't that
> work?

Presumably that doesn't work because m_panel->hovered() is still true. (And the
check when the timer fires completely disregards said flag.)

[philipj_slow, 2014-08-15 21:07:16]

On 2014/08/15 18:29:07, fs (ooo) wrote:
> On 2014/08/13 15:34:09, philipj wrote:
> > +fs +acolwell who looked at this code recently.
> > 
> > The problem: touching fires mouse* events in addition to the touch* evenst,
> and
> > that makes it seem like the video is being hovered.
> > 
> > How would one do it using only Web-exposed APIs, or with Blink-in-JS?
> > https://codereview.chromium.org/456323002/
> 
> By actually handling the touch events themselves? (And hence preventing the
> "legacy conversion".)

Yep, I wrote a little test and by calling preventDefault() on the touchend event
the fake mouse events aren't fired. Would doing the equivalent of that
internally solve the problem?

[Ignacio Solla, 2014-08-19 09:23:47]

On 2014/08/15 21:07:16, philipj wrote:
> On 2014/08/15 18:29:07, fs (ooo) wrote:
> > On 2014/08/13 15:34:09, philipj wrote:
> > > +fs +acolwell who looked at this code recently.
> > > 
> > > The problem: touching fires mouse* events in addition to the touch*
evenst,
> > and
> > > that makes it seem like the video is being hovered.
> > > 
> > > How would one do it using only Web-exposed APIs, or with Blink-in-JS?
> > > https://codereview.chromium.org/456323002/
> > 
> > By actually handling the touch events themselves? (And hence preventing the
> > "legacy conversion".)
> 
> Yep, I wrote a little test and by calling preventDefault() on the touchend
event
> the fake mouse events aren't fired. Would doing the equivalent of that
> internally solve the problem?

I may not have the full picture, but I think that we need to translate touch
events into mouse events because MediaControls only handles mouse events.
Otherwise, we will not fire mouseover/mousemove events and the controls will not
become visible on touch.

[philipj_slow, 2014-08-19 11:26:37]

Right, the controls depend on the simulated mouse events, so if implementing
this using only Web-exposed APIs one would also have to handle touchend events
in all the places that we currently handle click events, as well as showing the
controls in the touchstart/touchmove/touchend events. With only default event
handlers to work with, maybe that won't actually be an improvement.

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?

At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS, which can
use regular event handlers and preventDefault() more easily, will simplify
things somewhat.

I'd like a second opinion. rbyers may have ideas on the input conversion stuff,
and otherwise acolwell has been reviewing my own media controls changes.

[Ignacio Solla, 2014-08-19 12:04:45]

On 2014/08/19 11:26:37, philipj wrote:
> Right, the controls depend on the simulated mouse events, so if implementing
> this using only Web-exposed APIs one would also have to handle touchend events
> in all the places that we currently handle click events, as well as showing
the
> controls in the touchstart/touchmove/touchend events. With only default event
> handlers to work with, maybe that won't actually be an improvement.
> 
> 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?
> 
> At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS, which
can
> use regular event handlers and preventDefault() more easily, will simplify
> things somewhat.
> 
> I'd like a second opinion. rbyers may have ideas on the input conversion
stuff,
> and otherwise acolwell has been reviewing my own media controls changes.

Please find my reply about isSimulated vs derivesFromTouch in
https://codereview.chromium.org/453493002/. I have moved it there because I
believe this change is independent of that decision.

[Rick Byers, 2014-08-19 16:14:12]

On 2014/08/19 11:26:37, philipj wrote:
> Right, the controls depend on the simulated mouse events, so if implementing
> this using only Web-exposed APIs one would also have to handle touchend events
> in all the places that we currently handle click events, as well as showing
the
> controls in the touchstart/touchmove/touchend events. With only default event
> handlers to work with, maybe that won't actually be an improvement.

This is still a major pain (means you have to have your own click detection in
JS, which will never be as good as the browser's).  See my comments on this bug
(http://crbug.com/401159) for details and references to how we're trying to
improve the platform here.  But then, yes, we should totally be using ShadowDOM
and Blink-in-JS for these controls (or at least hooking into the exact same code
paths that JS would use).

> 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?
> 
> At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS, which
can
> use regular event handlers and preventDefault() more easily, will simplify
> things somewhat.
> 
> I'd like a second opinion. rbyers may have ideas on the input conversion
stuff,
> and otherwise acolwell has been reviewing my own media controls changes.

We should definitely be able to reliably identify mouse events that are
synthesized from touch (we already have that for PlatformMouseEvent).  I've
added comments on that CR as well, I don't think it'll be hard to land.  But is
it sufficient?  You'll still have the problem that Node::hovered() will return
true so you couldn't rely on that API.  If that's not too hard to fix, I'd
prefer we just go that route - it's closer to the right behavior.  In
particular, we should consider the case of a touchscreen laptop here too - the
controls should hide after a timeout there as well (even though the device does
support a mouse).

[Rick Byers, 2014-08-19 16:15:23]

On 2014/08/19 16:14:12, Rick Byers wrote:
> On 2014/08/19 11:26:37, philipj wrote:
> > Right, the controls depend on the simulated mouse events, so if implementing
> > this using only Web-exposed APIs one would also have to handle touchend
events
> > in all the places that we currently handle click events, as well as showing
> the
> > controls in the touchstart/touchmove/touchend events. With only default
event
> > handlers to work with, maybe that won't actually be an improvement.
> 
> This is still a major pain (means you have to have your own click detection in
> JS, which will never be as good as the browser's).  See my comments on this
bug
> (http://crbug.com/401159) for details and references to how we're trying to
> improve the platform here.  But then, yes, we should totally be using
ShadowDOM
> and Blink-in-JS for these controls (or at least hooking into the exact same
code
> paths that JS would use).
> 
> > 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?
> > 
> > At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS, which
> can
> > use regular event handlers and preventDefault() more easily, will simplify
> > things somewhat.
> > 
> > I'd like a second opinion. rbyers may have ideas on the input conversion
> stuff,
> > and otherwise acolwell has been reviewing my own media controls changes.
> 
> We should definitely be able to reliably identify mouse events that are
> synthesized from touch (we already have that for PlatformMouseEvent).  I've
> added comments on that CR as well, I don't think it'll be hard to land.  But
is
> it sufficient?  You'll still have the problem that Node::hovered() will return
> true so you couldn't rely on that API.  If that's not too hard to fix, I'd
> prefer we just go that route - it's closer to the right behavior.  In
> particular, we should consider the case of a touchscreen laptop here too - the
> controls should hide after a timeout there as well (even though the device
does
> support a mouse).

Let me know what you guys think.  If there's a strong reason to believe getting
that approach working will be difficult or otherwise independent of what you're
trying to do here, then I'm happy to do a more thorough review of this CL.  But
naively it seems like it shouldn't be hard to just do this right based on a
'synthesized from touch' event in the mouse events.

[Ignacio Solla, 2014-08-19 16:23:54]

On 2014/08/19 16:15:23, Rick Byers wrote:
> On 2014/08/19 16:14:12, Rick Byers wrote:
> > On 2014/08/19 11:26:37, philipj wrote:
> > > Right, the controls depend on the simulated mouse events, so if
implementing
> > > this using only Web-exposed APIs one would also have to handle touchend
> events
> > > in all the places that we currently handle click events, as well as
showing
> > the
> > > controls in the touchstart/touchmove/touchend events. With only default
> event
> > > handlers to work with, maybe that won't actually be an improvement.
> > 
> > This is still a major pain (means you have to have your own click detection
in
> > JS, which will never be as good as the browser's).  See my comments on this
> bug
> > (http://crbug.com/401159) for details and references to how we're trying to
> > improve the platform here.  But then, yes, we should totally be using
> ShadowDOM
> > and Blink-in-JS for these controls (or at least hooking into the exact same
> code
> > paths that JS would use).
> > 
> > > 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?
> > > 
> > > At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS,
which
> > can
> > > use regular event handlers and preventDefault() more easily, will simplify
> > > things somewhat.
> > > 
> > > I'd like a second opinion. rbyers may have ideas on the input conversion
> > stuff,
> > > and otherwise acolwell has been reviewing my own media controls changes.
> > 
> > We should definitely be able to reliably identify mouse events that are
> > synthesized from touch (we already have that for PlatformMouseEvent).  I've
> > added comments on that CR as well, I don't think it'll be hard to land.  But
> is
> > it sufficient?  You'll still have the problem that Node::hovered() will
return
> > true so you couldn't rely on that API.  If that's not too hard to fix, I'd
> > prefer we just go that route - it's closer to the right behavior.  In
> > particular, we should consider the case of a touchscreen laptop here too -
the
> > controls should hide after a timeout there as well (even though the device
> does
> > support a mouse).
> 
> Let me know what you guys think.  If there's a strong reason to believe
getting
> that approach working will be difficult or otherwise independent of what
you're
> trying to do here, then I'm happy to do a more thorough review of this CL. 
But
> naively it seems like it shouldn't be hard to just do this right based on a
> 'synthesized from touch' event in the mouse events.

I completely agree that we should fix this problem for touchscreen laptops too.
But I just wanted to move one small step at the time:
- This patch fixes the bug for touch-only devices, ie. Android. 
- The following patch builds on this change and fixes the bug for touch+mouse
devices too.

If you prefer I could combine both changes,  but I think it'd be easier to
review and submit as separate changes.

[Rick Byers, 2014-08-20 23:36:03]

On 2014/08/19 16:23:54, Ignacio Solla wrote:
> On 2014/08/19 16:15:23, Rick Byers wrote:
> > On 2014/08/19 16:14:12, Rick Byers wrote:
> > > On 2014/08/19 11:26:37, philipj wrote:
> > > > Right, the controls depend on the simulated mouse events, so if
> implementing
> > > > this using only Web-exposed APIs one would also have to handle touchend
> > events
> > > > in all the places that we currently handle click events, as well as
> showing
> > > the
> > > > controls in the touchstart/touchmove/touchend events. With only default
> > event
> > > > handlers to work with, maybe that won't actually be an improvement.
> > > 
> > > This is still a major pain (means you have to have your own click
detection
> in
> > > JS, which will never be as good as the browser's).  See my comments on
this
> > bug
> > > (http://crbug.com/401159) for details and references to how we're trying
to
> > > improve the platform here.  But then, yes, we should totally be using
> > ShadowDOM
> > > and Blink-in-JS for these controls (or at least hooking into the exact
same
> > code
> > > paths that JS would use).
> > > 
> > > > 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?
> > > > 
> > > > At this point I'm inclined to L-G-T-M this and hope that Blink-in-JS,
> which
> > > can
> > > > use regular event handlers and preventDefault() more easily, will
simplify
> > > > things somewhat.
> > > > 
> > > > I'd like a second opinion. rbyers may have ideas on the input conversion
> > > stuff,
> > > > and otherwise acolwell has been reviewing my own media controls changes.
> > > 
> > > We should definitely be able to reliably identify mouse events that are
> > > synthesized from touch (we already have that for PlatformMouseEvent). 
I've
> > > added comments on that CR as well, I don't think it'll be hard to land. 
But
> > is
> > > it sufficient?  You'll still have the problem that Node::hovered() will
> return
> > > true so you couldn't rely on that API.  If that's not too hard to fix, I'd
> > > prefer we just go that route - it's closer to the right behavior.  In
> > > particular, we should consider the case of a touchscreen laptop here too -
> the
> > > controls should hide after a timeout there as well (even though the device
> > does
> > > support a mouse).
> > 
> > Let me know what you guys think.  If there's a strong reason to believe
> getting
> > that approach working will be difficult or otherwise independent of what
> you're
> > trying to do here, then I'm happy to do a more thorough review of this CL. 
> But
> > naively it seems like it shouldn't be hard to just do this right based on a
> > 'synthesized from touch' event in the mouse events.
> 
> I completely agree that we should fix this problem for touchscreen laptops
too.
> But I just wanted to move one small step at the time:
> - This patch fixes the bug for touch-only devices, ie. Android. 
> - The following patch builds on this change and fixes the bug for touch+mouse
> devices too.
> 
> If you prefer I could combine both changes,  but I think it'd be easier to
> review and submit as separate changes.

That's fine with me.  I've skimmed the changes but I don't have context with
media controls.  If someone (Philip?) who is wants to L-G this patch, then I can
rubber-stamp it.

[philipj_slow, 2014-08-21 08:42:34]

I've reviewed everything again, LGTM.

Note that Blink doesn't use TODO(user), just FIXME.

[Ignacio Solla, 2014-08-21 08:58:31]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-08-21 08:58:46]

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

[commit-bot: I haz the power, 2014-08-21 16:05:03]

Committed patchset #5 (80001) as 180720