Clear hover flag after switching full-screen and inline video.

Clear hover flag after switching full-screen and inline video.

BUG=374716
Media Controls does not fade out after switching between full-screen and inline video mode,
because MediaControls is recorded as hovered after switching.

But actually, after switching between full-screen and inline mode, Media Controls would move
to another position and not hovered by mouse or touch focus.

Clear the hover flag after switching between full-screen and inline mode would help MediaControls
to fade out after 3 seconds without mouse movement. Thus reduce 200mW power consumption during
video playback. (Measured on Intel tablet platform)

[eric_li, 2014-05-22 05:19:03]

Dear acolwel and dominicc:

   Could you help take some time to review this patch? This patch is to solve
the problem that media progress bar does not fade out after switching between
full-screen and inline video.

   Thanks in advance.

Eric

[philipj_slow, 2014-05-22 08:58:23]

The only other code to call setUserActionElement is
core/dom/UserActionElementSet.cpp, which makes me think it probably isn't
intended for "outside" use like in this patch.

I can't say I understand why the existing behavior is the way it is. Wouldn't it
be more sane to prevent fade-out only if the controls panel itself is hovered,
and have the same behavior in fullscreen and non-fullscreen? Preventing fade-out
for a playing video just because you leave the mouse in (say) the corner of the
video doesn't seem very useful, especially not since any mouse movement would
cause the controls to re-appear.

[eric_li, 2014-05-23 02:05:40]

On 2014/05/22 08:58:23, philipj wrote:
> The only other code to call setUserActionElement is
> core/dom/UserActionElementSet.cpp, which makes me think it probably isn't
> intended for "outside" use like in this patch.
> 
> I can't say I understand why the existing behavior is the way it is. Wouldn't
it
> be more sane to prevent fade-out only if the controls panel itself is hovered,
> and have the same behavior in fullscreen and non-fullscreen? Preventing
fade-out
> for a playing video just because you leave the mouse in (say) the corner of
the
> video doesn't seem very useful, especially not since any mouse movement would
> cause the controls to re-appear.

Actually, the control panel fade-out behavior is same in both fullscreen and
non-fullscreen.
It would fade out after 3 seconds no mouse movement, and it would be always
shown while 
hovered.
But, after switching to fullscreen or back to non-fullscreen, because switching
button in the panel
is just touched or clicked, the control panel is recorded as "Hovered", so it
would not
fade out as current logic. In fact, the control panel would move to another
position after
switching so it is not really hovered. This patch is to solve such problem, to
correct
control panel hover flag after switching.
setUserActionElement is a public method defined in Node class, it is the most
direct way to 
correct this flag.

For your first concern, does it make more sense that calling
"m_panel->setHovered(false);" instead?
setHovered is public method defined in UserActionElementSet.h.

[philipj_slow, 2014-05-23 11:24:43]

OK, so I tested in Chrome Canary and the behavior is indeed already what I
called "more sane," I was testing with Opera Next which apparently doesn't have
this change yet.

It sounds like you've found a more general bug that has consequences outside of
the media controls. I've filed a separate bug for that:
https://code.google.com/p/chromium/issues/detail?id=376728

Probably the way to fix it is to send mouse move events to the new document
coordinates when entering and exiting fullscreen, but I'm not terribly familiar
with how fullscreen or input handling works at that level.

[eric_li, 2014-05-27 03:15:04]

From my perspective, I think we should not send mouse move event
when entering/exiting fullscreen as the mouse does not actually
moved. Override mouse/touch input event might cause inconsistent
and vague behavior. It does not make sense that somewhere force
the mouse move to correct a node internal status.

The more sane approach might be: The actual moving item update
the node hover flag.
Mouse moving, mouse event update node hover flag.
Node moving, node itself update it's hover flag.

How about your opinion?

[philipj_slow, 2014-05-27 08:58:31]

The mouse has moved relative to the document, though. Synthesizing a mouse move
event may not be necessary to fix the bug, it's just something that ought to
trigger whatever code is responsible for updating the hovered state. Someone
with more experience in this area would need to provide input.

What exactly do you mean by "Node moving, node itself update it's hover flag."?
Entering and exiting fullscreen ought to amount to resizing the window, which
doesn't necessarily cause the node to move in document coordinates, although it
could.

[dominicc (has gone to gerrit), 2014-06-03 01:35:38]

-dominicc, +dglazkov

He has perfect knowledge of Shadow DOM and media controls.

[dglazkov, 2014-06-03 14:37:46]

On 2014/06/03 01:35:38, dominicc wrote:
> -dominicc, +dglazkov
> 
> He has perfect knowledge of Shadow DOM and media controls.

I have neither of those, not anymore! :)
I defer to philipj. He seems to have the handle on the problem.

[philipj_slow, 2014-06-04 08:10:51]

As I said in #5, this looks like a more general bug:
https://code.google.com/p/chromium/issues/detail?id=376728

Who's our expert on all things related to input events and hovering?

[dglazkov, 2014-06-04 14:34:17]

On 2014/06/04 08:10:51, philipj wrote:
> As I said in #5, this looks like a more general bug:
> https://code.google.com/p/chromium/issues/detail?id=376728
> 
> Who's our expert on all things related to input events and hovering?

Ah drats, I thought I was able to get away clean :P

I'll look a this later this week -- Hayato-san, if you have a chance before
then, can you PTAL?

[hayato, 2014-06-05 03:59:48]

On 2014/06/04 14:34:17, dglazkov wrote:
> On 2014/06/04 08:10:51, philipj wrote:
> > As I said in #5, this looks like a more general bug:
> > https://code.google.com/p/chromium/issues/detail?id=376728
> > 
> > Who's our expert on all things related to input events and hovering?
> 
> Ah drats, I thought I was able to get away clean :P
> 
> I'll look a this later this week -- Hayato-san, if you have a chance before
> then, can you PTAL?

I am assuming that we should focus the root cause, right?

I took a look at https://code.google.com/p/chromium/issues/detail?id=376728
I am not familiar with how fullscreen is implemented. Seems we should dig into a
bit more to resolve the root cause.

[eric_li, 2014-06-10 07:36:18]

Updated patch set 2, while HTMLMediaElement finished switching fullscreen and
non-fullscreen, update the HoverActive state by calling
document().frame()->eventHandler().scheduleHoverStateUpdate().
As philipj said in #7, using this method to update hover state makes more sense
than clearing the flag.

Please review it.

[eric_li, 2014-06-20 02:46:48]

Hi philipj and dglazkov,
   Do you have any comments on the patch set 2?

[dglazkov, 2014-06-20 04:16:44]

On 2014/06/20 at 02:46:48, jingshui.li wrote:
> Hi philipj and dglazkov,
>    Do you have any comments on the patch set 2?

Does it have to be just on <media> element. As philipj demonstrated in issue
376728, this is a specific case of a general problem. I think you're on the
right track, though! :)

[philipj_slow, 2014-06-20 21:18:17]

On 2014/06/20 04:16:44, dglazkov wrote:
> On 2014/06/20 at 02:46:48, jingshui.li wrote:
> > Hi philipj and dglazkov,
> >    Do you have any comments on the patch set 2?
> 
> Does it have to be just on <media> element. As philipj demonstrated in issue
> 376728, this is a specific case of a general problem. I think you're on the
> right track, though! :)

Yeah, how about moving the calls to a suitable location in
FullscreenElementStack? In webkitDidEnterFullScreenForElement and
webkitDidExitFullScreenForElement should work, although I'm not sure where it
should go to guarantee that the first frame after entering and exiting
fullscreen is correct, since scheduleHoverStateUpdate() is async (starts a
timer).

[eric_li, 2014-06-23 23:32:44]

Thanks for comments, I will investigate and verify it.

[philipj_slow, 2014-09-10 10:00:58]

Any update on this? Please close this review if you're not working on it.

[xun.sun, 2014-09-11 03:47:17]

On 2014/09/10 10:00:58, philipj wrote:
> Any update on this? Please close this review if you're not working on it.

We have been trying to. But we are blocked as the video does not go fullscreen
now - https://code.google.com/p/chromium/issues/detail?id=396747

[philipj_slow, 2014-09-11 10:18:04]

On 2014/09/11 03:47:17, xun.sun wrote:
> On 2014/09/10 10:00:58, philipj wrote:
> > Any update on this? Please close this review if you're not working on it.
> 
> We have been trying to. But we are blocked as the video does not go fullscreen
> now - https://code.google.com/p/chromium/issues/detail?id=396747

https://code.google.com/p/chromium/issues/detail?id=376728 , which I believe to
be the root cause, can be reproduced on desktop Chrome.

It would be nice if fullscreen could be tested in Android ContentShell,
though...

[xun.sun, 2014-09-16 12:29:34]

On 2014/09/11 10:18:04, philipj wrote:
> On 2014/09/11 03:47:17, xun.sun wrote:
> > On 2014/09/10 10:00:58, philipj wrote:
> > > Any update on this? Please close this review if you're not working on it.
> > 
> > We have been trying to. But we are blocked as the video does not go
fullscreen
> > now - https://code.google.com/p/chromium/issues/detail?id=396747
> 
> https://code.google.com/p/chromium/issues/detail?id=376728 , which I believe
to
> be the root cause, can be reproduced on desktop Chrome.
>

Thanks for the info. I'm testing a patch for this on Linux. 

> It would be nice if fullscreen could be tested in Android ContentShell,
> though...

Now if I try to make a video element fullscreen on Android, the video element
would dismiss with audio playing.

Any idea what might be wrong on Android? Since the behavior is now different
than that described in
https://code.google.com/p/chromium/issues/detail?id=396747, simply bisecting
might be ineffective.

[philipj_slow, 2014-09-16 12:33:03]

On 2014/09/16 12:29:34, xun.sun wrote:
> Now if I try to make a video element fullscreen on Android, the video element
> would dismiss with audio playing.

I'm not sure what this means. I thought fullscreen just didn't work at all in
Content Shell for Android.

> Any idea what might be wrong on Android? Since the behavior is now different
> than that described in
> https://code.google.com/p/chromium/issues/detail?id=396747, simply bisecting
> might be ineffective.

AFAIK, the problem is that the bits responsible for actually going fullscreen
aren't public, as they're integrated with Chrome for Android and Opera for
Android's respective UI layer. It would be great to pull as much as possible
into shared code, but I haven't looked at how feasible that is.

[xun.sun, 2014-09-16 13:47:10]

On 2014/09/16 12:33:03, philipj wrote:
> On 2014/09/16 12:29:34, xun.sun wrote:
> > Now if I try to make a video element fullscreen on Android, the video
element
> > would dismiss with audio playing.
> 
> I'm not sure what this means. I thought fullscreen just didn't work at all in
> Content Shell for Android.

Fullscreen is not working in ContentShell for Android. It used to work before so
this is a regression. And I am looking for hints / help to restore this.

> 
> > Any idea what might be wrong on Android? Since the behavior is now different
> > than that described in
> > https://code.google.com/p/chromium/issues/detail?id=396747, simply bisecting
> > might be ineffective.
> 
> AFAIK, the problem is that the bits responsible for actually going fullscreen
> aren't public, as they're integrated with Chrome for Android and Opera for
> Android's respective UI layer. It would be great to pull as much as possible
> into shared code, but I haven't looked at how feasible that is.

Could you clarify what this means for Content Shell for Android?

[philipj_slow, 2014-09-17 10:50:40]

On 2014/09/16 13:47:10, xun.sun wrote:
> On 2014/09/16 12:33:03, philipj wrote:
> > On 2014/09/16 12:29:34, xun.sun wrote:
> > > Now if I try to make a video element fullscreen on Android, the video
> element
> > > would dismiss with audio playing.
> > 
> > I'm not sure what this means. I thought fullscreen just didn't work at all
in
> > Content Shell for Android.
> 
> Fullscreen is not working in ContentShell for Android. It used to work before
so
> this is a regression. And I am looking for hints / help to restore this.

Oh, it used to work? I was under the impression that it had never worked. If you
know when it worked, then it's simply a matter of bisecting.

> > > Any idea what might be wrong on Android? Since the behavior is now
different
> > > than that described in
> > > https://code.google.com/p/chromium/issues/detail?id=396747, simply
bisecting
> > > might be ineffective.
> > 
> > AFAIK, the problem is that the bits responsible for actually going
fullscreen
> > aren't public, as they're integrated with Chrome for Android and Opera for
> > Android's respective UI layer. It would be great to pull as much as possible
> > into shared code, but I haven't looked at how feasible that is.
> 
> Could you clarify what this means for Content Shell for Android?

AFAIK, fullscreen doesn't work for Content Shell for Android. If it once did
work then it can presumably be fixed easily. My assumption was that there's just
a bunch of code missing, which would have to be written.

[xun.sun, 2014-10-14 15:45:17]

On 2014/09/17 10:50:40, philipj wrote:
> On 2014/09/16 13:47:10, xun.sun wrote:
> > On 2014/09/16 12:33:03, philipj wrote:
> > > On 2014/09/16 12:29:34, xun.sun wrote:
> > > > Now if I try to make a video element fullscreen on Android, the video
> > element
> > > > would dismiss with audio playing.
> > > 
> > > I'm not sure what this means. I thought fullscreen just didn't work at all
> in
> > > Content Shell for Android.
> > 
> > Fullscreen is not working in ContentShell for Android. It used to work
before
> so
> > this is a regression. And I am looking for hints / help to restore this.
> 
> Oh, it used to work? I was under the impression that it had never worked. If
you
> know when it worked, then it's simply a matter of bisecting.
> 
> > > > Any idea what might be wrong on Android? Since the behavior is now
> different
> > > > than that described in
> > > > https://code.google.com/p/chromium/issues/detail?id=396747, simply
> bisecting
> > > > might be ineffective.
> > > 
> > > AFAIK, the problem is that the bits responsible for actually going
> fullscreen
> > > aren't public, as they're integrated with Chrome for Android and Opera for
> > > Android's respective UI layer. It would be great to pull as much as
possible
> > > into shared code, but I haven't looked at how feasible that is.
> > 
> > Could you clarify what this means for Content Shell for Android?
> 
> AFAIK, fullscreen doesn't work for Content Shell for Android. If it once did
> work then it can presumably be fixed easily. My assumption was that there's
just
> a bunch of code missing, which would have to be written.

Please close this CL as it's obsoleted by
https://codereview.chromium.org/552903004/.