Closed Bug 1128431 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified
fennec 38+ ---

People

(Reporter: CristinaM, Assigned: ally)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Attached image 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.
Blocks: 1063844
tracking-fennec: --- → ?
Given the bugs filed for "small screen" layout issues, I think we should make a few small screen layout resources.
Assignee: nobody → liuche
tracking-fennec: ? → 38+
QA Contact: ioana.chiorean
Assignee: liuche → ally
Blocks: onboarding
No longer blocks: 1063844
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.
band-aid works on htc evo 3d, nexus 4, samsung galaxy s5
Sony Ericsson xperia : doesnt fit so scrolls properly as expected
huawei ascend also fitting, though I note that the blue area is smaller proportionally
it seems that in the android layout world, explicit padding around child messes up the weighting of parent elements?
given the upcoming merge, I think we might have to run with the bandaid
yes, you may give me shit for taking a photo. intellij's screenshot thingy is not cooperating and this discussion needs to _move_.
scrolls around in landscape view
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.
bandaid #2, w/mdpi layout

tested on the htc evo 3d
Attachment #8582531 - Flags: review?(liuche)
Duplicate of this bug: 1135770
Duplicate of this bug: 1135771
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.
Attachment #8582531 - Flags: review?(liuche) → review+
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
Attachment #8582531 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/466c2dfda162
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8582531 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 39.0a1 2015-03-26;
- 38.0a2 2015-03-26;
Device: Samsung Galaxy R (Android 2.3.).
Status: RESOLVED → VERIFIED
Blocks: 1154428
You need to log in before you can comment on or make changes to this bug.