1128431 - 'Start browsing' link from onboarding v1.5 is not visible on small screen devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristina Madaras, QA [:CristinaM]]

Attachment: onboarding.png

Environment:
Device: Samsung Galaxy R (Android 2.3.4)
Build: Firefox for Android 38.0a1 (2015-02-01)

Steps to reproduce:
1. Make sure your Firefox profile is clean (Android Settings -> Apps -> Manage applications -> Firefox -> Force Stop & Clear data);
2. Launch Firefox.

Expected result:
Onboarding screen is displayed containing 'Sign in to Nightly' button and 'Start browsing' link.

Actual results:
'Start browsing' link is not visible.

Notes: Please see the attachment.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Given the bugs filed for "small screen" layout issues, I think we should make a few small screen layout resources.

Comment 2

[Allison Naaktgeboren :ally]

Digging into this, we're pretty sure this a layout bug.

Ruled out
- altering or adding weight to the missing element does not have an effect
- altering the weights of the parents
- scrollview has correct number & type of children
- there are no listviews present to interfere 
- layout emulators show missing textview in correct place
- reordering the elements. the bottom thing is cut off or not present and there is still no scrollbar
--- I notice that the padding between the hint(firstrun_welcome_subtext)  & the following child is honored over wrap_content of the bottom children:
 android:paddingTop="20dp"
 android:paddingBottom="30dp"

Works (scrollbar when needed + visible element)
- hardcording the height instead of wrap content
- keeping wrap content & hardcoding a minHeight at 92dp+ works, but we feel that this is a band-aid and that there is something bigger going on.

diff --git a/mobile/android/base/resources/layout/firstrun_welcome_fragment.xml b/mobile/android/base/resources/layout/firstrun_welcome_fragment.xml
--- a/mobile/android/base/resources/layout/firstrun_welcome_fragment.xml
+++ b/mobile/android/base/resources/layout/firstrun_welcome_fragment.xml
@@ -54,15 +54,16 @@
                     style="@style/Widget.Firstrun.Button"
                     android:background="@drawable/firstrun_button_background"
                     android:layout_gravity="center"
                     android:text="@string/firstrun_welcome_button_account"/>
 
             <TextView android:id="@+id/welcome_browse"
                       android:layout_width="@dimen/firstrun_content_width"
                       android:layout_height="wrap_content"
+                      android:minHeight="92dp"
                       android:padding="20dp"
                       android:gravity="center"
                       android:textAppearance="@style/TextAppearance.FirstrunRegular.Link"
                       android:text="@string/firstrun_welcome_button_browser"/>
         </LinearLayout>
     </LinearLayout>
 </ScrollView>


So now we have a band-aid as a fallback. My plan is to see if I can make this behave in a more css like manner to scale the gaps between text accordingly because this feels like a bad interaction between autowrapping height & fixed heights.

Comment 3

[Allison Naaktgeboren :ally]

band-aid works on htc evo 3d, nexus 4, samsung galaxy s5

Comment 4

[Allison Naaktgeboren :ally]

Sony Ericsson xperia : doesnt fit so scrolls properly as expected

Comment 5

[Allison Naaktgeboren :ally]

huawei ascend also fitting, though I note that the blue area is smaller proportionally

Comment 6

[Allison Naaktgeboren :ally]

it seems that in the android layout world, explicit padding around child messes up the weighting of parent elements?

Comment 7

[Allison Naaktgeboren :ally]

given the upcoming merge, I think we might have to run with the bandaid

Comment 8

[Allison Naaktgeboren :ally]

Attachment: screenshot with bandaid, reduced blue

Comment 9

[Allison Naaktgeboren :ally]

Attachment: landscape nexus 10 w/bandaid applied

yes, you may give me shit for taking a photo. intellij's screenshot thingy is not cooperating and this discussion needs to _move_.

Comment 10

[Allison Naaktgeboren :ally]

Attachment: huawei ascend landscape (scrolls)

scrolls around in landscape view

Comment 11

[Chenxia Liu [:liuche]]

I'm okay with this approach if this is a separate mdpi layout, so we minimize the impact of this on larger devices, esp in landscape.

Comment 12

[Allison Naaktgeboren :ally]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=786478ed8eb3

Comment 13

[Allison Naaktgeboren :ally]

Attachment: welcomefirstrunlayout

bandaid #2, w/mdpi layout

tested on the htc evo 3d

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8582531 [details] [diff] [review]
welcomefirstrunlayout

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

I thought at first that this should be in layout-small, but this problem isn't restricted to small devices (and in fact doesn't manifest on some small devices like the Galaxy Ch@t) so layout-mdpi it is. Curious and curiouser.

Please land and request uplift when fx-team opens again. Also, good practice to just include the commit message when you flag for review, esp for an aurora bug.

Comment 17

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/466c2dfda162

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8582531 [details] [diff] [review]
welcomefirstrunlayout

Approval Request Comment
[Feature/regressing bug #]: First Run (bug 1063844) on mdpi devices
[User impact if declined]: First-time users on mdpi devices will have an unscrollable, truncated first run experience in portrait
[Describe test coverage new/current, TreeHerder]: try run, local testing
[Risks and why]: Very low, new resource file that is an exact copy of an existing resource file with one item height change, to be used only by mdpi devices
[String/UUID change made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/466c2dfda162

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9ab25405ce1

Comment 21

[Flaviu Cos]

Verified as fixed in builds:
- 39.0a1 2015-03-26;
- 38.0a2 2015-03-26;
Device: Samsung Galaxy R (Android 2.3.).