770101 - about:config should label integer fields correctly so keyboard uses numerical mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gervase Markham [:gerv]]

Fennec has its own implementation of about:config. Entries in about:config can be boolean, integer or string. Booleans have a "toggle" rather than a "modify" button, which is good, but for integers, the field should be labelled as an integer field such that the keyboard enters numerical mode.

Gerv

Comment 1

[:Margaret Leibovic]

We're creating this prompt with the prompt service:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/config.xhtml#150

I don't think the prompt service API has a way to indicate that a field is a number, so it would be hard to let our Java implementation know that we want to show the numerical keyboard.

Comment 2

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

Is there a way we can cheat? My thoughts here are:
* We control the caller (config.xhtml) and the callee (PromptService.java)
* We could switch to use the message ("Prompt:Show") and add a hint for the type
* The Java code would use the hint, if found, to adjust the UI

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v1)

Cool stuff ... just cheating a little  >)

Comment 4

[Mark Capella [:capella]]

Attachment: Screen Shot

Comment 5

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

Comment on attachment 704376 [details] [diff] [review]
Patch (v1)

Wes is a better reviewer for PromptService

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 704376 [details] [diff] [review]
Patch (v1)

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

Great stuff :) I hate that we're not just doing this editing inline (i.e. we support input[type="number"] in HTML, if you want to work on fixing up aboout:config, I would love you), but this seems like a nice feature to have in prompts anyway. Just want a slightly different API.

::: mobile/android/base/PromptService.java
@@ +106,5 @@
>              mType = getSafeString(aJSONInput, "type");
>              String id = getSafeString(aJSONInput, "id");
>              mId = TextUtils.isEmpty(id) ? mType : id;
>              mHint = getSafeString(aJSONInput, "hint");
> +            mKbdType = getSafeString(aJSONInput, "keyboard");

This may make the patch more complex, but I don't want to add more parameters here (its already too much I think, but I haven't been clever enough to pair it down). I'd rather we just have type="number" than specify a keyboard type. Hopefully we can still share most of the code with the textbox input type.

On Android version 11+, we can actually use a NumberPicker instead:
https://developer.android.com/reference/android/widget/NumberPicker.html

::: mobile/android/chrome/content/config.xhtml
@@ +147,5 @@
>          } else {
> +          let message = { type: "Prompt:Show" };
> +          message.title = title;
> +          message.text = gStringBundle.formatStringFromName("modifyPref.promptText", [aPref.name], 1);
> +          message.buttons = [ "OK", "Cancel" ];

Localization. These strings are in chrome://global/locale/commonDialogs.properties

Comment 7

[Mark Capella [:capella]]

Ok, localizing the strings and creating a new PromptService type of "number" is easy enough. Even implementing the NumberPicker is fairly simple (and cool!) but the class doesn't seem to allow for negative numeric values ...

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v2)

Maybe I'm getting too creative here with the NumberPicker :P ... See new screen shot. I added a togglebutton to handle negative integers ... then ran into two issues.

Setting the pickers' MIN value to 0 and MAX value to |Integer.MAX_VALUE| causes what appears to be an android bug (image left). Setting MAX to |Integer.MAX_VALUE - 1| works ok (image center) with the following behaviour that I haven't fixed yet.

If you click the pickers' middle / selected number, it opens the keyboard for you to make a manual entry (image right). Cool so far. If you click DONE on the keyboard all proceeds normally. However, if you enter a number and press the OK button in the dialog, it ends without grabbing the new data in the picker editbox. (Plus sign changes are preserved).

Still playing ...

Comment 9

[Mark Capella [:capella]]

Attachment: Screenshot NumberPicker

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v3)

This seems to solve all issues I wasn't happy with. The user can now select all positive / negative INT values including Integer.MAX_VALUE... I fixed the scrolling issue but setting the wheel to not continuously wrap around then ends.

Also, keying a numeric value and clicking the OK button now accepts the current fields data without first having to click DONE in the soft keyboard.

Have a look?

Comment 11

[Mark Capella [:capella]]

Attachment: Screen Shot Landscape

Bug 729056 may also be corrected by this change.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 705276 [details] [diff] [review]
Patch (v3)

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

I think we should get ibarlow's feedback on the +- button, and I want to play with it a bit myself, but this will crash on 2.3 so wanted to get back to you now rather than later.

::: mobile/android/base/PromptService.java
@@ +204,5 @@
> +                signButton.setTextOff("-");
> +                signButton.setChecked(value >= 0);
> +                pickerView.addView(signButton);
> +
> +                NumberPicker numberPicker = new NumberPicker(GeckoApp.mAppContext);

This will crash on Android version < 11. We'll need to provide a textbox fallback there.

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v4)

Crashes are bad, 'mmkay ?

Comment 14

[Mark Capella [:capella]]

Comment on attachment 706407 [details] [diff] [review]
Patch (v4)

Forgot to flag ibarlow for plus / minus sign feedback ...

Comment 15

[Ian Barlow (:ibarlow)]

Attachment: mockup

It seems like a number picker with arrows on top and bottom might be a more appropriate UI for this? See attached.

Comment 16

[Mark Capella [:capella]]

The basic NumberPicker widget allows for one of two styles based on theme ... one with numbers above/ below, and one with arrows above / below ... (it doesn't allow for negative numbers either hence my plus/minus toggle modification ...)

So I'm confused by the mockup ... Was this something you coded / the widget allows for and I haven't noticed yet? Or do you suggest a new custom widget employing both options?

Comment 17

[Ian Barlow (:ibarlow)]

No, it's a mockup i did, but I didn't realize there were those limitations to the picker widget. 

I suppose at that point I would just use the numbers above/below widget, with no + or - added, since if you can easily scroll up and down using the picker. Unless I am missing something, I can't see any additional value to the + and - signs in your previous mockups.

Comment 18

[Mark Capella [:capella]]

Ok thanks. I think that's why wesj wanted your input re: plus / minus ... I liked the convenience ... but yes you can scroll for selection, and tapping the edittext field allows quick entry

Comment 19

[Mark Capella [:capella]]

Moving back to wesj queue for review ... after re-reviewing, maybe I've confused things. I think we do need the plus/minus sign.

Scrolling and edit text only allows for input of numbers from 0 to Integer.MAX_VALUE (or 2,147,483,647).

The NumberPicker won't allow for values less than zero, so we can't scroll the whole range of Integer.MIN_VALUE -> 0 -> Integer.MAX_VALUE ... Since we have fields such as |javascript.options.mem.max| that default to -1, we need a way to specify that.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 706407 [details] [diff] [review]
Patch (v4)

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

Playing with this, I think we can use NumberPicker.setFormatter to set a formatter that shows negative values for us:

numberPicker.setFormatter(new NumberPicker.Formatter() {
  public String format(int value) {
    return Integer.toString(Integer.MAX_VALUE/2 - value);
  }
});

but there seem to be issues with the "initial" value in that case (https://code.google.com/p/android/issues/detail?id=35482). Android is also really vague about what picker.setValue() and picker.getValue() do in those cases. I'm debating in my head what's the right answer, but I think I'd rather do this and deal with the initial value problem on its own (since this isn't primary ui yet anyway). I've been trying to figure out what we can do by looking at the NumberPicker source, but no luck yet.

Any ideas Capella?

Comment 21

[Mark Capella [:capella]]

Well I didn't go down that road (quick switch under the cover kinda thing) ... won't it still mask the issue of not being able to input the full range of integer values -2,147,483,648 -> 0 -> 2,147,483,647 ?

Won't it just allow a scroll range of (2,147,483,647 / 2) +/- centered on the current selected |value| ? Or do you think this allows the NumberPicker range to dynamically float centered on that value and still allow a full range of input?

Too bad NumberPicker doesn't use longs internally ...

Comment 22

[Mark Capella [:capella]]

This bug is being closed / merged with the larger overall revamp being done in bug 840144.

Comment 23

[Gervase Markham [:gerv]]

capella: sad to see this bug get so close and then get tied up in a larger patch which is not checked in 5 months later... Any news?

Gerv

Comment 24

[Mark Capella [:capella]]

Oh, yes, I agree, but I'll have to check with wesj as he was reviewing this original patch, as well as offering the WIP for the complete revamp (bug 840144).

I think the revamp patch that I finished from wesj while functional, was too big a change to get review / approval.

We might be able to move this along, I really liked the Java number picker.

Comment 25

[Wesley Johnston (:wesj)]

I think landing this would be useful. I think we got a bit sidetracked trying to make it perfect, when maybe we'd be better to just settle for "good".

Comment 26

[Mark Capella [:capella]]

Attachment: bug770101 v(v5)

Well, someone moved my cheese, but a little refactoring after 5 months s/have been expected :-)

Same solution as before, new NumberInput method different version based on Android OS (NumberPicker class is available from Honeycomb (v11) on forward ... before that we

Comment 27

[Mark Capella [:capella]]

Attachment: bug770101 (v5)

Well, someone moved my cheese, but a little refactoring after 5 months s/have been expected ;-) Same solution as before, new NumberInput method w/different implementation based on Android OS.

NumberPicker class is available from Honeycomb (v11) on forward ... before that we use a simpler numeric input ... both versions default to numeric keyboard only.

Screenshots to follow ...

Comment 28

[Mark Capella [:capella]]

Attachment: Screenshot numeric input via NumberPicker

Comment 29

[Mark Capella [:capella]]

Attachment: Screenshot numeric input w/o NumberPicker

Comment 30

[Wesley Johnston (:wesj)]

Comment on attachment 804465 [details] [diff] [review]
bug770101 (v5)

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

:( I feel bad, but this needs to use the new Prompt.jsm stuff.

::: mobile/android/base/PromptInput.java
@@ +154,5 @@
> +
> +            ToggleButton signButton = new ToggleButton(context);
> +            signButton.setTextOn("+");
> +            signButton.setTextOff("-");
> +            signButton.setChecked(value >= 0);

I still kinda hate this. It looks a bit ugly, but this is fairly hidden... I think we should toss this and just use the number input mode. i.e. the Honeycomb stuff. For anyone entering a number that has a large range anyway, it will make life bearable.

i.e. Sorry for the goose chase :( Lets keep this even simpler and go back to your original v1 patch

::: mobile/android/chrome/content/config.xhtml
@@ +144,5 @@
>                return;
>            }
>  
>            Services.prefs.setBoolPref(aPref.name, result.value);
>          } else {

We should use Prompt.jsm... but it will need to be updated to handle number.
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Prompt.jsm
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm

I say that because it does prompts asynchronously, and we're basically deprecating the old style (I should add a warning when it happens).

Comment 31

[Mark Capella [:capella]]

Attachment: bug770101v6

Git 'er done !!! ;-)

fyi as an aside re: |For anyone entering a number that has a large range anyway, it will make life bearable| ... using the number picker, you wouldn't only have to roll the wheel to select the field value ... you can tap the editText in the center line and it opens for direct numeric input :-)

Comment 32

[Wesley Johnston (:wesj)]

Comment on attachment 804828 [details] [diff] [review]
bug770101v6

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

::: mobile/android/base/PromptInput.java
@@ +13,5 @@
>  import org.json.JSONObject;
>  
>  import android.content.Context;
> +import android.content.res.Configuration;
> +// Why aren't these sorted? ---v

You didn't sort them?

@@ +106,5 @@
> +                               InputType.TYPE_NUMBER_FLAG_SIGNED);
> +            return input;
> +        }
> +
> +        public String getValue() {

I think this is the same as EditInput. r.e. we don't need it?

Comment 33

[Mark Capella [:capella]]

1) Sorted, 2) removed nice catch (could take out of |PasswordInput| in future) 3) push to try ...

Comment 34

[Mark Capella [:capella]]

er, like this: https://tbpl.mozilla.org/?tree=Try&rev=703fc171f792

Comment 35

[Mark Capella [:capella]]

*whew* ! Git 'er done !

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

Comment 36

[Carsten Book [:Tomcat]]

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