Calendar date selection is hard to use, and looks bad

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

Trunk
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox49 verified, fennec+)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8715558 [details]
renfe_brokencalendar.png

We show a prompt to help with date selection for date input fields on websites. If the screen device is large enough, we show a CalendarView, otherwise we show scrollable date selectors.

E.g. Nexus 9 in portrait: show normal scrollable selector, in landscape: shows calendar.

The CalendarView is squashed into a smaller area than intended, and is quite hard to use (the dates are tiny), and looks bad (the Month/year is squished) - see attached screenshot.

We should either fix the layouting so that the calendar selector is big enough, or completely remove the CalendarView and use the same standard date-picker on all systems.

The logic that decides between CalendarView and the normal date-picker can be found at:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java#260

We create the DateTimePicker in:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/PromptInput.java#178

And this is all assembled into a dialog in:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#378
(Assignee)

Comment 1

3 years ago
Also, date selection with the CalendarView is currently completely broken (and crashes on some devices), see Bug 1239823
Depends on: 1239823
tracking-fennec: --- → ?
tracking-fennec: ? → +

Comment 2

3 years ago
I agree this is pretty bad...

How hard do you think it would be to fix this? Could we just fall back to using the day/month/year spinners in all cases, rather than the calendar?

Could this be a good mentor bug?
Flags: needinfo?(ahunt)
(Assignee)

Comment 3

3 years ago
Created attachment 8716092 [details]
Screenshot of calendar on 4.x device

(In reply to :Margaret Leibovic from comment #2)
> I agree this is pretty bad...
> 
> How hard do you think it would be to fix this? Could we just fall back to
> using the day/month/year spinners in all cases, rather than the calendar?
> 
> Could this be a good mentor bug?

Falling back to the spinners would be very easy to do - maybe it's worth doing that now, and creating a (mentor?) bug for bringing the calendar back again?

If we do keep/bring-back the calendar, I feel like it might be worth using it for _all_ tablets, instead of the weird decision code we have now. We have enough screen space on tablets to use the calendar (regardless of orientation), and I find it more natural to select dates from the calendar - whereas on a phone the spinner seems more natural. :antlam, what do you think?

(Currently we seem to use the calendar only on tablets in landscape mode, but I don't really understand the decision code.)


The CalendarView actually looks ok on older (4.x?) devices, it seems to be just the material design CalendarView that breaks like this. (Screenshot attached)
Flags: needinfo?(ahunt) → needinfo?(alam)
(In reply to Andrzej Hunt :ahunt from comment #3)
> Falling back to the spinners would be very easy to do - maybe it's worth
> doing that now, and creating a (mentor?) bug for bringing the calendar back
> again?

Sounds reasonable. For clarity, can I see a screenshot of these spinners?

> If we do keep/bring-back the calendar, I feel like it might be worth using
> it for _all_ tablets, instead of the weird decision code we have now. We
> have enough screen space on tablets to use the calendar (regardless of
> orientation)

I have no idea why we have "weird decision code" but all tablets should get the same UI, just like how they all get the same Fennec UI (tabs on top).

> and I find it more natural to select dates from the calendar -
> whereas on a phone the spinner seems more natural. :antlam, what do you
> think?

I don't actually see a problem with showing the calendar UI everywhere. It's what the Calendar app on Android uses isn't it? It works fine for me on the 6P, and N5..

I think Android spinners are more for inline forms. But even then, the new calendar UI works as an overlay just fine (for 6.0 devices)

> (Currently we seem to use the calendar only on tablets in landscape mode,
> but I don't really understand the decision code.)

Me neither, I say tablets get the same treatment regardless of screen orientation.
Flags: needinfo?(alam) → needinfo?(ahunt)
(Assignee)

Comment 5

3 years ago
We should probably get this done sooner rather than later, assigning to myself! I'll try to see if we can just show the calendar ~everywhere, in that case we might be able to bpass the PromptInput (which is causing the layout issues).
Assignee: nobody → ahunt
Flags: needinfo?(ahunt)
(Assignee)

Comment 6

3 years ago
Created attachment 8757411 [details]
MozReview Request: Bug 1245692 - Pre: sanitise PromptInput setup r?mcomella

Review commit: https://reviewboard.mozilla.org/r/55848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55848/
(Assignee)

Comment 7

3 years ago
Created attachment 8757412 [details]
MozReview Request: Bug 1245692 - Make CalendarView use full width of dialog r?sebastian

Squashing CalendarView makes it look bad and hard to use - by allowing
it to expand to the dialog width we get a usable CalendarView.

Note that this breaks on Android 4.x. On these devices CalendarView is
implemented using a ListView, which for some reason isn't given
any space during layouting - this results in the actual days in the month
being hidden (we do however see the weekday titles / month title).

Review commit: https://reviewboard.mozilla.org/r/55850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55850/
(Assignee)

Comment 8

3 years ago
Created attachment 8757413 [details]
MozReview Request: Bug 1245692 - Always use CalendarView for date selection r?sebastian

Our current decision criteria is arbitrary: there's no good reason not
to use a CalendarView here. Moreover our previous criteria would result
in small tablets showing different views depending on orientation (Nexus 7:
CalendarView in landscape, pickers in portrait mode).

Review commit: https://reviewboard.mozilla.org/r/55852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55852/
(Assignee)

Comment 9

3 years ago
This is mostly done, except that the layouting is broken on Android 4. The simplest solution might be to hardcode height on Android 4.X to ensure the CalendarView is usable, I'm going to try and read up on layouting a bit more to see if there's something we're doing wrong though (the layout is overly complex: we have multiple LinearLayouts just to build the prompt, and then more layouts within DateTimePicker, it's possible we're doing something there that's causing the ListView in 4.X's CalendarView implementation to trip up).
(Assignee)

Comment 10

3 years ago
Created attachment 8758905 [details]
MozReview Request: Bug 1245692 - Force height for CalendarView on pre-lollipop r?sebastian

By default CalendarView doesn't receive sufficient height on pre-lollipop
devices, in fact the entire dialog won't even appear unless we manually
assign height. This is slightly hacky, but necessary to ensure correct
layouting. (We previously assigned both height and width to the CalendarView,
however that results in all kinds of odd behaviour - this change is necessary
only to have acceptable behaviour on older devices.)

Review commit: https://reviewboard.mozilla.org/r/57002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57002/
Attachment #8757411 - Attachment description: MozReview Request: Bug 1245692 - Pre: sanitise PromptInput setup → MozReview Request: Bug 1245692 - Pre: sanitise PromptInput setup r?mcomella
Attachment #8757413 - Attachment description: MozReview Request: Bug 1245692 - Always use CalendarView for date selection → MozReview Request: Bug 1245692 - Always use CalendarView for date selection r?sebastian
Attachment #8757412 - Attachment description: MozReview Request: Bug 1245692 - Make CalendarView use full width of dialog → MozReview Request: Bug 1245692 - Make CalendarView use full width of dialog r?sebastian
Attachment #8758905 - Flags: review?(s.kaspari)
Attachment #8757411 - Flags: review?(michael.l.comella)
Attachment #8757413 - Flags: review?(s.kaspari)
Attachment #8757412 - Flags: review?(s.kaspari)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8757411 [details]
MozReview Request: Bug 1245692 - Pre: sanitise PromptInput setup r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55848/diff/1-2/
(Assignee)

Comment 12

3 years ago
Comment on attachment 8757413 [details]
MozReview Request: Bug 1245692 - Always use CalendarView for date selection r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55852/diff/1-2/
(Assignee)

Comment 13

3 years ago
Comment on attachment 8757412 [details]
MozReview Request: Bug 1245692 - Make CalendarView use full width of dialog r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55850/diff/1-2/
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 years ago
Created attachment 8758925 [details]
nexus4_jellybean.png

Here's a screenshot with this change on an N4
(Assignee)

Comment 15

3 years ago
Created attachment 8758926 [details]
nexus4_lollipop.png

And another N4 screenshot, this time with lollipop
Comment on attachment 8757413 [details]
MozReview Request: Bug 1245692 - Always use CalendarView for date selection r?sebastian

https://reviewboard.mozilla.org/r/55852/#review54040
Attachment #8757413 - Flags: review?(s.kaspari) → review+
Comment on attachment 8757412 [details]
MozReview Request: Bug 1245692 - Make CalendarView use full width of dialog r?sebastian

https://reviewboard.mozilla.org/r/55850/#review54042
Attachment #8757412 - Flags: review?(s.kaspari) → review+
Comment on attachment 8757411 [details]
MozReview Request: Bug 1245692 - Pre: sanitise PromptInput setup r?mcomella

https://reviewboard.mozilla.org/r/55848/#review54174

Spoke irl - Andrzej mentioned he did this in order to prevent unnecessary work if the `getView` methods were called more than once. However, the downside is we keep a reference to the constructed view, which references context, and provides us the opportunity to leak. This is normally fine if you're class is being used as a View (so it's obvious you have the context), but in this case, `* extends PromptInput` is not a view and is put into a mystery array and who knows how long that reference is kept around.

Andrzej said it'd be fine to remove this commit, hence r-.

::: mobile/android/base/java/org/mozilla/gecko/prompts/ColorPickerInput.java:32
(Diff revision 2)
> -    }
>  
> -    @Override
> -    public View getView(Context context) throws UnsupportedOperationException {
>          LayoutInflater inflater = LayoutInflater.from(context);
>          mView = inflater.inflate(R.layout.basic_color_picker_dialog, null);

You should generally inflate with a parent to ensure the theme/layout attributes carry over to the child: https://possiblemobile.com/2013/05/layout-inflation-as-intended/

That being said, the calling code is too wacky for me to identify if this is necessary here & this code was already existing so do what you will.
Attachment #8757411 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8758905 [details]
MozReview Request: Bug 1245692 - Force height for CalendarView on pre-lollipop r?sebastian

https://reviewboard.mozilla.org/r/57002/#review54344

::: mobile/android/base/java/org/mozilla/gecko/widget/DateTimePicker.java:345
(Diff revision 1)
>              });
>  
>              mPickers.addView(mCalendar);
> +
> +            if (Versions.preLollipop) {
> +                mCalendar.getLayoutParams().height = 400;

1) Why 400pixels and not a dp value?
2) You could add LayoutParams to the addView() call instead of adding it and then changing the default values afterwards.
3) Does seetting match_parent/wrap_content have any effects?
Attachment #8758905 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

3 years ago
Attachment #8757411 - Attachment is obsolete: true

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7789534d886d
https://hg.mozilla.org/mozilla-central/rev/bf1bb13cc624
https://hg.mozilla.org/mozilla-central/rev/6666bf0ccb50
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Verified as fixed in build 49.0a2 (2016-06-15);
Device: 
- Nexus 5 (Android 6.0.1);
- LG G4 (Android 5.1);
- Motorola Razr (Android 4.4.4).
status-firefox49: fixed → verified
You need to log in before you can comment on or make changes to this bug.