Restore scrolloffset after exiting fullscreen.

Restore scrolloffset after exiting fullscreen.

Page scale factor in full screen mode can be different from the normal
mode. Right after exiting fullscreen, the page scale factor will be
stale, because it remains the value of full screen mode until quite a
few ondraws later.

However, WebView will try to restore to the normal mode scroll offset
immediately after exiting fullscreen. So WebView will be temporarily
restored to a wrong scroll offset in CSS pixel.

This patch will try to restore to the right location when page scale
factor changes back to the initial value in normal mode.

BUG=558045

[hush (inactive), 2015-12-30 23:19:21]

hush@chromium.org changed reviewers:
+ boliu@chromium.org

[hush (inactive), 2015-12-30 23:19:21]

Hi Bo,
PTAL!
This will solve the case of scrolloffset jumping around after exiting
fullscreen, if it is caused by differences in page scale factor.

However, there are other cases:
a bad activation after the page scale factor has been restored might change the
scroll offset to something way off. But that's likely a problem in cc, because I
see the same problem in chrome as well.

[hush (inactive), 2015-12-30 23:20:10]

changes in browser view renderer are mechanical. They are name changes

[boliu, 2015-12-30 23:25:02]

Question about description:

> However, WebView will try to restore to the normal mode scroll offset
immediately after exiting fullscreen.

where do we do that?

[hush (inactive), 2015-12-30 23:30:04]

On 2015/12/30 23:25:02, boliu wrote:
> Question about description:
> 
> > However, WebView will try to restore to the normal mode scroll offset
> immediately after exiting fullscreen.
> 
> where do we do that?

We just do it in the next onDraw that follows the exitFullscreen().
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

[boliu, 2015-12-31 00:07:03]

Let's see if I understand the fix correctly..

So let's say the two views are WebView and FullScreenView.

when exiting full screen, you are restoring the WebView scroll offset from
AwContents.java, but *not* the WebView page scale, right? Then how did WebView
get the wrong scroll offset in the first place? The wrong offset must have came
from BVR at some point after exiting full screen? Where did that happen exactly?

[hush (inactive), 2015-12-31 00:24:51]

On 2015/12/31 00:07:03, boliu wrote:
> Let's see if I understand the fix correctly..
> 
> So let's say the two views are WebView and FullScreenView.
> 
> when exiting full screen, you are restoring the WebView scroll offset from
> AwContents.java, but *not* the WebView page scale, right? Then how did WebView
> get the wrong scroll offset in the first place? The wrong offset must have
came
> from BVR at some point after exiting full screen? Where did that happen
exactly?

When exiting fullscreen, AwContents.java will restore the scroll offset to the
*correct* physical pixel. But at this time, the page scale factor value in
browser_view_renderer is stale. So browser_view_renderer will be restored a
wrong CSS pixel scroll offset.

AwContents will continue to think the native side is in the correct offset
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

That is the case until browser_view_renderer finally receives an activation
containing the correct value of page scale factor. But at this point
browser_view_renderer is already at a wrong CSS pixel scroll offset, it will
calculate a wrong physical pixel value and tell AwContents to scroll to that
wrong offset via client_->ScrollContainerViewTo. 

The fix works by remembering what the initial scroll offset in physical pixel
is, and restore to it when AwContents/BrowserViewRenderer receives the correct
value of page scale factor through activation.

[boliu, 2015-12-31 00:34:37]

On 2015/12/31 00:24:51, hush wrote:
> On 2015/12/31 00:07:03, boliu wrote:
> > Let's see if I understand the fix correctly..
> > 
> > So let's say the two views are WebView and FullScreenView.
> > 
> > when exiting full screen, you are restoring the WebView scroll offset from
> > AwContents.java, but *not* the WebView page scale, right? Then how did
WebView
> > get the wrong scroll offset in the first place? The wrong offset must have
> came
> > from BVR at some point after exiting full screen? Where did that happen
> exactly?
> 
> When exiting fullscreen, AwContents.java will restore the scroll offset to the
> *correct* physical pixel. But at this time, the page scale factor value in
> browser_view_renderer is stale. So browser_view_renderer will be restored a
> wrong CSS pixel scroll offset.

This would be in the first onDraw after exitFullScreen mentioned above?

> 
> AwContents will continue to think the native side is in the correct offset
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> 
> That is the case until browser_view_renderer finally receives an activation
> containing the correct value of page scale factor. But at this point
> browser_view_renderer is already at a wrong CSS pixel scroll offset, it will
> calculate a wrong physical pixel value and tell AwContents to scroll to that
> wrong offset via client_->ScrollContainerViewTo. 
> 
> The fix works by remembering what the initial scroll offset in physical pixel
> is, and restore to it when AwContents/BrowserViewRenderer receives the correct
> value of page scale factor through activation.

Ohh, so you re-restore the correct physical scroll offset, after the next page
scale change? So you are assuming the next setPageScaleFactorAndLimits call
corresponds to this activation with the correct page scale?

[hush (inactive), 2015-12-31 00:39:09]

> This would be in the first onDraw after exitFullScreen mentioned above?
yes.

> Ohh, so you re-restore the correct physical scroll offset, after the next page
scale change?
yes.
> So you are assuming the next setPageScaleFactorAndLimits call
corresponds to this activation with the correct page scale?
Sorry, I made a mistake here in the code. I don't actually assume the next
setPageScaleFactorAndLimits call will contain the correct page scale. I should
have compared the incoming page scale with the "mInitialPageScaleFactor" stored
in the transition state.
The assumption is that we will get a bunch of setPageScaleFactorAndLimits after
exiting fullscreen, and eventually there will be a setPageScaleFactorAndLimits
that sets the page scale to the original scale before fullscreen.

[boliu, 2015-12-31 00:42:47]

On 2015/12/31 00:39:09, hush wrote:
> > This would be in the first onDraw after exitFullScreen mentioned above?
> yes.
> 
> > Ohh, so you re-restore the correct physical scroll offset, after the next
page
> scale change?
> yes.
> > So you are assuming the next setPageScaleFactorAndLimits call
> corresponds to this activation with the correct page scale?
> Sorry, I made a mistake here in the code. I don't actually assume the next
> setPageScaleFactorAndLimits call will contain the correct page scale. I should
> have compared the incoming page scale with the "mInitialPageScaleFactor"
stored
> in the transition state.
> The assumption is that we will get a bunch of setPageScaleFactorAndLimits
after
> exiting fullscreen, and eventually there will be a setPageScaleFactorAndLimits
> that sets the page scale to the original scale before fullscreen.

I don't think that's ever gonna work reliably..

Feels like we have to just restore the old page scale synchronously inside
exitFullScreen, then the scroll restore in the first onDraw will converted to
the correct css offset

[hush (inactive), 2015-12-31 00:43:13]

Updated ps3 based on what we just discussed

[hush (inactive), 2015-12-31 00:48:20]

On 2015/12/31 00:42:47, boliu wrote:
> On 2015/12/31 00:39:09, hush wrote:
> > > This would be in the first onDraw after exitFullScreen mentioned above?
> > yes.
> > 
> > > Ohh, so you re-restore the correct physical scroll offset, after the next
> page
> > scale change?
> > yes.
> > > So you are assuming the next setPageScaleFactorAndLimits call
> > corresponds to this activation with the correct page scale?
> > Sorry, I made a mistake here in the code. I don't actually assume the next
> > setPageScaleFactorAndLimits call will contain the correct page scale. I
should
> > have compared the incoming page scale with the "mInitialPageScaleFactor"
> stored
> > in the transition state.
> > The assumption is that we will get a bunch of setPageScaleFactorAndLimits
> after
> > exiting fullscreen, and eventually there will be a
setPageScaleFactorAndLimits
> > that sets the page scale to the original scale before fullscreen.
> 
> I don't think that's ever gonna work reliably..
> 
> Feels like we have to just restore the old page scale synchronously inside
> exitFullScreen, then the scroll restore in the first onDraw will converted to
> the correct css offset

There will be several activations on BrowserViewRenderer containing the stale
page scale factor, after exit fullscreen.

So if AwContents immediately sets the initial page scale factor to
BrowserViewRenderer duing exitFullScreen, this value will be overwritten to a
wrong value again, by another activation.
That means BrowserViewRenderer has to somehow know the page scale factor from
the activation is wrong and ignore it until it is right. And this further means
BrowserViewRenderer has to know something about fullscreen transitions to make
that ignore decision. I think it is too much for BVR to know. That's why I
didn't choose to do this initially.

[boliu, 2015-12-31 00:55:24]

On 2015/12/31 00:48:20, hush wrote:
> On 2015/12/31 00:42:47, boliu wrote:
> > On 2015/12/31 00:39:09, hush wrote:
> > > > This would be in the first onDraw after exitFullScreen mentioned above?
> > > yes.
> > > 
> > > > Ohh, so you re-restore the correct physical scroll offset, after the
next
> > page
> > > scale change?
> > > yes.
> > > > So you are assuming the next setPageScaleFactorAndLimits call
> > > corresponds to this activation with the correct page scale?
> > > Sorry, I made a mistake here in the code. I don't actually assume the next
> > > setPageScaleFactorAndLimits call will contain the correct page scale. I
> should
> > > have compared the incoming page scale with the "mInitialPageScaleFactor"
> > stored
> > > in the transition state.
> > > The assumption is that we will get a bunch of setPageScaleFactorAndLimits
> > after
> > > exiting fullscreen, and eventually there will be a
> setPageScaleFactorAndLimits
> > > that sets the page scale to the original scale before fullscreen.
> > 
> > I don't think that's ever gonna work reliably..
> > 
> > Feels like we have to just restore the old page scale synchronously inside
> > exitFullScreen, then the scroll restore in the first onDraw will converted
to
> > the correct css offset
> 
> There will be several activations on BrowserViewRenderer containing the stale
> page scale factor, after exit fullscreen.

It needs to be plumbed all the way to cc impl-side? Would be similar to
ContentViewCore.pinchByDelta, but guaranteed to be synchronous in the webview
sense.

Otherwise, I don't see any way to do this reliably. How bad is the issue in
practice?

> 
> So if AwContents immediately sets the initial page scale factor to
> BrowserViewRenderer duing exitFullScreen, this value will be overwritten to a
> wrong value again, by another activation.
> That means BrowserViewRenderer has to somehow know the page scale factor from
> the activation is wrong and ignore it until it is right. And this further
means
> BrowserViewRenderer has to know something about fullscreen transitions to make
> that ignore decision. I think it is too much for BVR to know. That's why I
> didn't choose to do this initially.

[boliu, 2015-12-31 00:59:31]

Another thought. What if onDraw didn't force sync the container view scroll
offset? Blink itself must restore to the old state scroll/scale, if we don't
mess around with scrolls before it's done that?

[hush (inactive), 2015-12-31 01:11:14]

On 2015/12/31 00:55:24, boliu wrote:
> On 2015/12/31 00:48:20, hush wrote:
> > On 2015/12/31 00:42:47, boliu wrote:
> > > On 2015/12/31 00:39:09, hush wrote:
> > > > > This would be in the first onDraw after exitFullScreen mentioned
above?
> > > > yes.
> > > > 
> > > > > Ohh, so you re-restore the correct physical scroll offset, after the
> next
> > > page
> > > > scale change?
> > > > yes.
> > > > > So you are assuming the next setPageScaleFactorAndLimits call
> > > > corresponds to this activation with the correct page scale?
> > > > Sorry, I made a mistake here in the code. I don't actually assume the
next
> > > > setPageScaleFactorAndLimits call will contain the correct page scale. I
> > should
> > > > have compared the incoming page scale with the "mInitialPageScaleFactor"
> > > stored
> > > > in the transition state.
> > > > The assumption is that we will get a bunch of
setPageScaleFactorAndLimits
> > > after
> > > > exiting fullscreen, and eventually there will be a
> > setPageScaleFactorAndLimits
> > > > that sets the page scale to the original scale before fullscreen.
> > > 
> > > I don't think that's ever gonna work reliably..
> > > 
> > > Feels like we have to just restore the old page scale synchronously inside
> > > exitFullScreen, then the scroll restore in the first onDraw will converted
> to
> > > the correct css offset
> > 
> > There will be several activations on BrowserViewRenderer containing the
stale
> > page scale factor, after exit fullscreen.
> 
> It needs to be plumbed all the way to cc impl-side? Would be similar to
> ContentViewCore.pinchByDelta, but guaranteed to be synchronous in the webview
> sense.
> 
> Otherwise, I don't see any way to do this reliably. How bad is the issue in
> practice?
> 
> > 
> > So if AwContents immediately sets the initial page scale factor to
> > BrowserViewRenderer duing exitFullScreen, this value will be overwritten to
a
> > wrong value again, by another activation.
> > That means BrowserViewRenderer has to somehow know the page scale factor
from
> > the activation is wrong and ignore it until it is right. And this further
> means
> > BrowserViewRenderer has to know something about fullscreen transitions to
make
> > that ignore decision. I think it is too much for BVR to know. That's why I
> > didn't choose to do this initially.

The thing is even if I plumb the page scale factor on the cc-impl side, I'm not
sure if this works eventually. Will this value on impl side stick when there are
multiple commits from blink with a different page scale factor?

In practice, the user almost never get to the correct location after exiting
full screen, whether it is chrome or a webview app.
You can try loading youtube mobile site on Chrome, or this site:
http://www.quirksmode.org/html5/tests/video.html

[boliu, 2015-12-31 01:15:53]

On 2015/12/31 01:11:14, hush wrote:
> On 2015/12/31 00:55:24, boliu wrote:
> > On 2015/12/31 00:48:20, hush wrote:
> > > On 2015/12/31 00:42:47, boliu wrote:
> > > > On 2015/12/31 00:39:09, hush wrote:
> > > > > > This would be in the first onDraw after exitFullScreen mentioned
> above?
> > > > > yes.
> > > > > 
> > > > > > Ohh, so you re-restore the correct physical scroll offset, after the
> > next
> > > > page
> > > > > scale change?
> > > > > yes.
> > > > > > So you are assuming the next setPageScaleFactorAndLimits call
> > > > > corresponds to this activation with the correct page scale?
> > > > > Sorry, I made a mistake here in the code. I don't actually assume the
> next
> > > > > setPageScaleFactorAndLimits call will contain the correct page scale.
I
> > > should
> > > > > have compared the incoming page scale with the
"mInitialPageScaleFactor"
> > > > stored
> > > > > in the transition state.
> > > > > The assumption is that we will get a bunch of
> setPageScaleFactorAndLimits
> > > > after
> > > > > exiting fullscreen, and eventually there will be a
> > > setPageScaleFactorAndLimits
> > > > > that sets the page scale to the original scale before fullscreen.
> > > > 
> > > > I don't think that's ever gonna work reliably..
> > > > 
> > > > Feels like we have to just restore the old page scale synchronously
inside
> > > > exitFullScreen, then the scroll restore in the first onDraw will
converted
> > to
> > > > the correct css offset
> > > 
> > > There will be several activations on BrowserViewRenderer containing the
> stale
> > > page scale factor, after exit fullscreen.
> > 
> > It needs to be plumbed all the way to cc impl-side? Would be similar to
> > ContentViewCore.pinchByDelta, but guaranteed to be synchronous in the
webview
> > sense.
> > 
> > Otherwise, I don't see any way to do this reliably. How bad is the issue in
> > practice?
> > 
> > > 
> > > So if AwContents immediately sets the initial page scale factor to
> > > BrowserViewRenderer duing exitFullScreen, this value will be overwritten
to
> a
> > > wrong value again, by another activation.
> > > That means BrowserViewRenderer has to somehow know the page scale factor
> from
> > > the activation is wrong and ignore it until it is right. And this further
> > means
> > > BrowserViewRenderer has to know something about fullscreen transitions to
> make
> > > that ignore decision. I think it is too much for BVR to know. That's why I
> > > didn't choose to do this initially.
> 
> The thing is even if I plumb the page scale factor on the cc-impl side, I'm
not
> sure if this works eventually. Will this value on impl side stick when there
are
> multiple commits from blink with a different page scale factor?
> 
> In practice, the user almost never get to the correct location after exiting
> full screen, whether it is chrome or a webview app.
> You can try loading youtube mobile site on Chrome, or this site:
> http://www.quirksmode.org/html5/tests/video.html

Oh doesn't even work in chrome? Then all hope is lost..

I mean have to make sure blink does the right thing first before fixing webview.
This sort of hacky patch-up fix is just going to cause more problems when it
doesn't work.

[hush (inactive), 2015-12-31 01:21:41]

On 2015/12/31 01:15:53, boliu wrote:
> On 2015/12/31 01:11:14, hush wrote:
> > On 2015/12/31 00:55:24, boliu wrote:
> > > On 2015/12/31 00:48:20, hush wrote:
> > > > On 2015/12/31 00:42:47, boliu wrote:
> > > > > On 2015/12/31 00:39:09, hush wrote:
> > > > > > > This would be in the first onDraw after exitFullScreen mentioned
> > above?
> > > > > > yes.
> > > > > > 
> > > > > > > Ohh, so you re-restore the correct physical scroll offset, after
the
> > > next
> > > > > page
> > > > > > scale change?
> > > > > > yes.
> > > > > > > So you are assuming the next setPageScaleFactorAndLimits call
> > > > > > corresponds to this activation with the correct page scale?
> > > > > > Sorry, I made a mistake here in the code. I don't actually assume
the
> > next
> > > > > > setPageScaleFactorAndLimits call will contain the correct page
scale.
> I
> > > > should
> > > > > > have compared the incoming page scale with the
> "mInitialPageScaleFactor"
> > > > > stored
> > > > > > in the transition state.
> > > > > > The assumption is that we will get a bunch of
> > setPageScaleFactorAndLimits
> > > > > after
> > > > > > exiting fullscreen, and eventually there will be a
> > > > setPageScaleFactorAndLimits
> > > > > > that sets the page scale to the original scale before fullscreen.
> > > > > 
> > > > > I don't think that's ever gonna work reliably..
> > > > > 
> > > > > Feels like we have to just restore the old page scale synchronously
> inside
> > > > > exitFullScreen, then the scroll restore in the first onDraw will
> converted
> > > to
> > > > > the correct css offset
> > > > 
> > > > There will be several activations on BrowserViewRenderer containing the
> > stale
> > > > page scale factor, after exit fullscreen.
> > > 
> > > It needs to be plumbed all the way to cc impl-side? Would be similar to
> > > ContentViewCore.pinchByDelta, but guaranteed to be synchronous in the
> webview
> > > sense.
> > > 
> > > Otherwise, I don't see any way to do this reliably. How bad is the issue
in
> > > practice?
> > > 
> > > > 
> > > > So if AwContents immediately sets the initial page scale factor to
> > > > BrowserViewRenderer duing exitFullScreen, this value will be overwritten
> to
> > a
> > > > wrong value again, by another activation.
> > > > That means BrowserViewRenderer has to somehow know the page scale factor
> > from
> > > > the activation is wrong and ignore it until it is right. And this
further
> > > means
> > > > BrowserViewRenderer has to know something about fullscreen transitions
to
> > make
> > > > that ignore decision. I think it is too much for BVR to know. That's why
I
> > > > didn't choose to do this initially.
> > 
> > The thing is even if I plumb the page scale factor on the cc-impl side, I'm
> not
> > sure if this works eventually. Will this value on impl side stick when there
> are
> > multiple commits from blink with a different page scale factor?
> > 
> > In practice, the user almost never get to the correct location after exiting
> > full screen, whether it is chrome or a webview app.
> > You can try loading youtube mobile site on Chrome, or this site:
> > http://www.quirksmode.org/html5/tests/video.html
> 
> Oh doesn't even work in chrome? Then all hope is lost..
> 
> I mean have to make sure blink does the right thing first before fixing
webview.
> This sort of hacky patch-up fix is just going to cause more problems when it
> doesn't work.

Yeah, try Chrome stable and you will see. Haha...