1358775 - Clicking on a link with the dynamic toolbar hidden slides the page down before navigating

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Prior to bug 1335895, if you were on a page with the dynamic toolbar hidden, and clicked on a link, the toolbar would become visible *without* sliding the page (i.e. it would come down and obscure the top of the page). And then it would navigate to the link you clicked on.

With bug 1335895, however, when you click on a link, the entire page slides down as the dynamic toolbar comes into view.

This might be intentional, but I don't recall any mention of it in the dynamic toolbar rewrite bug so I'm assuming it's a bug for now.

Comment 1

[Randall Barker [:rbarker]]

I think the issue may be that call to make the toolbar visible from UI doesn't work the way it used to. When the toolbar becomes visible now, the content always slides down. I'll need to think about a proper fix so that making the toolbar visible does not slide content.

Comment 2

[Randall Barker [:rbarker]]

Attachment: Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

Review commit: https://reviewboard.mozilla.org/r/135978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135978/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

nical, it's just the changes in LayerManagerComposite that you're being asked to look at here. The original dynamic toolbar bug landing was in https://hg.mozilla.org/mozilla-central/rev/81de9d1439b0 - again if you want to look at the LayerManagerComposite changes there it might provide a little more context.

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

https://reviewboard.mozilla.org/r/135978/#review139246

I am not super found of the idea of mutating temprarily the transform and setting it back a bit later. Bugs tend to creep if someone inserts an early-return in between for whatever reason. Especially since this code is not run on the platforms most people use for development.
It would be probably a lot more invasive but I would prefer something where the content transform is accessed through a function that applies the toolbar's transformation without relying on resetting the state at the end of the frame. Or have a RAII helper to ensure the transform is properly reset if you prefer.

::: gfx/layers/composite/LayerManagerComposite.cpp:903
(Diff revision 1)
> -  ScopedCompositorRenderOffset scopedOffset(mCompositor->AsCompositorOGL(), ScreenPoint(0, toolbarHeight));
> +  ShiftContentForToolbar(&screenOffset, &projectionMatrix);
>  #endif
>  
>    if (actualBounds.IsEmpty()) {
>      mCompositor->GetWidget()->PostRender(&widgetContext);
>      return;

Isn't it going to be an issue if we shift the content an return here without shifting back?

::: gfx/layers/composite/LayerManagerComposite.cpp:1147
(Diff revision 1)
> +  *aOriginalOffsetOut = mCompositor->AsCompositorOGL()->GetScreenRenderOffset();
> +  *aOriginalProjectionOut = mCompositor->AsCompositorOGL()->GetProjMatrix();
> +
> +  if (CompositorBridgeParent* bridge = mCompositor->GetCompositorBridgeParent()) {
> +    AndroidDynamicToolbarAnimator* animator = bridge->GetAPZCTreeManager()->GetAndroidDynamicToolbarAnimator();
> +    MOZ_ASSERT(animator);

nit: I'd prefer either this to be a MOZ_RELEASE_ASSERT, or returning before calling GetContentOffset.

::: gfx/layers/composite/LayerManagerComposite.cpp:1169
(Diff revision 1)
> +}
> +
> +void
> +LayerManagerComposite::RenderToolbar(const ScreenPoint& aOriginalOffset, const gfx::Matrix4x4& aOriginalProjection)
> +{
>    // If GetTargetContext returns null we are drawing to the screen so draw the toolbar offset if present.

nit: Looks like this comment makes less sense after this change. Maybe somthing like "If GetTargetContext does not return null we are not rendering to the screen so there is no toolbar to draw."?

Comment 5

[Randall Barker [:rbarker]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135978/diff/1-2/

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

https://reviewboard.mozilla.org/r/135978/#review139698

This is a bit of a rubber-stamp since I don't have a good global understanding of this code.

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:391
(Diff revision 2)
> -  CompositorBridgeParent* parent = CompositorBridgeParent::GetCompositorBridgeParentFromLayersId(mRootLayerTreeId);
> -  if (parent) {
> -    AsyncCompositionManager* manager = parent->GetCompositionManager(nullptr);
> -    if (manager) {
> -      manager->SetFixedLayerMarginsBottom(GetFixedLayerMarginsBottom());
> +  if (continueAnimating) {
> +    manager->SetFixedLayerMargins(mCompositorToolbarHeight, 0);
> +  } else {
> +    if (mCompositorAnimationDirection == MOVE_TOOLBAR_DOWN) {
> +      if (!mCompositorReceivedFirstPaint) {
> +        parent->GetAPZCTreeManager()->AdjustScrollForSurfaceShift(ScreenPoint(0.0f, (float)mCompositorMaxToolbarHeight));

Do we need to request a composite for this adjustment?

::: gfx/layers/composite/LayerManagerComposite.cpp:801
(Diff revision 2)
> -    mOriginalOffset(mCompositor->GetScreenRenderOffset())
> +    mOriginalOffset(mCompositor->GetScreenRenderOffset()),
> +    mOriginalProjection(mCompositor->GetProjMatrix())
>    {
> -    mCompositor->SetScreenRenderOffset(aOffset);
> +    ScreenPoint offset(mOriginalOffset.x + aOffset.x, mOriginalOffset.y + aOffset.y);
> +    mCompositor->SetScreenRenderOffset(offset);
> +    // Calling CompositorOGL::SetScreenRenderOffset does not affect the project matrix

s/project/projection

::: gfx/layers/composite/LayerManagerComposite.cpp:963
(Diff revision 2)
>      &widgetContext, LayoutDeviceIntRect::FromUnknownRect(actualBounds));
>  
>    mCompositor->NormalDrawingDone();
>  
>  #if defined(MOZ_WIDGET_ANDROID)
> +  RenderToolbar();

I assume the reason we're now rendering the toolbar *after* the layers if that it can be on top of the layers. A comment to that effect might be useful.

Comment 7

[Randall Barker [:rbarker]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

https://reviewboard.mozilla.org/r/135978/#review139716

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:391
(Diff revision 2)
> -  CompositorBridgeParent* parent = CompositorBridgeParent::GetCompositorBridgeParentFromLayersId(mRootLayerTreeId);
> -  if (parent) {
> -    AsyncCompositionManager* manager = parent->GetCompositionManager(nullptr);
> -    if (manager) {
> -      manager->SetFixedLayerMarginsBottom(GetFixedLayerMarginsBottom());
> +  if (continueAnimating) {
> +    manager->SetFixedLayerMargins(mCompositorToolbarHeight, 0);
> +  } else {
> +    if (mCompositorAnimationDirection == MOVE_TOOLBAR_DOWN) {
> +      if (!mCompositorReceivedFirstPaint) {
> +        parent->GetAPZCTreeManager()->AdjustScrollForSurfaceShift(ScreenPoint(0.0f, (float)mCompositorMaxToolbarHeight));

The function AndroidDynamicToolbarAnimator::UpdateAnimation is called in AsyncCompositionManager::TransformShadowTree which is where the APZ animations get ticked. I assume there is a composite request at the end.

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135978/diff/2-3/

Comment 9

[Botond Ballo [:botond]]

(In reply to Randall Barker [:rbarker] from comment #7)
> The function AndroidDynamicToolbarAnimator::UpdateAnimation is called in
> AsyncCompositionManager::TransformShadowTree which is where the APZ
> animations get ticked. I assume there is a composite request at the end.

I thought maybe there wasn't one if we return false, but I see now that I was mistaken: returning false just means we might not schedule *another* composite, the current one will proceed regardless. So, all good!

Comment 10

[Randall Barker [:rbarker]]

Nical, I believe I have addressed the issues you raised but I can't figure out how to unclear the review flag on review board. Did you want to review the code with the changes? I wasn't sure what the expectation was after you cleared the review.

Comment 11

[Randall Barker [:rbarker]]

(In reply to Randall Barker [:rbarker] from comment #10)
> Nical, I believe I have addressed the issues you raised but I can't figure
> out how to unclear the review flag on review board. Did you want to review
> the code with the changes? I wasn't sure what the expectation was after you
> cleared the review.

Sorry, I just misunderstood what moz review was showing me. Cancelling ni.

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8864339 [details]
Bug 1358775 - Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar

https://reviewboard.mozilla.org/r/135978/#review140476

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27edd0a20233
Fix the dynamic toolbar animator to animate the toolbar in place instead of shifting content with the toolbar r=botond,nical

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27edd0a20233

Comment 15

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 55.0a1 (2017-06-12).
Devices:
-LG G4 (Android 5.1)
-Nexus 5 (Android 6.0.1)
-Huawei MediaPad M2 (Android 5.1.1)