869411 - About:home is cut off beneath title bar

Status: VERIFIED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Now that we have a scrolling header, I frequently run into an issue where about:home gets cut off at the top, and there is no way to pull the page further down to reveal the top.

Presumably it's because we fix the title bar in place and about:home doesn't know to start lower down, but one way or another we should fix it.

Comment 1

[Chris Lord [:cwiiis]]

This is a regression, if you could add STR if it isn't just self-evident, that'd be good  :)

Comment 2

[Ian Barlow (:ibarlow)]

The only way I seem to be able to reproduce it is similar to bug 869413, where I come in by clicking a link in another app, after the browser has been left unused for a while. Maybe it has something to do with us being killed in the background? Sorry, I know that's pretty vague still

Comment 3

[Brian Nicholson (:bnicholson)]

I've seen this many times, and it looks like a regression from bug 838793.

STR:
1) Open Fennec and see about:home
2) Open another tab, go to http://www.google.com
3) Minimize Fennec and cause it to be killed in the background
4) Reopen Fennec
5) Switch to about:home tab

This bug is probably only worth tracking for releases that have dynamic toolbar pref'd on by default (23+?).

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Move toolbar to gecko_app layout

I don't think we need to dynamically inflate the browser toolbar; we should be able to include it in gecko_app.xml (which, despite its name, is only used in BrowserApp).

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 750167 [details] [diff] [review]
Part 1: Move toolbar to gecko_app layout

Review of attachment 750167 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Replace getPadding() with getTopPadding() in AboutHome

As you mentioned in bug 838793, the extra padding parameters are overkill in AboutHome. This removes them.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Set AboutHome padding in setDynamicToolbarEnabled() instead of refreshToolbarHeight()

Bear with me here, because the way I got to this patch is a bit complicated...

First, I noticed that refreshToolbarHeight() uses getUserVisibleHint() to determine whether to set the padding for about:home, but we call refreshToolbarHeight() before we set the hint to true. Simply moving refreshToolbarHeight() after setUserVisibleHint(true) fixes this bug -- mostly. I say mostly because there's still a brief period at startup where about:home is cut off. This happens for a couple of reasons:

1) Since AboutHome is a fragment, it is immediately reattached when BrowserApp is created, and the padding isn't adjusted until after it's shown.

2) We call refreshToolbarHeight() at least once before a layout pass has happened. refreshToolbarHeight() gets the height from mBrowserToolbar's height, but the height is 0 since the toolbar isn't yet attached to the window.

#1 can be addressed by saving the padding in the fragment state (which I do in the next patch). #2 is what this patch tries to fix.

Rather than setting about:home padding every time we refresh the toolbar height, I moved the setTopPadding() call into setDynamicToolbarEnabled(). This sets the padding only when BrowserApp is created or when the dynamic toolbar pref changes. To prevent this method from being called before a layout pass, I posted the call to setDynamicToolbarEnabled() to the View's queue. postToUiThread()/runOnUiThread() don't use the same Handler as View (a new Handler is created when the view is attached to the window [1]), so merely running this on the UI thread means it could happen before a layout pass. View#post() can be called off of the UI thread only when the view is attached to the window, so View#post() is also insufficient on its own. It does work if View#post() is nested inside of runOnUiThread(), which is what I ended up doing, but I don't like that part of the patch. If you know of a better way to do this, I'll happily change it.

Note that since refreshToolbarHeight() no longer calls getUserVisibleHint(), the initial change I mentioned about moving refreshToolbarHeight() after setUserVisibleHint(true) no longer applies.

[1] http://androidxref.com/4.1.1/xref/frameworks/base/tools/layoutlib/bridge/src/android/view/AttachInfo_Accessor.java#32

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Save top padding state in AboutHome

Saves the padding as part of the fragment state, as mentioned above.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Set AboutHome padding in setDynamicToolbarEnabled() instead of refreshToolbarHeight(), v2

I think this is a better way to fix the misalignment at startup issue. Instead of worrying about a race between setDynamicToolbarHeight() and the layout pass, we can remember whether the dynamic toolbar was enabled before the activity is destroyed. Unless the pref changes, setDynamicToolbarEnabled() will now be called only once for the entire duration of Fennec's lifetime, including OOM kills. The reason is that we have a check for "value == mDynamicToolbarEnabled" in the pref observer, and since we're initializing mDynamicToolbarEnabled to its previous state, we won't call setDynamicToolbarEnabled (meaning we won't end up calling mAboutHome.setTopPadding() with a height of 0).

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 750169 [details] [diff] [review]
Part 2: Replace getPadding() with getTopPadding() in AboutHome

Review of attachment 750169 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/AboutHome.java
@@ +40,1 @@
>      private int mPaddingTop;

nit: Rename to mTopPadding for consistency?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 750208 [details] [diff] [review]
Part 4: Save top padding state in AboutHome

Review of attachment 750208 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: mobile/android/base/widget/AboutHome.java
@@ +43,3 @@
>      private LastTabsSection mLastTabsSection;
>      private RemoteTabsSection mRemoteTabsSection;
>      private TopSitesView mTopSitesView;

nit: add empty line here.

@@ +62,3 @@
>          super.onCreate(savedInstanceState);
>  
>          mLightweightTheme = ((GeckoApplication) getActivity().getApplication()).getLightweightTheme();

nit: add empty line here.

@@ +62,5 @@
>          super.onCreate(savedInstanceState);
>  
>          mLightweightTheme = ((GeckoApplication) getActivity().getApplication()).getLightweightTheme();
> +        if (savedInstanceState != null) {
> +            mPaddingTop = savedInstanceState.getInt(STATE_TOP_PADDING);

I'd use getInt(STATE_TOP_PADDING, 0) just to be more explicit about the intended default value here.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 750918 [details] [diff] [review]
Part 3: Set AboutHome padding in setDynamicToolbarEnabled() instead of refreshToolbarHeight(), v2

Review of attachment 750918 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Sorry for taking so long, this got lost under piles of other bug mail :/

Comment 13

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e930f5a6e1b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b42d9640fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c85b3bce59
https://hg.mozilla.org/integration/mozilla-inbound/rev/06567a8f5b8c

Comment 14

[Brian Nicholson (:bnicholson)]

Oops, somehow missed a couple of mPaddingTop->mTopPadding renames. Landed follow-up:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad1d3d468d2

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e930f5a6e1b9
https://hg.mozilla.org/mozilla-central/rev/70b42d9640fe
https://hg.mozilla.org/mozilla-central/rev/a4c85b3bce59
https://hg.mozilla.org/mozilla-central/rev/06567a8f5b8c
https://hg.mozilla.org/mozilla-central/rev/8ad1d3d468d2

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 750167 [details] [diff] [review]
Part 1: Move toolbar to gecko_app layout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 838793
User impact if declined: top of about:home will be cut off after an OOM restore
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/64fc1861d51c
https://hg.mozilla.org/releases/mozilla-aurora/rev/90c8dc46a182
https://hg.mozilla.org/releases/mozilla-aurora/rev/f14cb7d1d276
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6a4948b6d9a

Comment 18

[Brian Nicholson (:bnicholson)]

I swear that these patches were working for me when I was testing this before, but I still see this issue on 23/24. Filed bug 878491.

Comment 19

[Gabriela [:gaby2300]]

I can verify this is fixed in a Galaxy S2 phone with Android 4.0.3 and the latest Firefox Beta.

Comment 20

[Adrian Tamas (:AdrianT)]

I am unable to reproduce the issue on Firefox Mobile 23 RC and Aurora 24.0a1 2013-07-31 on the Samsung Galaxy R (Android 2.3.4)