1302150 - Dynamic toolbar doesn't reappear after scrolling to bottom of long page

Status: VERIFIED FIXED

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

Description

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

It looks like with the C++ APZ the NativePanZoomController code never invokes PanZoomTarget.panZoomStopped(), so the code at [1] is never getting run.

[1] http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1567

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this something we're thinking of getting to, or should we stop tracking it?

Comment 2

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

Presumably this won't get fixed until bug 1335895 is done.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 53, as we are in the last week of the 53 beta cycle.

Comment 4

[Randall Barker [:rbarker]]

Attachment: Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

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

Comment 5

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

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review139168

Mostly a rubber stamp since it's pretty hard to follow all the different code paths and conditions. Nothing looked obviously wrong.

Comment 6

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

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review139168

That said, do you have any plans to write automated tests for the core dynamic toolbar code? I think it would be worthwhile, because it's getting to the point where there are so many different scenarios to deal with that fixing one could easily regress another. Having automated test coverage for scenarios that are fixed to prevent them from regressing would be very valuable.

Comment 7

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Comment on attachment 8864340 [details]
> Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is
> reached
> 
> https://reviewboard.mozilla.org/r/135982/#review139168
> 
> That said, do you have any plans to write automated tests for the core
> dynamic toolbar code? I think it would be worthwhile, because it's getting
> to the point where there are so many different scenarios to deal with that
> fixing one could easily regress another. Having automated test coverage for
> scenarios that are fixed to prevent them from regressing would be very
> valuable.

snorp and I are discussing the best approach to take.

Comment 8

[Randall Barker [:rbarker]]

After further testing I found some issues with dragging and animating the toolbar at the bottom of the page at the same time. Since this patch hasn't landed yet, I figured it would be better to post a new version that fixes the issues rather than create a new bug and post the fixes there. Kats do you have an opinion on this?

Comment 9

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

Ok, you can update the patch on this bug.

Comment 10

[Randall Barker [:rbarker]]

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

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

Comment 11

[Randall Barker [:rbarker]]

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

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

Comment 12

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

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review140550

Comment 13

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

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review140552

Comment 14

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

Flag me for review when the patch is ready.

Comment 15

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

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review141076

This is largely a rubberstamp, it's really hard to wrap my head around the different states so I have no idea if this is doing the right thing or not.

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:159
(Diff revision 3)
>    void TranslateTouchEvent(MultiTouchInput& aTouchEvent);
>    ScreenIntCoord GetFixedLayerMarginsBottom();
>    void NotifyControllerSnapshotFailed();
>    void CheckForResetOnNextMove(ScreenIntCoord aCurrentTouch);
> +  // Returns true if the page scroll offset is near the bottom.
> +  bool ScrollOffsetNearBottom();

Make this function const if you can.

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:160
(Diff revision 3)
> +  // Returns true if toolbar is completely visible or completely hidden.
> +  bool ToolbarInTransition();

The comment here seems to say the opposite of what I would expect the function to do based on its name. And the implementation seems to match the name, so the comment is wrong? Also, make the function const if you can.

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:1009
(Diff revision 3)
>    MOZ_ASSERT(APZThreadUtils::IsControllerThread());
> -  // The toolbar will only hide if dragging up so ignore positive deltas from dragging down.
> -  if (delta >= 0) {
> +  // if the page is too small then the toolbar can not be hidden
> +  if ((float)mControllerSurfaceHeight >= (mControllerFrameMetrics.mPageRect.YMost() * SHRINK_FACTOR)) {
> +    // If the toolbar is partial hidden, show it.
> +    if ((mControllerToolbarHeight != mControllerMaxToolbarHeight) && !mPinnedFlags) {
> +      StartCompositorAnimation(MOVE_TOOLBAR_DOWN, eImmediate, mControllerToolbarHeight, /* wait for page resize */ true);

This function has side effects! Totally not obvious from the name or the documentation.

Comment 16

[Randall Barker [:rbarker]]

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

https://reviewboard.mozilla.org/r/135982/#review141076

> The comment here seems to say the opposite of what I would expect the function to do based on its name. And the implementation seems to match the name, so the comment is wrong? Also, make the function const if you can.

Yeah, the function was orignally ToolbarNotInTransition() which led to the double negative !ToolbarNotInTransition() so I removed the Not but forgot to update the comment.

> This function has side effects! Totally not obvious from the name or the documentation.

I renamed it PageTooSmallEnsureToolbarVisible(). Originally it only tested for the page being too small but since every place it returned true, I was making it visible, it seemed easier to roll it into the test function.

Comment 17

[Randall Barker [:rbarker]]

Comment on attachment 8864340 [details]
Bug 1302150 - Ensure the dynamic toolbar becomes visible when end of page is reached

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06c11db07536
Ensure the dynamic toolbar becomes visible when end of page is reached r=kats

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06c11db07536

Comment 20

[Oana Horvath [:ohorvath]]

Verified as fixed on latest Nightly 55.0a1 (2017-6-12). The dynamic toolbar appears when you get to the bottom of the page.
Devices:
-Prestigio Grace X5 (Android 4.4.2)
-LG G4 (Android 5.1)
-Nexus 5 (Android 6.0.1)
-Huawei MediaPad M2 (Android 5.1.1)