Closed Bug 1474888 Opened Last year Closed Last year

Fix Prompt.java's AlertDialog

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: petru, Assigned: petru)

References

Details

Attachments

(1 file)

Fennec is set to handle the configuration changes itself.

As such, the AlertDialogs is kept across screen rotations.

At least in the case of the Dialog from Prompt.java [1] this is problematic because the old layout is kept which is not suitable for the new configuration.
As reported in bug 1460072 comment #0 and easily seen here - https://drive.google.com/file/d/1-E8g0PMfq3AsYp-QUkENXYaaBMa_MOFw/view?usp=sharing
This bug exists also in Chrome [2] and Focus [3].

- The easiest solution would be to just dismiss the dialog in the case of a screen rotation and let the user start it again. This would be the most detrimental to the UX I think.
- Next easiest solution would be to restart that same dialog in the case of a screen rotation. This would automatically create a new dialog which means the user would have lost his input until then.
- The best solution for the user would be to save the state of the currently displayed View and restore it after the re-creation of the same View. 
But because the AlertDialog in question uses views from different platform widgets with different state and no easy way to save and return their state to us this would be an intricate process, prone to bugs.

[1] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#176
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=847566
[3] https://github.com/mozilla-mobile/focus-android/issues/2595
NI-in Bram to ask for his opinion from a UX perspective.
Flags: needinfo?(bram)
Blocks: 1460072
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Hi Petru, the best solution is to use a dialogue that was designed with a responsive layout, so that it can flexibly reposition based on the user’s current screen rotation without changing or refreshing its state.

However, it sounds like this is impossible to do? Or at least, this is highly dependent on whether the host that provides the dialogue has designed a responsive layout for it – and most hosts haven’t.

The next best solution is to save the dialogue state, recreate the view, then restore the state. But you mentioned that this is intricate.

With all those constraints, I would recommend going with option 2: recreating the view without saving the state. This is bound to cause annoyance, but we hope that most users would have picked a selection and hit “Okay” before rotating the screen.
Flags: needinfo?(bram)
Was looking into saving state and restoring it after a screen rotation but there are a few possible inputs for which I can't find how to test to make sure all will continue working.

According to the list of possible inputs [1] I need help with finding how to test the following inputs:
- menulist
- label
- icongrid
- tabs

[1] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/base/java/org/mozilla/gecko/prompts/PromptInput.java#357
A tip - you can restrict the DXR search by using a filter like "path:mobile*js", which makes finding relevant results easier. But in any case what I found was:
- menulist seems to be a dropdown list and is used by the WebRTC permissions dialogue [1], try https://mozilla.github.io/webrtc-landing/gum_test.html
- label is used for viewing certificate details [2]: Try http://www.buttercookie.de/temp/ca.crt and then click on "View"
- icongrid is used by the helper app prompts [3] you get when a file download can be handled by other apps as well, or when the Android icon in the URL bar appears for more than one app that can handle the current page
- context menus internally seem to use prompts as well and use tabs [4], try long-pressing an image link to see that in action


[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebrtcUI.jsm#251
[2] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/components/NSSDialogService.js#148
[3] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/modules/HelperApps.jsm#84
[4] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/chrome/content/browser.js#3109
Thanks Jan!

Managed to test each PromptInput and I think the above patch should handle the screen rotation as best as possible.
A minor glitch could be seen now and then but being able to keep the user input should make up for that.

There are a few PromptInput concretizations that do not need to save their current input, as a choice will be made and sent when the user clicks a certain option: TabInput, IconGridInput and LabelInput.
The rest of the PromptInputs, upon screen rotation, will save their current input and restore it in the newly created widget for the new AlertDialog.
Attachment #8994227 - Flags: review?(sdaswani) → review?(nchen)
Comment on attachment 8994227 [details]
Bug 1474888 - Fix Prompt.java's AlertDialog;

https://reviewboard.mozilla.org/r/258834/#review266460

::: mobile/android/base/java/org/mozilla/gecko/prompts/ColorPickerInput.java:27
(Diff revision 1)
>          String init = obj.getString("value");
> -        mInitialColor = Color.rgb(Integer.parseInt(init.substring(1, 3), 16),
> +        mInitialColor = getColorCode(init);

Combine the two lines

::: mobile/android/base/java/org/mozilla/gecko/prompts/PromptService.java:18
(Diff revision 1)
> -import android.util.Log;
>  
>  public class PromptService implements BundleEventListener {
>      private static final String LOGTAG = "GeckoPromptService";
>  
> -    private final Context mContext;
> +    private final Context context;

We prefer `m` prefixes but not big deal
Attachment #8994227 - Flags: review?(nchen) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc9dc68f20ac
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: qe-verify+
Blocks: 1460585
Attachment #8994227 - Flags: review?(sdaswani)
Verified as fixed on Nightly 63. The date/time picker looks better on rotation, also verified the other input types are ok.
Huawei Nexus 6P (Android 8.1.0)
Google Pixel (Android P Beta)
HTC Desire 820 (Android 6.0.1)
Samsung Galaxy Tab 3 (Android 8.0)
Is this something we should consider for Beta backport?
Flags: needinfo?(petru.lingurar)
Comment on attachment 8994227 [details]
Bug 1474888 - Fix Prompt.java's AlertDialog;

Approval Request Comment
[Feature/Bug causing the regression]: The original layout for input pickers is maintained across rotations.
[User impact if declined]: Layout may not fit well on the new configuration after screen rotation.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: --
[List of other uplifts needed for the feature/fix]: --
[Is the change risky?]: No
[Why is the change risky/not risky?]: Has been QE verified and in the time being in Nightly no issues have been reported.
[String changes made/needed]: --
Flags: needinfo?(petru.lingurar)
Attachment #8994227 - Flags: approval-mozilla-beta?
Comment on attachment 8994227 [details]
Bug 1474888 - Fix Prompt.java's AlertDialog;

One of several small fixes for the date picker related to the switch in 62 to using the platform library. Verified in nightly. 
Let's uplift for next Monday's beta 17 build.
Attachment #8994227 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Can you please check this, Levente?
Flags: needinfo?(levente.sacal)
Verified as fixed in 62 Beta 17. Tested the date/time picker on https://www.456bereastreet.com/lab/html5-input-types/ on landscape as well.
Sony Xperia Z5 (Android 7.0)
Samsung Galaxy S8 (Android 8.0.0)
Flags: needinfo?(levente.sacal)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1486200
No longer depends on: 1486200
You need to log in before you can comment on or make changes to this bug.