730330 - input type='date/time' should use the system date/time picker

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Wesley Johnston (:wesj)]

Right now input type="date/time" give you whatever special keyboard your system provides (which for me ain't really all that special). It might be nice to just pop up the system Date/Time picker dialog.

Note, this is not the same as actually implementing these input types (see bug 446510 and dependents), since I assume they do special things like providing their value to js as a Date object.

Comment 1

[Doug Turner (:dougt)]

not blocking.

Comment 2

[Raphael Catolino (:raphc)]

Attachment: patch

One thing I don't like with this patch is that the PromptService is called from the ime creator. However the date widget is no android IME. It may be interesting to implement the date picking as an ime instead of a classic dialog (basically extending InputMethodService in PromptService). That way the date wouldn't be treated differently from other inputs.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 627676 [details] [diff] [review]
patch

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

Looks good! Glad to see this happening.

> It may be interesting to implement the date picking as an ime instead of a classic dialog

I think this is true, but we don't usually write our own IME's. Instead we use what the user chose (i.e. what the system gives us). From my experience, most IME's don't bother to implement this. It might be nice to see if we can query if this one does, and if so use their UI instead. Mounir said that his phone's IME has one, and given he's in charge of this I'm guessing he doesn't like it?

::: mobile/android/base/GeckoInputConnection.java
@@ +793,5 @@
> +                geckoMsg.getJSONObject("gecko").put("inputs",dateArray);
> +                geckoMsg.getJSONObject("gecko").put("buttons",buttonArray);
> +                Log.d(LOGTAG,"Date Prompt Gecko Message: "+geckoMsg);
> +                JSONObject value = new JSONObject(
> +                    GeckoAppShell.handleGeckoMessage(geckoMsg.toString()));

I'd rather you just use the java PromptService directly and use PromptService.waitForReturn(); In fact, I think if we basically create a second, non-JSON constructor for PromptInput and PromptButton (PromptButton is a stupid class that should go away and be replaced by a String[]), you could avoid all of this initial JSON.

You will still need to parse the return though. I should eventually have a way to specific a native Hashtable return as well (or figure out to use one based on the constructor used....)

::: mobile/android/base/PromptService.java
@@ +206,5 @@
> +            } else if (type.equals("date")) {
> +                DatePicker dp = (DatePicker)view;
> +                return String.format("%04d",dp.getYear())+"-"+
> +                  String.format("%02d",dp.getMonth())+"-"+
> +                  String.format("%02d",dp.getDayOfMonth());

Would be nice to use http://developer.android.com/reference/java/text/DateFormat.html, Eventually we may need to return this in a form that Gecko can also easily convert to a PR_Time?

Comment 4

[Raphael Catolino (:raphc)]

>I think this is true, but we don't usually write our own IME's. Instead we use what the user chose (i.e. what the system gives us). From my experience, most IME's don't bother to implement this. It might be nice to see if we can query if this one does, and if so use their UI instead. Mounir said that his phone's IME has one, and given he's in charge of this I'm guessing he doesn't like it?

On my phone the standard date IME consist of a keypad with numbers and a '/' so it's not quite as nice as a date picker widget. I thought it would be possible to extend it so that it can use the system widget instead. It shouldn't change anything from the user pov (except it might me made non-modal). Of course if the phone's IME already provides it, it would be better use that. However I'm not sure how it can be done, we can query whether a specific input type is enabled, but I don't think we can query how it does the input (date widget or specialized keyboard).

>I'd rather you just use the java PromptService directly and use PromptService.waitForReturn(); In fact, I think if we basically create a second, non-JSON constructor for PromptInput and PromptButton (PromptButton is a stupid class that should go away and be replaced by a String[]), you could avoid all of this initial JSON.

Yes, it seems rather stupid to communicate via JSON messages between java methods.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 627676 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoInputConnection.java
@@ -982,5 @@
>          View v = GeckoApp.mAppContext.getLayerController().getView();
>  
>          if (v == null)
>              return;
> -

Please, remove this empty line.

::: mobile/android/base/PromptService.java
@@ +206,5 @@
> +            } else if (type.equals("date")) {
> +                DatePicker dp = (DatePicker)view;
> +                return String.format("%04d",dp.getYear())+"-"+
> +                  String.format("%02d",dp.getMonth())+"-"+
> +                  String.format("%02d",dp.getDayOfMonth());

Actually, we need something that matches the format requested by the HTML specifications. Using DateFormat would be a good approach because that would allow us to easily handle all date/time types (week, month, etc.).

Comment 6

[Raphael Catolino (:raphc)]

Attachment: patch

I started to add the time support, but it seems that the TimePicker widget does not support seconds and miliseconds input, if we want to use a widget for this input we'll have to extend the standard one.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Raphael Catolino from comment #6)
> Created attachment 627900 [details] [diff] [review]
> patch
> 
> I started to add the time support, but it seems that the TimePicker widget
> does not support seconds and miliseconds input, if we want to use a widget
> for this input we'll have to extend the standard one.

This isn't a problem. As you can see here [1], a time can be represented without seconds and milliseconds and still be valid regarding the specifications. In addition, by default, <input type='time'> shouldn't allow seconds and milliseconds precision if I understand the specifications correctly (default step is 1 minute) [2]. Finally, I think it is a good UI to not show seconds and milliseconds: this is a precision users are usually not interested of and don't really care about. However, scripts might want to set more precise time.

This said, we could open a follow-up that would show a widget with seconds/milliseconds using some computations (using the min and step attributes) to know if this precision is relevant.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#concept-time
[2] http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#time-state-%28type=time%29

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Wesley Johnston (:wesj) from comment #3)
> > It may be interesting to implement the date picking as an ime instead of a classic dialog
> 
> I think this is true, but we don't usually write our own IME's. Instead we
> use what the user chose (i.e. what the system gives us). From my experience,
> most IME's don't bother to implement this. It might be nice to see if we can
> query if this one does, and if so use their UI instead. Mounir said that his
> phone's IME has one, and given he's in charge of this I'm guessing he
> doesn't like it?

Actually, the IME I get for date/time inputs is like the one Raphael described. This is very far from what I would expect and doesn't prevent the user to set a value not allowed by the HTML specifications.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 627900 [details] [diff] [review]
patch

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

Looking good!

::: mobile/android/base/GeckoInputConnection.java
@@ +809,5 @@
> +    private void displayPrompt(String inputType) {
> +        final PromptService prompt = GeckoAppShell.getPromptService();
> +        Editable content = getEditable();
> +        String[] buttons = {"Set","Cancel"};
> +        prompt.SetButtons(buttons);

I don't love this, but I can't hate it because its mostly my fault. The main reason inputs are stored in the PromptService was because it made it easier to loop through them and get their values when the dialog closed. Otherwise we have to get all the child views on the dialog and loop over them somehow....

Buttons don't really need that. But! I've distracted you to much from your bug and can live with it for now.

::: mobile/android/base/PromptService.java
@@ +182,5 @@
> +                DatePicker input = new DatePicker(GeckoApp.mAppContext);
> +                if (!value.equals("")) {
> +                    try {
> +                        GregorianCalendar calendar = new GregorianCalendar();
> +                        calendar.setTime(new SimpleDateFormat("yyyy-MM-dd").parse(value));

This bit scares me. Any idea what happens if the site pre-populates the field with something wrong? Eventually I guess Gecko will send us a Julian date, so maybe its not worth worrying about yet...

@@ +249,5 @@
> +            } else if (type.equals("date")) {
> +                DatePicker dp = (DatePicker)view;
> +                GregorianCalendar calendar = 
> +                    new GregorianCalendar(dp.getYear(),dp.getMonth(),dp.getDayOfMonth());
> +                return new SimpleDateFormat("yyyy-MM-dd").format(calendar.getTime());

I still think we can get this from the system:

http://stackoverflow.com/questions/2117565/displaying-dates-in-localized-format-on-android

(maybe I liked to Java's DateFormat object before....) Or maybe HTML5 forces a particular format?

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Wesley Johnston (:wesj) from comment #9)
> [...] Or maybe HTML5 forces a particular format?

It does.

Comment 11

[Raphael Catolino (:raphc)]

>This bit scares me. Any idea what happens if the site pre-populates the field with >something >wrong? Eventually I guess Gecko will send us a Julian date, so maybe its not >worth worrying >about yet...

The value is parsed against "yyyy-MM-dd", which means that if the string contains an other char than [0-9] or - an exception is thrown and the preset value get discarded when the user set his own. Then if the string is not a valid date, parse() returns null and the calendar is set on today's date. However the parsing algorithm of SimpleDateFormat has a peculiar understanding of a valid date, that is yyyy, MM and dd can be any value decimal values. If, for example, dd>31 MM is incremented and dd is set on dd modulo 31, same thing for MM (yyyy can be any decimal value). This is not the behavior most users would expect, but I don't think it could be harmful. Anyway, I wonder whether this is really a UI problem, the value must be sanitized as specified in html5 by gecko at some point before we get it.

Testing this has made me notice that the default date range allowed by the date picker widget is between 1900 and 2100. Someone might very well want to set some other date, however the functions use to change this default are not available with the android api level fennec is using. There seems to be no trivial way to modify this range without using an xml resource file, and since I will have to extend the date picker widget for month/week I'm thinking I might as well do that now, and patch all of those at the same time.

Comment 12

[Raphael Catolino (:raphc)]

Attachment: Sources from android

Ok, so I made a custom widget to be used with any of input type=date/time/datetime/month/week ui. I used the android DatePicker.java code as base and adapted it. The NumberPicker class hasn't been made available until sdk 11, so I reused this code as well to make a NumberPicker on platforms using an older sdk.
This patch contains the sources (and the associated resources files) I got from android without any modifications (Excepted the DatePicker name change to DatetimePicker).

Comment 13

[Raphael Catolino (:raphc)]

Attachment: Custom date picker widget

This patch contains all code relative to integration of the NumberPicker class, and the needed modifications to the DatePicker class.

Comment 14

[Raphael Catolino (:raphc)]

Attachment: Date picker widget use

And this one contains the changes to the preexisting fennec code to use the new widget.

Comment 15

[Raphael Catolino (:raphc)]

Attachment: Custom date picker widget

Comment 16

[Raphael Catolino (:raphc)]

Attachment: Date picker widget use

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 631740 [details] [diff] [review]
Sources from android

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

I hate that we have to do this, but NumberPicker is API v11+ I guess, and I like it better than the crap my phone has. Can we put the widget things in a widget subfolder (mobile/android/base/widget/). It would be nice if we could be careful and only use this on pre-v11 stuff so that 1.) we get native themeing in future versions and 2.) We can eventually delete a bunch of this.

Comment 18

[Wesley Johnston (:wesj)]

So... I really hate bringing all this code in just for this. I need to think about it a little bit I guess, but can we have a little discussion about how important it is to support these oddball types? Or is there some way we can do it without supporting them?

I am willing to throw away some of these "number pickers act different on different android version" things in order to simplify the code somewhat. I'd love it if we could do this same thing without having to bring in the number picker stuff somehow.

Comment 19

[Raphael Catolino (:raphc)]

(In reply to Wesley Johnston (:wesj) from comment #18)
> So... I really hate bringing all this code in just for this. I need to think
> about it a little bit I guess, but can we have a little discussion about how
> important it is to support these oddball types? 

> I am willing to throw away some of these "number pickers act different on
> different android version" things in order to simplify the code somewhat.
> I'd love it if we could do this same thing without having to bring in the
> number picker stuff somehow.

Month and week (and datetime I guess) may not be that useful, however I think that date and time are. A nice ui for those would really be appreciated.
Using the stocks widget for them has a major drawback: in API < 11 one can not set the min and max date dynamically, it has to be done in res file. As a result I used a custom date picker widget in the patch. Once that is done, supporting all of those types doesn't really add that much code.

I guess we could just drop fancy support for api < 11 and just provide them with the stock ime allowing to enter date/times. It may imply some code in handling the result, but it would certainly be nothing as big as the NumberPicker widget. It would also allow us to only implement one behavior for the date picker, but mainly it would allow us to get rid of the android imported NumberPicker class. That would definitely be nice.
However implementing a custom DatePicker widget is not really an option if we want a nice ui (especially for month and week), even in api>11. I guess we could manage something by just playing with views in PromptService but we would just move some code around. That said, I think we could reduce the DatePicker code by stripping it of all methods we don't explicitly need for the date/time/datetime/month/week ui.

Now I guess we could just drop fancy support for api < 11 and provide them with the stock ime allowing to enter date/times. It may imply some code in handling the result, but it would certainly be nothing as big as the NumberPicker widget. It would also allow us to only implement one behavior for the date picker, but mainly it would allow us to get rid of the android imported NumberPicker class. That would definitely be nice.
However implementing a custom DatePicker widget is not really an option if we want a nice ui (especially for month and week), even in api>11. I guess we could manage something by just playing with views in PromptService but we would just move some code around. That said, I think we could reduce the DatePicker code by stripping it of all methods we don't explicitly need for the date/time/datetime/month/week ui.

Finally we could also keep fancy support only for date and time (datetime might be manageable too)in API>11 and fallback on classic IME for everything else. I'd like to know how big the part of firefox mobile users stuck on android 2.3- really is though.

Comment 20

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

Could we use something like this and skip adding the android code?
http://code.google.com/p/datetimepicker/

it seems like we could get a simple UI on android 2.3 and lower.

Comment 21

[Raphael Catolino (:raphc)]

Yep it would work for datetime, but not for week and month (and still wouldn't allow min/max date attributes on android 2.3 and lower, but that is not a big problem). 
But then again week and month are not that much used, so I'm ok with that solution.
 
In http://code.google.com/p/datetimepicker/ the author use a view switcher, i guess that allows him not to deal with the relative placements of the date picker (with the calendar view) and the time picker. It probably simplify the code but it must feel strange on large devices, having to switch between date and time when there is room enough to display both. And even on phones we could display both the date and time pickers since there's not enough room to display a calendar view anyway. Do you think it's worth the effort of handling the different layouts on different screens?

Comment 22

[Mounir Lamouri (:mounir)]

Those input types are important especially for mobile. Current date/time pickers render very badly on mobile and using native widgets would make UX better. Without a good implementation, web authors are not going to use those types. We have a good opportunity to help improve the general UX for mobile users.

I wouldn't support dropping some types because they feel less useful. However, dropping support for some version of Androids is a call I would let Mobile peers to take but IMO, that would be bad for UX and therefore for the product.

Regarding the general UX, I think we should target at least what Chrome Beta is doing which means having a native widget for all types except week. Given that Raphael did implement a support for week, I don't think it would make sense to drop that.

Comment 23

[Wesley Johnston (:wesj)]

I'm fine with that then. I'll review as is on Monday.

Comment 24

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

Comment 25

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 26

[Raphael Catolino (:raphc)]

I cleaned a few things up in attachment 640062 [details] [diff] [review]. And i rebased everything against a up to date tree. 
Some changes in GeckoApp.java / GeckoInputConnection.java have broken attachment 640063 [details] [diff] [review] though.
I was using a PromptService in GeckoInputConnection that is not available anymore in the new version. I then added a getter in GeckoApp to be able to use it from GeckoInputConnection but while it seems to work when the dialog is created, fennec closes randomly after a few moments if the widget is not closed.
Logcat hasn't been much help on this, but the randomness of the crash make me think of a GC problem, as if the dialog was freed even though we still need it.

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 640062 [details] [diff] [review]
custom date/time picker

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

Overall looks good. Mostly just questions throughout. No r+ yet because I just want to look at it again when you're done (its big and reading through it over and over is good). I'm also gonna have mark look at it.

I'll build with it now and look at the crash as well. I remember also seeing one myself (on GB devices?) when this first went up because of something to do with the min/max values, so I'm gonna look for that too.

::: mobile/android/base/DatetimePicker.java
@@ +1,2 @@
>  /*
> +  Copyright (C) 2007 The Android Open Source Project

Put this little guy back.

@@ +17,2 @@
>  
> +package org.mozilla.gecko;

I want to put start putting these things into an org.mozilla.gecko.view namespace I think. As well as putting them in a "view" folder.

@@ +64,5 @@
> +    private boolean DAY_ENABLED = true;
> +    private boolean HOUR_ENABLED = true;
> +    private boolean MINUTE_ENABLED = true;
> +    private boolean SECOND_ENABLED = true;
> +    private boolean CALENDAR_ENABLED = false;

We typically only use all caps for constants.

@@ +66,5 @@
> +    private boolean MINUTE_ENABLED = true;
> +    private boolean SECOND_ENABLED = true;
> +    private boolean CALENDAR_ENABLED = false;
> +    private int mScreenWidth;
> +    private int mScreenHeight;

A note about the units there are in is helpful. We should use screen dpi to put these in inches. Same for LIMIT_CALENDAR_WIDTH above.

@@ +102,5 @@
> +            mTempDate.setTimeInMillis(mCurrentDate.getTimeInMillis());
> +            if (Build.VERSION.SDK_INT > 10) {
> +                Log.d(LOGTAG,"Sdk version > 10, using new behavior");
> +                //The native date picker widget on these sdks increment
> +                //the next field when one field reach the maximum

Hmm... I'm confused about what we're doing here and having trouble testing right now. This reads to me like incrementing past Jan 31 will take you to February 1. To fix it, we're flipping the day of month back to 1? If that's all, maybe we just leave it?

I see this is in the stock implementation though, so I'm guessing that I misunderstand. Maybe we can at least clean it up a bit with a helper function. Something like:

void setTempDate(int mode, int val, int min, int max, bool useFancyGingerbreadNumberpickers)

@@ +204,5 @@
>      }
>  
>      public DatetimePicker(Context context) {
> +        super(context);
> +        Log.d(LOGTAG,"Building date picker");

Logging is great. We should probably hide it behind a if (DEBUG) clause though.

@@ +207,5 @@
> +        super(context);
> +        Log.d(LOGTAG,"Building date picker");
> +        setCurrentLocale(Locale.getDefault());
> +        mMinDate.set(DEFAULT_START_YEAR,1,1);
> +        mMaxDate.set(DEFAULT_END_YEAR,1,1);

Jan 1 is an interesting end date. Can we make it Dec 31?

@@ +212,5 @@
> +        LayoutInflater inflater = (LayoutInflater) context
> +                .getSystemService(Context.LAYOUT_INFLATER_SERVICE);
> +        inflater.inflate(R.layout.datetime_picker, this, true);
> +        Log.d(LOGTAG,"View inflated");
> +        onValueChangeListener onChangeListener = new onValueChangeListener();

Should this class be "OnValueChangeListener"?

@@ +224,5 @@
> +
> +        WindowManager wm = (WindowManager) context.getSystemService(Context.WINDOW_SERVICE);
> +        Display display = wm.getDefaultDisplay();
> +        mScreenWidth = display.getWidth();
> +        mScreenHeight = display.getHeight();

We will need to adjust these, and the constants using DPI so that these are essentially in inches.

@@ +226,5 @@
> +        Display display = wm.getDefaultDisplay();
> +        mScreenWidth = display.getWidth();
> +        mScreenHeight = display.getHeight();
> +        Log.d(LOGTAG,"screen width: "+mScreenWidth+" screen height: "+mScreenHeight);
> +        if (Build.VERSION.SDK_INT > 10 && mScreenWidth > LIMIT_CALENDAR_SCREEN_WIDTH) {

This function needs a little whitespace and comment love. Add a blank line before the if section starts, and more throughout where we can split up sections. I see you pulled some comments out of the original source as well. If they're useful, we should leave them in.

@@ +227,5 @@
> +        mScreenWidth = display.getWidth();
> +        mScreenHeight = display.getHeight();
> +        Log.d(LOGTAG,"screen width: "+mScreenWidth+" screen height: "+mScreenHeight);
> +        if (Build.VERSION.SDK_INT > 10 && mScreenWidth > LIMIT_CALENDAR_SCREEN_WIDTH) {
> +            Log.d(LOGTAG,"SDK recent, displaying calendar");

These log statements can be a bit more specific too. i.e. "SDK > 10" is better than SDK recent.

@@ +244,5 @@
> +                    notifyDateChanged();
> +                }
> +            });
> +            mPickers.addView(mCalendar);
> +            setCalendarShown(true);

Can you post some screenshots of this calendar? If I remember right, we're showing a date picker widget + a calendar and updating them in tandem whenever the other changes? I think we probably only want one or the other. Having two different methods for inputting a date is confusing. But maybe I don't understand it.

I'd much rather pick dates from a calendar if it doesn't suck.... Especially if we can use it to pick weeks. But that's a good follow up...

@@ +261,5 @@
> +        mDaySpinner.setMaxValue(mTempDate.getActualMaximum(Calendar.DAY_OF_MONTH));
> +        mDaySpinner.setOnValueChangeListener(onChangeListener);
> +        mDaySpinner.setSpeed(100);
> +        mDaySpinnerInput = mDaySpinner.getInputField();
> +

Maybe we can move all this repeated work into its own function. Something like:

mDaySpinner = setupSpinner(R.id.day, min, max);

@@ +435,4 @@
>              return;
>          }
> +        if (shown){
> +            CALENDAR_ENABLED = true;

I think you can simplify a lot of these to something like:

CALENDAR_ENABLED = shown;
mCalendar.setVisibility(shown ? VISIBLE : GONE);

@@ +449,5 @@
> +            mYearSpinner.setVisibility(VISIBLE);
> +            YEAR_ENABLED = true; 
> +        } else {
> +            mYearSpinner.setVisibility(GONE);
> +            setMonthShown(false);

I'm not sure why we're turning off month in this function. I don't think we should. Callers should have to enable/disable what they want. This seems likely to lead to headaches for someone trying to use this.

@@ +456,5 @@
>      }
>  
> +    public void setWeekShown(boolean shown) {
> +        if (shown) {
> +            Log.d(LOGTAG,"setting visibility to visible");

Make sure logging makes sense without a lot of context around it.

@@ +558,5 @@
>          }
>      }
>  
> +    public void updateDate(Calendar calendar) {
> +        if (mCurrentDate.equals(calendar)){

Space after ))

::: mobile/android/base/Makefile.in
@@ +38,5 @@
>    db/AndroidBrowserDB.java \
>    db/BrowserDB.java \
>    db/LocalBrowserDB.java \
>    db/DBUtils.java \
> +	DatetimePicker.java \

Make sure these are spaces, not tabs.

@@ +78,5 @@
>    MultiChoicePreference.java \
>    NSSBridge.java \
> +	NumberPickerHolder.java \
> +	NumberPicker.java \
> +	NumberPickerButton.java \

Same here. spaces not tabs.

::: mobile/android/base/NumberPicker.java
@@ +36,5 @@
>  
>  /**
>   * A view for selecting a number
>   *
>   * For a dialog using this view, see {@link android.app.TimePickerDialog}.

Clean up this comment stuff for Android documentation.

@@ +236,5 @@
>      /**
>       * Set the callback that indicates the number has been adjusted by the user.
>       * @param listener the callback, should not be null.
>       */
> +    public void setOnValueChangeListener(OnValueChangeListener listener) {

Why are you changing this? I think OnChangeListener is fine (and we'll save us from someone getting hurt in the future who doesn't realize we've rolled our own dateTimePickers).

@@ +302,5 @@
> +        }
> +    }
> +
> +    public int getMinValue() {
> +        return mStart;

These should probably mMin and mMax.

@@ +334,5 @@
>          }
>          mCurrent = current;
>          updateView();
>      }
> +    public void setValue(int current) {

Space between methods. How come the Android implementation doesn't have this already?

@@ +373,5 @@
>              current = mEnd;
>          }
>          mPrevious = mCurrent;
>          mCurrent = current;
> +        updateView();

Why'd you move this? Add a comment if its important that it happens first.

@@ +425,1 @@
>              // Restore to the old value as we don't allow empty values

Leave the whitespace

::: mobile/android/base/NumberPickerHolder.java
@@ +9,5 @@
> +import android.widget.LinearLayout;
> +import android.widget.EditText;
> +
> +public class NumberPickerHolder extends LinearLayout{
> +

Space after LinearLayout. This class basically bridges content between the native number picker and our implementation? I'm not sure I totally understand, but I really wish we could just make our interfaces line up so that callers didn't have to worry about differences at all. i.e. org.mozilla.gecko.NumberPicker should implement android.widget.NumberPicker. At the very least, internally to this class, I don't really want to hold on to an androidFormatter/Listener/NP and an mozillaFormatter/Listener/NP. Feel free to tell me this is the only pretty way to do it, but I'd love it if we could step around this?

@@ +43,5 @@
> +                int oldVal, int newVal){
> +            mOnValueChangeListener.onValueChange(holder,oldVal,newVal);
> +        }
> +    }
> +//Why not merge?

Yes. Why not?

@@ +62,5 @@
> +
> +    private void init(){
> +        if (Build.VERSION.SDK_INT > 10){
> +            Log.d(LOGTAG,"Using new native date picker");
> +            recentSdk = true;

Version 10 will not always be a recent SDK (in fact, its already not). Use a more descriptive boolean like supportsNativeNumberPicker.

::: mobile/android/base/strings.xml.in
@@ +156,5 @@
>    <string name="button_ok">&button_ok;</string>
>    <string name="button_cancel">&button_cancel;</string>
>    <string name="button_clear_data">&button_clear_data;</string>
> +  <string name="button_set">&button_set;</string>
> +  <string name="button_clear">&button_clear;</string>

I'm not a huge fan of the "clear" button, but I'm willing to let that ride to get this landed.

Comment 28

[Raphael Catolino (:raphc)]

Attachment: android sources

moved the widget stuff to a base/widget

Comment 29

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

Comment 30

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 31

[Raphael Catolino (:raphc)]

(In reply to Wesley Johnston (:wesj) from comment #27)

> 
> I'll build with it now and look at the crash as well. I remember also seeing
> one myself (on GB devices?) when this first went up because of something to
> do with the min/max values, so I'm gonna look for that too.
>
I saw that the crash was happening when calling processNextNativeEvent() in PromptService.waitForReturn. I've not looked into that code so I still don't know what is actually causing the crash, but it seems to get more stable when augmenting the polling interval. I was able to test using 200 ms without having any problems.

> ::: mobile/android/base/DatetimePicker.java

> 
> @@ +17,2 @@
> >  
> > +package org.mozilla.gecko;
> 
> I want to put start putting these things into an org.mozilla.gecko.view
> namespace I think. As well as putting them in a "view" folder.
I put them in a widget folder, it does a little more than just a view.

> @@ +66,5 @@
> > +    private boolean MINUTE_ENABLED = true;
> > +    private boolean SECOND_ENABLED = true;
> > +    private boolean CALENDAR_ENABLED = false;
> > +    private int mScreenWidth;
> > +    private int mScreenHeight;
> 
> A note about the units there are in is helpful. We should use screen dpi to
> put these in inches. Same for LIMIT_CALENDAR_WIDTH above.
I actually used the different screen classes the android sdk recognizes. That way we don't
have to choose an arbitrary width limit. I still use the width/length in pixel to check the orientation. There must be a cleaner way though. I'll probalby change that.
I have not been able to install fennec on my emulator (and I didn't find time to fix it), so I did not try on any large device though. This code needs some testing.

> @@ +102,5 @@
> > +            mTempDate.setTimeInMillis(mCurrentDate.getTimeInMillis());
> > +            if (Build.VERSION.SDK_INT > 10) {
> > +                Log.d(LOGTAG,"Sdk version > 10, using new behavior");
> > +                //The native date picker widget on these sdks increment
> > +                //the next field when one field reach the maximum
> 
> Hmm... I'm confused about what we're doing here and having trouble testing
> right now. This reads to me like incrementing past Jan 31 will take you to
> February 1. To fix it, we're flipping the day of month back to 1? If that's
> all, maybe we just leave it?
No the overflow on the next field is actually the behavior we want to have (at least that's the standard behavior on the platform).

> I see this is in the stock implementation though, so I'm guessing that I
> misunderstand. Maybe we can at least clean it up a bit with a helper
> function. Something like:
> 
> void setTempDate(int mode, int val, int min, int max, bool
> useFancyGingerbreadNumberpickers)

Yep it's cleaner that way, I did not use it with the old behavior as the code is really specific for month and year, and really simple for the other fields.

> @@ +224,5 @@
> > +
> > +        WindowManager wm = (WindowManager) context.getSystemService(Context.WINDOW_SERVICE);
> > +        Display display = wm.getDefaultDisplay();
> > +        mScreenWidth = display.getWidth();
> > +        mScreenHeight = display.getHeight();
> 
> We will need to adjust these, and the constants using DPI so that these are
> essentially in inches.

See my comment above.

> @@ +244,5 @@
> > +                    notifyDateChanged();
> > +                }
> > +            });
> > +            mPickers.addView(mCalendar);
> > +            setCalendarShown(true);
> 
> Can you post some screenshots of this calendar? If I remember right, we're
> showing a date picker widget + a calendar and updating them in tandem
> whenever the other changes? I think we probably only want one or the other.
> Having two different methods for inputting a date is confusing. But maybe I
> don't understand it.

Sadly I can't right now. I'll probably have my emulator working again by tomorrow.
Yes it does not seem that useful. What about datetime, should we have time pickers + calendar or date and time pickers?

> I'd much rather pick dates from a calendar if it doesn't suck.... Especially
> if we can use it to pick weeks. But that's a good follow up...

The calendar is nice from what I remember, but I don't think we can choose the week.

> @@ +435,4 @@
> >              return;
> >          }
> > +        if (shown){
> > +            CALENDAR_ENABLED = true;
> 
> I think you can simplify a lot of these to something like:
> 
> CALENDAR_ENABLED = shown;
> mCalendar.setVisibility(shown ? VISIBLE : GONE);
> 
> @@ +449,5 @@
> > +            mYearSpinner.setVisibility(VISIBLE);
> > +            YEAR_ENABLED = true; 
> > +        } else {
> > +            mYearSpinner.setVisibility(GONE);
> > +            setMonthShown(false);
> 
> I'm not sure why we're turning off month in this function. I don't think we
> should. Callers should have to enable/disable what they want. This seems
> likely to lead to headaches for someone trying to use this.

Even if some configurations don't make any sense? Like having only a year and a day for instance. 

> ::: mobile/android/base/NumberPicker.java
> @@ +36,5 @@
> >  
> >  /**
> >   * A view for selecting a number
> >   *
> >   * For a dialog using this view, see {@link android.app.TimePickerDialog}.
> 
> Clean up this comment stuff for Android documentation.

Do you mean all the javadoc-like comments?

> @@ +236,5 @@
> >      /**
> >       * Set the callback that indicates the number has been adjusted by the user.
> >       * @param listener the callback, should not be null.
> >       */
> > +    public void setOnValueChangeListener(OnValueChangeListener listener) {
> 
> Why are you changing this? I think OnChangeListener is fine (and we'll save
> us from someone getting hurt in the future who doesn't realize we've rolled
> our own dateTimePickers).
> 

> @@ +334,5 @@
> >          }
> >          mCurrent = current;
> >          updateView();
> >      }
> > +    public void setValue(int current) {
> 
> Space between methods. How come the Android implementation doesn't have this
> already?

The api has changed after the skd 10. And since the api before wasn't public I don't think we should keep the compatibility with that one, but rather with the new one. (which has OnValueChangeListener and setValue instead of setCurrent)

> @@ +373,5 @@
> >              current = mEnd;
> >          }
> >          mPrevious = mCurrent;
> >          mCurrent = current;
> > +        updateView();
> 
> Why'd you move this? Add a comment if its important that it happens first.

Done. And it actually make more sense that way IMO. First do all the internal computations then notify the listeners. It's also implemented that way in the all new version. https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/NumberPicker.java#L1521

> ::: mobile/android/base/NumberPickerHolder.java
> @@ +9,5 @@
> > +import android.widget.LinearLayout;
> > +import android.widget.EditText;
> > +
> > +public class NumberPickerHolder extends LinearLayout{
> > +
> 
> Space after LinearLayout. This class basically bridges content between the
> native number picker and our implementation? I'm not sure I totally
> understand, but I really wish we could just make our interfaces line up so
> that callers didn't have to worry about differences at all. i.e.
> org.mozilla.gecko.NumberPicker should implement android.widget.NumberPicker.

That would be better, but since NumberPicker is not public until sdk > 11, instantiating a class implementing it would probly cause a crash when using the old sdk.
This wrapper just allows to dynamically load either the native one if it's available or our own otherwise. I did keep the same api than the NumberPicker class (the new, public, one I mean) so that if we one day decide to drop support for old sdk we can just replace the numberPickerHolder class by the NumberPicker one without changing any of the actual calls to its methods.
I'm not a big fan of this solution though and I'm open to everything cleaner.

> At the very least, internally to this class, I don't really want to hold on
> to an androidFormatter/Listener/NP and an mozillaFormatter/Listener/NP. Feel
> free to tell me this is the only pretty way to do it, but I'd love it if we
> could step around this?
> 
> @@ +43,5 @@
> > +                int oldVal, int newVal){
> > +            mOnValueChangeListener.onValueChange(holder,oldVal,newVal);
> > +        }
> > +    }
> > +//Why not merge?
> 
> Yes. Why not?

Same problem I can't instanciate a class implementing a non-existing class. (ie NumberPicker with sdk < 11). I just forgot to remove the comment.
I more or less based myself on http://android-developers.blogspot.fr/2009/04/backward-compatibility-for-android.html

> ::: mobile/android/base/strings.xml.in
> @@ +156,5 @@
> >    <string name="button_ok">&button_ok;</string>
> >    <string name="button_cancel">&button_cancel;</string>
> >    <string name="button_clear_data">&button_clear_data;</string>
> > +  <string name="button_set">&button_set;</string>
> > +  <string name="button_clear">&button_clear;</string>
> 
> I'm not a huge fan of the "clear" button, but I'm willing to let that ride
> to get this landed.

I believe we're supposed to let users set the value to the empty string, if someone mistakenly starts entering a date we must allow him to clear it.

I have not been able to test those changes on larger devices so this patch might well be broken for those. I'll update tomorrow.

Comment 32

[Wesley Johnston (:wesj)]

Great and Thanks! I'll read through this again this afternoon too, and I'll install on my tablet and Galaxy Note to test.

Comment 33

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

I mainly replaced the code that checked the size of the devices, I now use the size in inches.

Comment 34

[Raphael Catolino (:raphc)]

Attachment: segfault stacktrace

So I did some testing (on a galaxy s2) to see why the input widgets were crashing after a few moments.
First I noticed some interesting facts, that did not help me that much, but still :

- It won't crash if the phone is not plugged in my computer. It won't crash if it's only plugged to a power source either. Does the fact that it is plugged to a computer has any influence on what code is executed?

- As mentioned in comment 32, it won't crash if the call to nsAppShell::ProcessNextNativeEvent are less frequent. And it won't crash as much on a tablet.

- The file picker widget is clearly separated from the date/time picker widgets, but it seems to be the only other one to use PromptService, and it doesn't have this problem.
So I changed the code to call a date picker widget instead of a file picker, and it doesn't crash either. The widget seems to be must faster called from this point (it rotates instantaneously instead of waiting 10 seconds to do so, for example) and since the performance seems to impact whether the call to processNextNativeEvent will cause a crash or not, I'm not too sure what to make of it.

I managed to get a backtrace using gdb, which shows that the app segfault when the next event in processNextNativeEvent is a SCREENSHOT, 
> at /home/raph/workspace/mozilla-mobile/widget/android/nsAppShell.cpp:502
it seems that sometimes MessageLoop::current() returns NULL instead of a pointer to the MessageLoop (because it has been freed?) And then the call to MessageLoop::current()->PostIdleTask() segfault.

I can't see why the MessageLoop wouldn't have any instances at this point.
In the mean time I added a test to check whether the MessageLoop::current() exists before calling PostIdleTask. It's more of hack than a solution though.

Comment 35

[Wesley Johnston (:wesj)]

raphc, last i talked to you, you were going to look at moving the initialization code into browser.js rather than the InputMethodManager. How is that going?

Comment 36

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

Comment 37

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 38

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

(In reply to Wesley Johnston (:wesj) from comment #35)
> raphc, last i talked to you, you were going to look at moving the
> initialization code into browser.js rather than the InputMethodManager. How
> is that going?

Also, I'm not 100% certain, but I think Mounir and I were talking on IRC about getting these changes landed without using any custom datetime picker as a first step. We would just use the whatever native UI Android supported, even if that meant not supporting the best UI on older versions of Android.

Is that what these patches implement?

Comment 39

[Raphael Catolino (:raphc)]

Yep that's what I did. The display of all these widgets is now triggered by an event listener in browser.js, just like it's done for the select element.
The IME was still brought up by the Editor, so I had to add a check to prevent it in GeckoInputConnection. I think this could be done earlier, in nsEditor.cpp, but I wasn't sure of all the implications in removing all the IME management stuff for this input elements there.

This still feels like a hack, but it's a more stable, and more efficient one. The widget is more responsive (doesn't take 10 seconds to rotate for instance) and I haven't experienced any crash so far. Anyway there shouldn't be any more thread-safety issues as the widgets are now created in the same thread as the other ones.

Comment 40

[Raphael Catolino (:raphc)]

(In reply to Mark Finkle (:mfinkle) from comment #38)
> (In reply to Wesley Johnston (:wesj) from comment #35)
> > raphc, last i talked to you, you were going to look at moving the
> > initialization code into browser.js rather than the InputMethodManager. How
> > is that going?
> 
> Also, I'm not 100% certain, but I think Mounir and I were talking on IRC
> about getting these changes landed without using any custom datetime picker
> as a first step. We would just use the whatever native UI Android supported,
> even if that meant not supporting the best UI on older versions of Android.
> 
> Is that what these patches implement?

No this would need some cleanup.
Just for info, this would probably mean a full support for SDK version > 11, a full support for time, and a partial support for date (maybe datetime too) for SDK < 11. The ui would still be the same (native one).
I think this can be done pretty easily. I'll try to post some new patches for this soon.

Comment 41

[Wesley Johnston (:wesj)]

Comment on attachment 648724 [details] [diff] [review]
date picker widget use

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

Looks good. I assume the PromptService.java stuff will change (very very sorry for all the churn :( ). cpeterson should approve the finishComposingText stuff. Might as well do that now since it will probably not change, I'll add him now!

::: mobile/android/chrome/content/InputWidgetHelper.js
@@ +14,5 @@
> +    // if we're busy looking at a InputWidget we want to eat any clicks that
> +    // come to us, but not to process them
> +    if (this._uiBusy || !this._isValidInput(aTarget)) {
> +      return;
> +    }

We dont use braces for one line if's on mobile. Drop 'em throughout this.

@@ +22,5 @@
> +    this._uiBusy = false;
> +  },
> +
> +  show: function(aElement) {
> +    let list = {

Just call this msg.

Comment 42

[Wesley Johnston (:wesj)]

Comment on attachment 648723 [details] [diff] [review]
custom date/time picker

Hopefully we don't need either of these then. You can do the input check in browser.js using Services.appinfo (which returns a nsIXULAppInfo object):

http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULAppInfo.idl

Or you could try having the prompt service return quickly (if the IME keyboard works....).

Comment 43

[Wesley Johnston (:wesj)]

Comment on attachment 641120 [details] [diff] [review]
android sources

Sounds like we don't need this either....

Comment 44

[Raphael Catolino (:raphc)]

Attachment: DatePicker source

We still need to use a custom date picker to display the week/month inputs.

Comment 45

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

This is only used when sdk > 11 now.

Comment 46

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 47

[Raphael Catolino (:raphc)]

(In reply to Wesley Johnston (:wesj) from comment #42)
> Comment on attachment 648723 [details] [diff] [review]
> custom date/time picker
> 
> Hopefully we don't need either of these then. You can do the input check in
> browser.js using Services.appinfo (which returns a nsIXULAppInfo object):
> 
> http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULAppInfo.idl
> 
> Or you could try having the prompt service return quickly (if the IME
> keyboard works....).
This is what I had done, before I saw your comment, if you think the overhead in calling all the message handling between browser/geckoAppShell/PromptService is too important, I can still do the check in browser.js. I had thought about using getBridge().getAPIVersion() though, would nsIXULAppInfo be more efficient?

So eventually these patches provide widgets for all date, time, month, etc, when SDK >= 11, and only for date and time otherwise. With the limitation that we can't use the date picker to input dates before 1900-01-01 and after 2100-12-31 (when SDK < 11).

Comment 48

[Chris Peterson [:cpeterson]]

Comment on attachment 648724 [details] [diff] [review]
date picker widget use

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

hi, Raphael. Your change looks pretty good, but I have some questions and suggestions below.

::: mobile/android/base/GeckoInputConnection.java
@@ +155,5 @@
>      @Override
>      public boolean finishComposingText() {
>          // finishComposingText() is sometimes called even when we are not composing text.
> +        // finishComposingText() is called by the input method manager from the main
> +        // thread so we have to make sure it's ran in the ui thread.

A small nit: s/it's ran in/it runs on/

@@ +1054,5 @@
> +        if (typeHint.equals("date") || typeHint.equals("datetime") ||
> +            typeHint.equals("week") || typeHint.equals("datetime-local") ||
> +            typeHint.equals("time") || typeHint.equals("month")) {
> +            return;
> +        }

Can you add a comment explaining why the type hints don't need to call the Runnable and `mIMETypeHint = typeHint?

@@ +1055,5 @@
> +            typeHint.equals("week") || typeHint.equals("datetime-local") ||
> +            typeHint.equals("time") || typeHint.equals("month")) {
> +            return;
> +        }
> +

I would prefer this sequence of if/equals be replaced with a static Collection<String> of strings constants. Like the following example code:

https://hg.mozilla.org/mozilla-central/file/cdc84c93ffe3/mobile/android/base/FormAssistPopup.java#l56

Then notifyIMEEnabled() can just do something like:

    if (sTypeHintTypes.contains(typeHint))
        return;

::: mobile/android/base/GeckoViewsFactory.java
@@ +5,5 @@
>  package org.mozilla.gecko;
>  
>  import org.mozilla.gecko.gfx.LayerView;
> +import org.mozilla.gecko.widget.NumberPickerHolder;
> +import org.mozilla.gecko.widget.NumberPickerButton;

Imports should be alphabetized with NumberPickerButton before NumberPickerHolder.

::: mobile/android/base/PromptService.java
@@ +5,5 @@
>  
>  package org.mozilla.gecko;
>  
>  import org.mozilla.gecko.gfx.LayerController;
> +import org.mozilla.gecko.widget.DatetimePicker;

1. Where is org.mozilla.gecko.widget.DatetimePicker? Is this class implemented in a different patch?

2. btw, I think we should call that class `DateTimePicker` (with an uppercase 'T'). "Date" and "Time" are separate words and "DateTime" is the capitalization style used by other Java libraries (like Joda Time).

@@ +39,1 @@
>  import android.widget.TextView;

Imports should be alphabetized with TimePicker after TextView.

@@ +40,5 @@
>  
>  import java.util.concurrent.SynchronousQueue;
>  import java.util.concurrent.TimeUnit;
> +import java.util.GregorianCalendar;
> +import java.text.SimpleDateFormat;

Imports should be alphabetized with java.text before java.util.

@@ +120,5 @@
> +            } else if (type.equals("date")) {
> +                DatetimePicker input = new DatetimePicker(GeckoApp.mAppContext);
> +                input.setSecondShown(false);
> +                input.setHourShown(false);
> +                input.setMinuteShown(false);

Do you need `input.setWeekShow(false)` like below? These set*Shown() calls are confusing because each block of code below calls different methods. Some are called with true, others false, so the default values are unclear.

This code might be cleaner if DateTimePicker declared an enum for the different formats used. The enum could be passed to the DateTimePicker constructor and it would set the set*Shown flags internally.

In fact, you might be able to hide all the SimpleDateFormat.parse() code inside DateTimePicker's constructor. This getView() method is getting pretty long.

@@ +187,5 @@
> +                    Log.d(LOGTAG,"Reusing previous value");
> +                    try {
> +                        GregorianCalendar calendar = new GregorianCalendar();
> +                        calendar.setTime(
> +                                new SimpleDateFormat("yyyy-MM").parse(value));

I think we should extract some of this copy/pasted code into a static helper method like:

    private static Date parseDateString(String dateFormat, String dateValue) {
        return new SimpleDateFormat(dateFormat).parse(dateValue);
    }

@@ +258,5 @@
> +                    calendar.setTimeInMillis(dp.getTimeInMillis());
> +                    return new SimpleDateFormat("yyyy-MM-dd kk:mm").format(calendar.getTime());
> +                } else if (type.equals("month")) {
> +                    return new SimpleDateFormat("yyyy-MM").format(calendar.getTime());
> +                }

I think we should extract some of this copy/pasted code into a static helper method like:

    private static String formatDateString(String dateFormat, Calendar calendar) {
        return new SimpleDateFormat(dateFormat).format(calendar.getTime());
    }

Comment 49

[Chris Peterson [:cpeterson]]

Comment on attachment 649341 [details] [diff] [review]
date picker widget use

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

I think (most of) my previous review's suggestions still apply, plus this one comment:

::: mobile/android/base/GeckoInputConnection.java
@@ +1066,5 @@
> +            Build.VERSION.SDK_INT > 10 &&
> +            (typeHint.equals("datetime") || typeHint.equals("month") ||
> +            typeHint.equals("week") || typeHint.equals("datetime-local"))) {
> +            return;
> +        }

You should add parentheses (and maybe some extra indentation) to disambiguate the ||s and && in this expression.

You can disregard my previous review's suggestion to replace these typeHint.equals() with a Collection<String> because you need to check the SDK version for some typeHints. <:)

Comment 50

[Raphael Catolino (:raphc)]

Attachment: custom date/time picker

Added the possibility to specify a initial value and the pickers to be displayed to the constructor.

Comment 51

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 52

[Raphael Catolino (:raphc)]

(In reply to Chris Peterson (:cpeterson) from comment #48)

> ::: mobile/android/base/PromptService.java
> @@ +5,5 @@
> >  
> >  package org.mozilla.gecko;
> >  
> >  import org.mozilla.gecko.gfx.LayerController;
> > +import org.mozilla.gecko.widget.DatetimePicker;
> 
> 1. Where is org.mozilla.gecko.widget.DatetimePicker? Is this class
> implemented in a different patch?

Yes, see attachment attachment 649654 [details] [diff] [review], in this bug.
> @@ +120,5 @@
> > +            } else if (type.equals("date")) {
> > +                DatetimePicker input = new DatetimePicker(GeckoApp.mAppContext);
> > +                input.setSecondShown(false);
> > +                input.setHourShown(false);
> > +                input.setMinuteShown(false);
> 
> Do you need `input.setWeekShow(false)` like below? These set*Shown() calls
> are confusing because each block of code below calls different methods. Some
> are called with true, others false, so the default values are unclear.
> 
> This code might be cleaner if DateTimePicker declared an enum for the
> different formats used. The enum could be passed to the DateTimePicker
> constructor and it would set the set*Shown flags internally.
> 
> In fact, you might be able to hide all the SimpleDateFormat.parse() code
> inside DateTimePicker's constructor. This getView() method is getting pretty
> long.
The DateTimePicker constructor can now take an initial value and an enum representing the pickers to be displayed. This has indeed allowed to shrink the getView() size.

Comment 53

[Chris Peterson [:cpeterson]]

Comment on attachment 649654 [details] [diff] [review]
date picker widget use

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

Looks good to me! r+ with a couple small suggestions.

::: mobile/android/base/GeckoInputConnection.java
@@ +1061,5 @@
>      public void notifyIMEEnabled(final int state, final String typeHint, final String actionHint) {
> +        // For some input type we will use a  widget tu display the ui, for those we must not display
> +        // the ime.
> +        // We can display a widget for date and time types, and since sdk 11, for datetime/month/week
> +        // as well.

Two small nits:
1. Can you change "a widget tu" to "a widget to"?
2. And fix the formatting of the comment so it fits on three lines?

::: mobile/android/base/PromptService.java
@@ +43,4 @@
>  import java.util.concurrent.SynchronousQueue;
>  import java.util.concurrent.TimeUnit;
> +import java.util.GregorianCalendar;
> +import java.util.Calendar;

imports should be alphabetical with java.util.Calendar before java.util.GregorianCalendar

@@ +219,5 @@
> +                TimePicker tp = (TimePicker)view;
> +                GregorianCalendar calendar =
> +                    new GregorianCalendar(0,0,0,tp.getCurrentHour(),tp.getCurrentMinute());
> +                return formatDateString("kk:mm",calendar);
> +            } else if (android.os.Build.VERSION.SDK_INT < 11 && type.equals("date")) {

Can you add a short comment explaining why type.equals("date") uses DateTimePicker instead of DatePicker when SDK_INT >= 11?

Comment 54

[Chris Peterson [:cpeterson]]

When this patch lands, it will fix my bug 780239 related to finishComposingText() running on the wrong thread. :)

Comment 55

[Raphael Catolino (:raphc)]

Attachment: date picker widget use

Comment 56

[Chris Peterson [:cpeterson]]

Raphael, are you still waiting for reviews from Wes?

Comment 57

[Mounir Lamouri (:mounir)]

(In reply to Chris Peterson (:cpeterson) from comment #56)
> Raphael, are you still waiting for reviews from Wes?

Yes, AFAIK, he was still waiting for reviews from Wes. (he is currently moving or just moved so he might not be very responsive)

Comment 58

[Wesley Johnston (:wesj)]

Comment on attachment 649985 [details] [diff] [review]
date picker widget use

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

Awesome. Sorry for the delay. I thought I was still waiting for these patches. This looks and works great! I think we can even come back and support for datetime and datetime-local on Gingerbread devices eventually by just adding the second input to the prompt call (only on those android versions). Can you file a follow up for that too?

::: mobile/android/base/GeckoInputConnection.java
@@ +156,5 @@
>      public boolean finishComposingText() {
>          // finishComposingText() is sometimes called even when we are not composing text.
> +        // finishComposingText() is called by the input method manager from the main
> +        // thread so we have to make sure it's run in the ui thread.
> +        postToUiThread(new Runnable() {

The patch giving you this was backed out. But I think we can bring it back in. bug 769520.

::: mobile/android/base/PromptService.java
@@ +109,5 @@
>                  hint  = aJSONInput.getString("hint");
>              } catch(Exception ex) { }
> +            try {
> +                value  = aJSONInput.getString("value");
> +            } catch(Exception ex) { }

Hmm... we actually use value in a few other places as well (although its not always a string). I think we should probably update them. Can you file a separate bug?

@@ +227,5 @@
> +                GregorianCalendar calendar =
> +                    new GregorianCalendar(dp.getYear(),dp.getMonth(),dp.getDayOfMonth());
> +                return formatDateString("yyyy-MM-dd",calendar);
> +            } else {
> +                DateTimePicker dp = (DateTimePicker)view;

I'm a little worried we'll crash with this if we've got some strange view in here, but if we have, something else has gone wrong too.

@@ +327,5 @@
> +            } catch(UnsupportedOperationException ex) {
> +              // We cannot display these input widgets with this sdk version,
> +              // do not display any dialog and finish the prompt now.
> +              finishDialog("{\"button\": -1}");
> +              return;

4 space indent. That happens in a few other places too.

::: mobile/android/chrome/content/InputWidgetHelper.js
@@ +30,5 @@
> +        { label: Strings.browser.GetStringFromName("inputWidgetHelper.clear") },
> +        { label: Strings.browser.GetStringFromName("inputWidgetHelper.cancel") }
> +      ],
> +      inputs: [
> +        { type: aElement.getAttribute('type'), value: aElement.value }

Since you use it a lot, probably best to just store the type in a var up here and reuse it.

@@ +50,5 @@
> +      }
> +    } else if (data.button == 0) {
> +      // Commit the new value.
> +      if (aElement.value != data[aElement.getAttribute('type')]) {
> +        aElement.value = data[aElement.getAttribute('type')];

i.e. See the type bit above.

Comment 59

[Chris Peterson [:cpeterson]]

(In reply to Wesley Johnston (:wesj) from comment #58)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +156,5 @@
> >      public boolean finishComposingText() {
> >          // finishComposingText() is sometimes called even when we are not composing text.
> > +        // finishComposingText() is called by the input method manager from the main
> > +        // thread so we have to make sure it's run in the ui thread.
> > +        postToUiThread(new Runnable() {
> 
> The patch giving you this was backed out. But I think we can bring it back
> in. bug 769520.

I plan to (eventually) land a revised patch for bug 769520. In the meantime, please feel free to resurrect the postToUiThread() method if it makes your patch easier to merge.

Comment 60

[Mounir Lamouri (:mounir)]

Raphael, do you think you can find time to update these patches (apply review comments) and attach here a version ready to be landed?

Comment 61

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5d6fe4410e
https://hg.mozilla.org/integration/mozilla-inbound/rev/42cec3ee2c80
https://hg.mozilla.org/integration/mozilla-inbound/rev/d38425445f83

Comment 62

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/8f5d6fe4410e
https://hg.mozilla.org/mozilla-central/rev/42cec3ee2c80
https://hg.mozilla.org/mozilla-central/rev/d38425445f83

Comment 63

[Cristian Nicolae (:xti)]

This issue is fixed on the latest Nightly. Tapping on the input date filed on http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_input_type_date will trigger the date/time picker.
Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-23)
Device: Galaxy Note
OS: Android 4.0.4