Update hover state after entering and leaving fullscreen.

Update hover state after entering and leaving fullscreen.

The update is scheduled after the render tree is updated and the element
entering / leaving fullscreen has completed what it needs to do.

R=philipj@opera.com
BUG=374716, 376728
Tests= fullscreen/ LayoutTests on Linux

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

[xun.sun, 2014-09-17 04:47:50]

This is a follow up of https://codereview.chromium.org/290883002 and should
obsolete 290883002. PTAL.

[philipj_slow, 2014-09-17 07:46:29]

On 2014/09/17 04:47:50, xun.sun wrote:
> This is a follow up of https://codereview.chromium.org/290883002 and should
> obsolete 290883002. PTAL.

This looks like it should fix the problem.

Please add two LayoutTests (for entering and exiting) that you've verified to
fail without this change. Since running fullscreen tests in content_shell has
the window at the same size after entering fullscreen, I think the easiest way
is to request fullscreen for an element in an iframe that will end up at
different absolute coordinates in fullscreen and non-fullscreen.

[xun.sun, 2014-09-19 02:25:31]

On 2014/09/17 07:46:29, philipj wrote:
> On 2014/09/17 04:47:50, xun.sun wrote:
> > This is a follow up of https://codereview.chromium.org/290883002 and should
> > obsolete 290883002. PTAL.
> 
> This looks like it should fix the problem.
> 
> Please add two LayoutTests (for entering and exiting) that you've verified to
> fail without this change. Since running fullscreen tests in content_shell has
> the window at the same size after entering fullscreen, I think the easiest way
> is to request fullscreen for an element in an iframe that will end up at
> different absolute coordinates in fullscreen and non-fullscreen.

Added one layouttest that covers both entering and exiting. PTAL.

[philipj_slow, 2014-09-19 14:31:02]

Test looks OK, I've also verified that it fails without the change. Apparently
no iframe tricks are needed.

Instead of copying the two resource files, can you please use the original ones?
It's OK by me to link directly to them, but if someone complains they could be
moved up to the global resources I guess.

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
File LayoutTests/fullscreen/enter-exit-full-screen-hover.html (right):

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:20: if
(!eventSender.gestureShowPress || !eventSender.gestureTap ||
!eventSender.mouseMoveTo) {
This looks like it was copy-pasted, only mouseMoveTo is needed. I would
recommend removing this entirely, just checking window.eventSender is enough.

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:31:
test("document.webkitIsFullScreen==true")
Please use shouldBe* instead of test:

-        test("document.webkitIsFullScreen==true")
+        shouldBeTrue("document.webkitIsFullScreen")
         // After entering fullscreen, the button should lose hover
-        test("getHoverActiveState(enterButton)=='default'")
+        shouldBeDefault("getHoverActiveState(enterButton)")
 
         waitForEventOnce(document, 'webkitfullscreenchange', function() {
-            test("document.webkitIsFullScreen==false")
+            shouldBeFalse("document.webkitIsFullScreen")
             // After leaving fullscreen, the button should lose hover
-            test("getHoverActiveState(exitButton)=='default'")
+            shouldBeDefault("getHoverActiveState(exitButton)")

This way, if the test fails, you get to know what the state was, not just what
it wasn't.

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:42: var center_1 =
elementCenter(exitButton);
How about enterButtonCenter and exitButtonCenter instead?

[xun.sun, 2014-09-20 17:39:17]

On 2014/09/19 14:31:02, philipj wrote:
> Test looks OK, I've also verified that it fails without the change. Apparently
> no iframe tricks are needed.
> 
> Instead of copying the two resource files, can you please use the original
ones?
> It's OK by me to link directly to them, but if someone complains they could be
> moved up to the global resources I guess.

Done.

> 
>
https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
> File LayoutTests/fullscreen/enter-exit-full-screen-hover.html (right):
> 
>
https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
> LayoutTests/fullscreen/enter-exit-full-screen-hover.html:20: if
> (!eventSender.gestureShowPress || !eventSender.gestureTap ||
> !eventSender.mouseMoveTo) {
> This looks like it was copy-pasted, only mouseMoveTo is needed. I would
> recommend removing this entirely, just checking window.eventSender is enough.
> 
>
https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
> LayoutTests/fullscreen/enter-exit-full-screen-hover.html:31:
> test("document.webkitIsFullScreen==true")
> Please use shouldBe* instead of test:
> 
> -        test("document.webkitIsFullScreen==true")
> +        shouldBeTrue("document.webkitIsFullScreen")
>          // After entering fullscreen, the button should lose hover
> -        test("getHoverActiveState(enterButton)=='default'")
> +        shouldBeDefault("getHoverActiveState(enterButton)")
>  
>          waitForEventOnce(document, 'webkitfullscreenchange', function() {
> -            test("document.webkitIsFullScreen==false")
> +            shouldBeFalse("document.webkitIsFullScreen")
>              // After leaving fullscreen, the button should lose hover
> -            test("getHoverActiveState(exitButton)=='default'")
> +            shouldBeDefault("getHoverActiveState(exitButton)")
> 
> This way, if the test fails, you get to know what the state was, not just what
> it wasn't.
> 
>
https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
> LayoutTests/fullscreen/enter-exit-full-screen-hover.html:42: var center_1 =
> elementCenter(exitButton);
> How about enterButtonCenter and exitButtonCenter instead?

Thanks for your suggestions. PTA3L.

[xun.sun, 2014-09-20 17:39:48]

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
File LayoutTests/fullscreen/enter-exit-full-screen-hover.html (right):

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:20: if
(!eventSender.gestureShowPress || !eventSender.gestureTap ||
!eventSender.mouseMoveTo) {
On 2014/09/19 14:31:01, philipj wrote:
> This looks like it was copy-pasted, only mouseMoveTo is needed. I would
> recommend removing this entirely, just checking window.eventSender is enough.

Done.

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:31:
test("document.webkitIsFullScreen==true")
On 2014/09/19 14:31:02, philipj wrote:
> Please use shouldBe* instead of test:
> 
> -        test("document.webkitIsFullScreen==true")
> +        shouldBeTrue("document.webkitIsFullScreen")
>          // After entering fullscreen, the button should lose hover
> -        test("getHoverActiveState(enterButton)=='default'")
> +        shouldBeDefault("getHoverActiveState(enterButton)")
>  
>          waitForEventOnce(document, 'webkitfullscreenchange', function() {
> -            test("document.webkitIsFullScreen==false")
> +            shouldBeFalse("document.webkitIsFullScreen")
>              // After leaving fullscreen, the button should lose hover
> -            test("getHoverActiveState(exitButton)=='default'")
> +            shouldBeDefault("getHoverActiveState(exitButton)")
> 
> This way, if the test fails, you get to know what the state was, not just what
> it wasn't.

Done.

https://codereview.chromium.org/552903004/diff/20001/LayoutTests/fullscreen/e...
LayoutTests/fullscreen/enter-exit-full-screen-hover.html:42: var center_1 =
elementCenter(exitButton);
On 2014/09/19 14:31:02, philipj wrote:
> How about enterButtonCenter and exitButtonCenter instead?

Done.

[philipj_slow, 2014-09-20 21:36:28]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2014-09-20 21:36:28]

lgtm

Thanks for fixing this!

[commit-bot: I haz the power, 2014-09-20 21:36:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/552903004/40001

[commit-bot: I haz the power, 2014-09-20 22:41:04]

Committed patchset #3 (id:40001) as 182373