1586986 - Make VisualViewport size behavior match Chrome's

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

On Chrome VisualViewport size is changed by the dynamic toolbar transitions, which means the height is including the dynamic toolbar height if the toolbar is visible.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

In our current implementation with CoordinatorLayout which seems the way we implemented the dynamic toolbar VisualViewport is including the toolbar height, the size comes from surfaceChanged callback in GeckoView. Whereas, if GeckoView is not a child of CoordinatorLayout, the size from surfaceChanged doesn't include the toolbar height.

:snorp or Sebastian, using CoordinatorLayout is the way to go for the dynamic toolbar? If so, we can assume the size comes from surfaceChanged is including the toolbar. (But we need a way to tell whether it's dynamic or static). Also I am wondering how Chrome implements the dynamic toolbar in their ChromeView.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

:snorp or Sebastian, using CoordinatorLayout is the way to go for the dynamic toolbar? If so, we can assume the size comes from surfaceChanged is including the toolbar. (But we need a way to tell whether it's dynamic or static). Also I am wondering how Chrome implements the dynamic toolbar in their ChromeView.

Probably, in such cases GeckoView should call onBoundsChanged with the size reducing the toolbar height as we've been doing for Fennec.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

:snorp or Sebastian, using CoordinatorLayout is the way to go for the dynamic toolbar?

This is definitely the default Android way of doing this. If we could make this work then this would mean less code for us to maintain and it would make it easier for other GeckoView-based apps to implement this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you, Sebastian. Then do we have a plan to use CoordinatorLayout in gecko view example and also in TestRunnerActivity? Though CoordinatorLayout was introduced in SDK 24 (IIRC), those usages are for internal test purpose, so it doesn't matter at all?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I've found a way to write a JUnit test for this, and it works locally. Let's see it on try;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44cd772f750b4e17c49287cfd3d0982315971df5

Comment 6

[Hiroyuki Ikezoe (:hiro)]

It seems a dead lock happens on SessionLifecycleTest.readFromParcel_closeOpenAndLoad and others.

:snorp, we need to send fixed bottom offset to both the main-thread and the compositor thread. Changing dispatchTo = "current" to "gecko" is a proper way to do it? And LayerViewSupport::SetFixedBottomOffSet will looks like this;

  void SetFixedBottomOffset(int32_t aOffset) {                                  
    if (mWindow) {                                                              
      mWindow->UpdateDynamicToolbarOffset(ScreenIntCoord(aOffset));             
    }                                                                           
                                                                                
    if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) {                     
      uiThread->Dispatch(NS_NewRunnableFunction(                                
          "LayerViewSupport::SetFixedBottomOffset", [this, offset = aOffset] {  
            if (RefPtr<UiCompositorControllerChild> child =                     
                    GetUiCompositorControllerChild()) {                         
              child->SetFixedBottomOffset(offset);                              
            }                                                                   
          }));                                                                  
    }                                                                           
  } 

It seems to work locally, but to be honest, I don't quite understand what the difference between Java UI thread and Android UI thread is.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

  void SetFixedBottomOffset(int32_t aOffset) {                                  
    if (mWindow) {                                                              
      mWindow->UpdateDynamicToolbarOffset(ScreenIntCoord(aOffset));             
    }                                                                           

Probably we need to use LockedWindowPtr here?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: An examle which shows us there will be inconsistency between Chrome and Firefox

This bug is more complicated than I thought.

On Chrome, visual viewport events are fired during dynamic toolbar transitions and visual viewport height is also changed in response to the dynamic toolbar transitions. Whereas percentage based value on position:fixed elements is not affected by the visual viewport change at all. Attaching file is an example which flushes a position:fixed element whose height is 99% in the callback of the visual viewport events (the element is the right most gray-ish one).

On Firefox the height of the position:fixed element will be changed because we will use the up-to-date visual viewport size, thus the position:fixed element size will grow with the dynamic toolbar changes.

I guess this visual inconsistency would be acceptable, but it makes me realize that Chrome allows that the visual viewport size is bigger than the layout viewport at the moment of the dynamic toolbar transitions. And for us more cumbersome thing is that using the up-to-date visual viewport size for position:fixed elements means we also need to tweak mFixedLayerMargins value on the compositor because mFixedLayerMargins is based on the value starting from the toolbar is completely visible, not in the middle of transitions, but how we can tell whether there were flushes or not.

I have no idea to solve the issue yet, but I am kind of inclined to not use the up-to-date visual viewport size for layout just like Chrome does. But even with the way, the mFixedLayerMargins value would be an issue at the end of the transition because we need to update percentage units on position:fixed elements in the end (bug 1586149).

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

It seems a dead lock happens on SessionLifecycleTest.readFromParcel_closeOpenAndLoad and others.

:snorp, we need to send fixed bottom offset to both the main-thread and the compositor thread. Changing dispatchTo = "current" to "gecko" is a proper way to do it?

That method is currently called on the Android UI (or "main") thread, and dispatchTo = "current" means it is received on that same thread in native code. If you change that to "gecko" then it will be proxied over to the Gecko main thread automatically. This sounds like it may be what you want.

And LayerViewSupport::SetFixedBottomOffSet will looks like this;

  void SetFixedBottomOffset(int32_t aOffset) {                                  
    if (mWindow) {                                                              
      mWindow->UpdateDynamicToolbarOffset(ScreenIntCoord(aOffset));             
    }                                                                           
                                                                                
    if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) {                     
      uiThread->Dispatch(NS_NewRunnableFunction(                                
          "LayerViewSupport::SetFixedBottomOffset", [this, offset = aOffset] {  
            if (RefPtr<UiCompositorControllerChild> child =                     
                    GetUiCompositorControllerChild()) {                         
              child->SetFixedBottomOffset(offset);                              
            }                                                                   
          }));                                                                  
    }                                                                           
  } 

This will call LayerViewSupport::SetFixedBottomOffseton the Android UI thread, which is not the same as the Compositor thread. The Android UI thread is used as the "controller" thread for APZ, though, so maybe that's what you want? It's also equivalent to the current behavior since we're being called on the Android UI thread as explained above.

It seems to work locally, but to be honest, I don't quite understand what the difference between Java UI thread and Android UI thread is.

They're the same. You may also see it referred to as the Android main thread (it shows up as "main" in the debugger).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you snorp!

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #9)

It seems to work locally, but to be honest, I don't quite understand what the difference between Java UI thread and Android UI thread is.

They're the same. You may also see it referred to as the Android main thread (it shows up as "main" in the debugger).

That's really good to know. Without knowing it, I will be horribly confused by the "main". :)

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I've decided to fix bug 1586147 and bug 1586149 altogether in this bug. Otherwise we will introduce undesirable weird behaviors.

There are two caveats with regard to visual viewport size changes;

1. Firing visual viewport resize events means that any arbitrary scripts can be run in the callback
   so even if we don't explicitly flush position:fixed elements styles AND if we use the up-to-date
   visual viewport size in layout, it's possible that position:fixed elements positions are changed
   by unpredictable scripts. So if we update the visual viewport size in layout, we should also explicitly
   flush positions:fixed elements.

2. As a result of 1), once position:fixed elements are re-positioned with the up-to-date visual viewport size
   by the flush, 'fixed layer margins' on the compositor which comes from android-components
   via setVerticalClipping is no longer valid as it is. We need to negate the value by the current
   dynamic toolbar height.

I hope this helps reviewers to review patches here.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

2. As a result of 1), once position:fixed elements are re-positioned with the up-to-date visual viewport size
   by the flush, 'fixed layer margins' on the compositor which comes from android-components
   via setVerticalClipping is no longer valid as it is. We need to negate the value by the current
   dynamic toolbar height.

oops. correction: 'by the difference of the current toolbar height and the max toolbar height'.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586986 - Deliver 'fixed-bottom' offset to the top of the pres context on the foreground tab. r?#geckoview-reviewers,tnikkel

The dynamic toolbar transition doesn't affect on background tabs since to
switch tabs the dynamic toolbar should be restored to its original state (i.e.,
completely visible state).

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586986 - Introduce 'fixed margins' on the main-thread to omit the gap between 'fixed margins' on the compositor and the last dynamic toolbar position on the main-thread. r?botond

The gap is caused by the difference between 'fixed margin offset' on the
compositor based on the state where the dynamic toolbar is completely visible
and position:fixed elements positioned by the time where is partially visible
or completely hidden. That's because the 'fixed margin offset' is computed
based on the bottom of the ICB whereas position:fixed elements are slightly
shifted from the ICB edge during the toolbar transition.

Depends on D52336

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586986 - Fire visual viewport resize events and flush position:fixed elements' layout in the same way what Chrome does. r?botond

On Chrome, visual viewport resize event is fired repeatedly during dynamic
toolbar transitions and visual viewport height obtained by the VisualViewport
API is also changed, but in terms of layout the height value is never used
until the dynamic toolbar height reaches to zero or is changed from zero.
The height used at the time is the height for vh units when the toolbar height
reaches to zero and the ICB height when the toolbar height is changed from zero.
To do so, we need to have another visual viewport size in parallel to the
original one and use them depending on situations.

Depends on D52337

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d6a8573e09b
Deliver 'fixed-bottom' offset to the top of the pres context on the foreground tab. r=geckoview-reviewers,tnikkel,snorp
https://hg.mozilla.org/integration/autoland/rev/0ff21dbde947
Introduce 'fixed margins' on the main-thread to omit the gap between 'fixed margins' on the compositor and the last dynamic toolbar position on the main-thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/086f87f26b1f
Fire visual viewport resize events and flush position:fixed elements' layout in the same way what Chrome does. r=botond

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8d6a8573e09b
https://hg.mozilla.org/mozilla-central/rev/0ff21dbde947
https://hg.mozilla.org/mozilla-central/rev/086f87f26b1f