Closed Bug 1254555 Opened 4 years ago Closed 3 years ago

Unnecessary scroll to reveal full first run panel and options

Categories

(Firefox for Android :: First Run, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox46 --- unaffected
firefox47 --- affected
firefox48 --- affected

People

(Reporter: ioana.chiorean, Assigned: shubham2892, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(3 files, 1 obsolete file)

Android 2.3.5 - HTC Desire HD
Fennec 46 Beta 1

Steps:
1. Open Fennec with a clean profile on a low resolution device

Expected Result: 
- you should be able to see the 'Next' link at the bottom part of the panel/screen

Actual Result:
- you can not see the Next button and some users might be blocked by that ( they can use the top area to switch to other panel)

Notes:
- new first run: https://www.youtube.com/watch?v=cfykZ9wEbQI&feature=youtu.be
- old first run: https://www.youtube.com/watch?v=5dZDNDTqsR0&feature=youtu.be
Flags: needinfo?(liuche)
Thanks for the videos Ioana.

I think we can do a couple of things with this, either:
- Add a spacer with a weight in between the text body
- Have a specific layout for small devices
- WONTFIX this

I'll add a mentor flag to this bug - basically, we want this to show the "Next" text on smaller-screen devices. Let's try this by adding a spacer with a weight between the "Next" item and the body text, and trying to make it as close to the current version as possible.
Blocks: onboarding
Mentor: liuche
Flags: needinfo?(liuche) → needinfo?(alam)
Whiteboard: [lang=java][good first bug]
I'm not sure what "adding a spacer" means but some screenshots on how this "spacer" affects most devices would be helpful :)
Flags: needinfo?(alam)
Hi,
I'am really interesting by this bug ( it will be my first bug)
But i don't understand how to put my emulator (mozemulator) to a low quality =/

Do you know how to do that?
Hi Alex, do you use Android Studio or IntelliJ? You should be able to create new emulators in Tools > Android > AVD Manager (or just shift-shift to bring up the search and type "AVD Manager"). From there you should be able to create an emulator that will show up in the device chooser when you want to run a build.

As a note, this probably isn't a ldpi device, an mdpi one would do - we want the case where there is space to show all the content, but we're not.

Additionally, make sure you test any changes you make on normal size devices and make sure that we're not regressing anything for the other devices.

Let me know if you have any questions!
Hi Chenxia, Thank you for your answer.
I'am using IntelliJ.
I succes to recreat the bug on a QGVA 320 * 480 mdpi, with marshmallow Android 6.0x86_64.
Can you just help me a bit where is situated the code of this bug? Or an indication ? ^.^
Hello,
Is it possible to be assigned to this bug?
(I found where is the problem, I 'am working on it  :)  )
Thanks Alex! We don't assign bugs until there's a patch, because we often get lots of interest but not much followup.

If you haven't seen it already, this is our guideline to writing/preparing patches. Let me know if you have questions, and thanks again for investigating this bug!
https://wiki.mozilla.org/Mobile/Fennec/Android/Suggested_workflow#Creating_commits_and_submitting_patches
Okey, I will do my best to fix this bug then :)
Hi,
I'm actually trying to fix the UI bug, but I can't find an easy way to launch the firstscreen panel.
Do you know an easy way to do it (maybe through Intent ?).
Attached patch Patch for Bug 1254555 (obsolete) — Splinter Review
Add files for multiples densities
Corrects the bug for mdpi and hdpi screens
Attachment #8749272 - Flags: review?(liuche)
Hello,
Is it possible to have a feedback about my patch ? :)
All of the primary developers are at Google I/O this week.
oh, okey thank you for the information
Comment on attachment 8749272 [details] [diff] [review]
Patch for Bug 1254555

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

Sorry for the delay!

Hmm, so looking at this, I don't think that we want to do this by hard-coding specific dimensions for different dpi screens. I don't think this would work for all devices, so we'd just be pushing the problem off to a smaller set of specific devices. Specifically, there should be a way to use layout_weight or perhaps RelativeLayout to constrain these layouts so they don't take up too much space, so I'd like to see an approach that doesn't rely on hard-coded dimensions.

::: mobile/android/base/resources/values-hdpi/dimens.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<resources>
> +
> +    <dimen name="firstrun_content_width">300dp</dimen>

If this value isn't changing between versions, you don't need to include it in a dpi-specific file.
Attachment #8749272 - Flags: review?(liuche)
Hi, Can I take up this bug(this will be my first bug) from where Alex left.
Shubham: looks like it's been sitting idle for a bit. Unless Alex says he'd like to take another run at it, go for it.
(In reply to Alex from comment #14)
> oh, okey thank you for the information

Hey Alex, are you still working on it. If not would it be okay if I take it over?
Alex ping for this.
Flags: needinfo?(noxus.iceman)
Hey, yeah you can do it, sorry for delay
Hi Mike, I have pushed the patch for the bug. Looking forward to your feedback. Thanks.
Flags: needinfo?(mhoye)
Thanks, Subham, but I'm not the right person. Chenxia, what do you think?
Flags: needinfo?(mhoye) → needinfo?(liuche)
Hi Chenxia, did you get a chance to look at the patch.
Comment on attachment 8793470 [details]
Bug 1254555 - Unnecessary scroll to reveal full first run panel.

Thanks Shubham, I took a look at this patch! The code looks good, but there's the problem when no switch is present and the phone is in landscape, and the "Next" text overlaps the text of the slide. Also, there's the case where the "Next" button is actually above the switch. I'll add screenshots of these two cases.

You could consider setting the switch view to invisible, and adding it to the RelativeLayout positioning hierarchy.

Alternatively, if you find it isn't actually easy to take the approach to fix this, we should just WONTFIX it.

Thanks for your help!
Flags: needinfo?(noxus.iceman)
Flags: needinfo?(liuche)
Attachment #8793470 - Flags: review?(mhoye) → feedback+
Assignee: nobody → shubham2892
Attached image Screenshot: Overlap
Attachment #8749272 - Attachment is obsolete: true
Hi, how can I work on this bug? I'm new to open source.
(In reply to Chenxia Liu [:liuche] from comment #25)
> Comment on attachment 8793470 [details]
> Bug 1254555 - Unnecessary scroll to reveal full first run panel.
> 
> Thanks Shubham, I took a look at this patch! The code looks good, but
> there's the problem when no switch is present and the phone is in landscape,
> and the "Next" text overlaps the text of the slide. Also, there's the case
> where the "Next" button is actually above the switch. I'll add screenshots
> of these two cases.
> 
> You could consider setting the switch view to invisible, and adding it to
> the RelativeLayout positioning hierarchy.
> 
> Alternatively, if you find it isn't actually easy to take the approach to
> fix this, we should just WONTFIX it.
> 
> Thanks for your help!

Hi Chenxia,
As of with the current structure, it is not easy to fix. One approach could be decreasing the padding between layout elements for the low resolution devices but as you already mentioned that would be pushing the problem off to a smaller set of specific devices.
Flags: needinfo?(liuche)
Thanks for looking into this, Shubham, because this bug has been open for a while.

I think the best course of action here is to WONTFIX this bug. I tried this out on a bunch of small devices, and for the most part with the newest firstrun experiment, this problem shows up rarely and it's not worth having more people invest time into this.

Thanks again!
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(liuche)
You need to log in before you can comment on or make changes to this bug.