Wait until after layout when restoring scroll on exiting fullscreen.

Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
Committed: https://crrev.com/d3fb3a358ce9c85bfd7bcda882a23179ac064775
Cr-Commit-Position: refs/heads/master@{#405854}

[bokan, 2016-07-05 14:31:29]

Description was changed from

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
==========

to

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
==========

[bokan, 2016-07-05 14:31:29]

bokan@chromium.org changed reviewers:
+ trchen@chromium.org

[bokan, 2016-07-05 14:31:54]

ptal

[bokan, 2016-07-07 16:22:14]

friendly ping

[bokan, 2016-07-12 16:42:27]

ping, trchen@, could you please take a look?

[trchen, 2016-07-12 21:11:53]

Ah oh, my apologies. Sorry I didn't see your previous messages.

I'd worry about race conditions. What happens if we have a script that requested
exiting then immediately entering fullscreen again? Wouldn't the second enter
fullscreen grab the un-restored scroll and scale?

How about we add an extra check at both FullscreenController::didUpdateLayout
and FullscreenController::enterFullScreenForElement, so we restore scroll state
only when the fullscreen state is not fullscreen, and also save scroll state
only when there is no previously saved scroll state?

[bokan, 2016-07-13 15:17:21]

On 2016/07/12 21:11:53, trchen wrote:
> Ah oh, my apologies. Sorry I didn't see your previous messages.
> 
> I'd worry about race conditions. What happens if we have a script that
requested
> exiting then immediately entering fullscreen again? Wouldn't the second enter
> fullscreen grab the un-restored scroll and scale?
> 
> How about we add an extra check at both FullscreenController::didUpdateLayout
> and FullscreenController::enterFullScreenForElement, so we restore scroll
state
> only when the fullscreen state is not fullscreen, and also save scroll state
> only when there is no previously saved scroll state?

Good catch, I hadn't thought of that! Made your suggested fixes and added a test
for that case. PTAL.

[bokan, 2016-07-14 22:17:53]

ping

[trchen, 2016-07-14 22:24:59]

Thanks! This looks great! lgtm

[trchen, 2016-07-14 22:25:54]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:26:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 22:49:09]

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

[commit-bot: I haz the power, 2016-07-14 22:49:10]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-07-15 15:45:54]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-07-15 15:45:54]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2122013003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-15 15:46:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 17:03:40]

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

[commit-bot: I haz the power, 2016-07-15 17:03:41]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-07-15 19:03:18]

After
https://chromium.googlesource.com/chromium/src/+/d666f503ae854fac360cc70da1f5...,
I had to add didExitFullscreenForElement() in the tests to get the layout to
take effect properly after exiting fullscreen. +cc foolip@ as FYI/double-check.

[bokan, 2016-07-15 19:03:41]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-07-15 19:03:41]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2122013003/#ps120001
(title: "Fix tests after foolip@'s patch")

[commit-bot: I haz the power, 2016-07-15 19:04:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 20:54:17]

Description was changed from

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
==========

to

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
==========

[commit-bot: I haz the power, 2016-07-15 20:54:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-15 20:56:56]

Description was changed from

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683
==========

to

==========
Wait until after layout when restoring scroll on exiting fullscreen.

Many pages can change while in fullscreen. For example, youtube applies
display:none to elements not in the fullscreen ancestor chain using CSS
selectors. This means that until we update style and layout, the page will not
be scrollable to the same extent (or at all) as it was when we entered
fullscreen.

The solution is to take note when we exit fullscreen and try to restore the
scale and scroll once the first layout after exiting fullscreen is completed.
This ensures the page is completely back to it's pre-fullscreen state.

BUG=625683

Committed: https://crrev.com/d3fb3a358ce9c85bfd7bcda882a23179ac064775
Cr-Commit-Position: refs/heads/master@{#405854}
==========

[commit-bot: I haz the power, 2016-07-15 20:56:57]

Patchset 7 (id:??) landed as
https://crrev.com/d3fb3a358ce9c85bfd7bcda882a23179ac064775
Cr-Commit-Position: refs/heads/master@{#405854}

[foolip, 2016-07-21 04:27:35]

On 2016/07/15 19:03:18, bokan wrote:
> After
>
https://chromium.googlesource.com/chromium/src/+/d666f503ae854fac360cc70da1f5...,
> I had to add didExitFullscreenForElement() in the tests to get the layout to
> take effect properly after exiting fullscreen. +cc foolip@ as
FYI/double-check.

I think the reason this is needed is related to the TODO in
FullscreenController::exitFullScreenForElement. If there were only a single way
to enter and exit fullscreen, then this test could use that way as well, but
what you have makes sense now.