1474888 - Fix Prompt.java's AlertDialog

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Petru-Mugurel Lingurar [:petru]]

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

Comment 1

[Petru-Mugurel Lingurar [:petru]]

NI-in Bram to ask for his opinion from a UX perspective.

Comment 2

[Bram Pitoyo [:bram]]

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.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

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

Comment 4

[Jan Henning [:JanH]]

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

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1474888 - Fix Prompt.java's AlertDialog;

PromptService will be informed when the screen is rotated and in turn it will
ask every Prompt shown to reset it's current layout.
For this, every Prompt will
- save it's current input value in PromptInput's mValue or similar field
already used for storing the default PromptInput value.
- create a new widget to be used in the new AlertDialog, with a new appropriate
layout for portrait / landscape. This is when the mValue field will be used
to initialize the new widget with the previous input.

Review commit: https://reviewboard.mozilla.org/r/258834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258834/

Comment 6

[Petru-Mugurel Lingurar [:petru]]

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.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8994227 [details]
Bug 1474888 - Fix Prompt.java's AlertDialog;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258834/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc9dc68f20ac
Fix Prompt.java's AlertDialog; r=jchen

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fc9dc68f20ac

Comment 11

[Oana Horvath [:ohorvath - out of office]]

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)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta backport?

Comment 13

[Petru-Mugurel Lingurar [:petru]]

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]: --

Comment 14

[Liz Henry (:lizzard)]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ebf6855161a1

Comment 16

[Ioana Chiorean]

Can you please check this, Levente?

Comment 17

[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)