Closed Bug 1357191 Opened 3 years ago Closed 2 years ago

Use Gecko controls for Fennec date/time fields

Categories

(Firefox for Android :: General, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files)

Currently, Fennec displays dates and times as unformatted raw values (e.g. "2017-04-17" or "16:26"). We want to use Gecko-provided controls so that they display formatted values (e.g. "04-17-2017" or "4:26 PM" in the US).

Because Fennec provides its own date/time pickers, we don't need to rely on the actual Gecko pickers, so I think the current date/time implementation is stable enough to be used in Fennec.
Right now, date/time fields in Fennec appear as regular text fields,
which display the date/time values without formatting. This patch makes
the fields use the Gecko controls, which do support formatting. This
only changes the appearance of the fields; we still display the native
date/time pickers when the fields are tapped on. The reset button is
hidden in the controls because the Fennec date/time picker provides
a separate "clear" button.
Attachment #8858915 - Flags: review?(s.kaspari)
Attachment #8858915 - Flags: review?(jjong)
Comment on attachment 8858915 [details] [diff] [review]
Use Gecko controls for Fennec date/time fields (v1)

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

For input type=date, our custom calendar picker pops up on click event (we also have a picker for type=time, but it's under pref "dom.forms.datetime.timepicker"). Will this conflict with Android's native calendar picker? Maybe you should prevent default on click.
Also, we have automation tests on this that are currently disabled on Fennec, they should be enabled with this patch, see dom/html/test/forms/mochitest.ini.

::: mobile/android/themes/core/content.css
@@ +340,5 @@
>  
> +button:-moz-native-anonymous.datetime-reset-button {
> +  display: none;
> +}
> +

So, datetime-reset-button is actually part of the xbl binding, I am not sure whether `-moz-native-anonymous` works with it. Have you tested to see if this works?
Attachment #8858915 - Flags: review?(jjong)
Another thing is, `updateDateTimeInputBox()` should be called when user selects a date on the picker, so that the value in the input box can be updated, I don't see that part in the patch.

[1] http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/webidl/HTMLInputElement.webidl#251
For this to work we would need to bind Android picker to the Gecko input element. It had to replicate almost all the feature the XUL/XHTML-based picker offered. This also poses fairly big amount of risk for Fennec, so I hope we can get QE to validate the feature before shipping or perf-on.

I am however more interested in understand the intention here, since bug 1352238 want to render form control with Android widgets, while this bug would like to continue the current (even hybrid) approach. Should we think this through and try to unify them? I am fine with either as long as it's an intentional decision.
See Also: → 1352238
Comment on attachment 8858915 [details] [diff] [review]
Use Gecko controls for Fennec date/time fields (v1)

Clearing review: Seems like there are some concerns? If there's a follow-up patch feel free to flag someone from the new Taipei core team. I'm not an expert on this part of the code anyways. :)
Attachment #8858915 - Flags: review?(s.kaspari)
(In reply to Jessica Jong [:jessica] from comment #2)
> Comment on attachment 8858915 [details] [diff] [review]
> Use Gecko controls for Fennec date/time fields (v1)
> 
> Review of attachment 8858915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For input type=date, our custom calendar picker pops up on click event (we
> also have a picker for type=time, but it's under pref
> "dom.forms.datetime.timepicker"). Will this conflict with Android's native
> calendar picker? Maybe you should prevent default on click.

Fennec does not activate DateTimePickerHelper.jsm, which is responsible for opening the XBL popup. So clicking on the field only opens the Fennec native picker.

> Also, we have automation tests on this that are currently disabled on
> Fennec, they should be enabled with this patch, see
> dom/html/test/forms/mochitest.ini.

Okay, I'll look into these tests.

> > +button:-moz-native-anonymous.datetime-reset-button {
> > +  display: none;
> > +}
> > +
> 
> So, datetime-reset-button is actually part of the xbl binding, I am not sure
> whether `-moz-native-anonymous` works with it. Have you tested to see if
> this works?

Yes, `-moz-native-anonymous` works in this case.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> For this to work we would need to bind Android picker to the Gecko input
> element. It had to replicate almost all the feature the XUL/XHTML-based
> picker offered. This also poses fairly big amount of risk for Fennec, so I
> hope we can get QE to validate the feature before shipping or perf-on.

We already have native Android date/time pickers in Fennec. This bug is about changing the appearance of date/time fields in Fennec to use the desktop date/time widget. We will continue to use native Android date/time pickers in Fennec after this change.

> I am however more interested in understand the intention here, since bug
> 1352238 want to render form control with Android widgets, while this bug
> would like to continue the current (even hybrid) approach. Should we think
> this through and try to unify them? I am fine with either as long as it's an
> intentional decision.

I don't think this bug conflicts with bug 1352238. The purpose of bug 1352238 is to provide web compatibility for the appearance:none CSS property, and the changes made here should not impact appearance:none compatibility.
To give more context, the attached screenshot shows the only change this bug will make. The top window shows the date/time fields in current Fennec versions. The bottom window shows the intended change to use Gecko-provided controls.
Thank you Jim for the clarification. I have some more questions.

As far as I know, ICU is not enabled on Fennec, but localization for input date/times (like am/pm strings) relies on ICU. So, some l10n will not work properly.

On desktop, the input box relies on some internal APIs and message passing to communicate with the picker. On Fennec, how does the input box communicates with the native Android date/time pickers? Do you just set the input element's value so that input box value is updated automatically? What about when input box has a preset value and needs to pass it to the picker, so that the picker can start on that date/time?
Good point! ICU is enabled in Nightly but not Release, so maybe this should depend on ICU shipping everywhere.

We only access the input value before the picker shows, and after the picker is dismissed. Before showing the picker, we pre-populate the picker with the input value. After the picker is dismissed, we set the input value to the picker value. The input value is _not_ updated while the picker is active. This all happens in Fennec-specific code (InputWidgetHelper.js), so it shouldn't depend on the internal date/time widget APIs and messages.k
Depends on: 1344625
BTW, I've been told that ICU being not in Release is because of the increase of APK size.
We use ICU on Android as default from 56. Our plan that Gecko will require ICU from 58 even if on Android (will remove non-ICU config on 58)
Comment on attachment 8858915 [details] [diff] [review]
Use Gecko controls for Fennec date/time fields (v1)

Now that we have ICU support, I think it's time to revisit using Gecko controls in Fennec.
Attachment #8858915 - Flags: review?(jjong)
Attachment #8858915 - Flags: review?(cnevinchen)
Hi Jim
Do I need to do a full build to apply this patch?
Sorry I don't know the idea behind this patch.
Can you teach me how to test it?
Flags: needinfo?(nchen)
(In reply to Nevin Chen [:nechen] from comment #13)
> Hi Jim
> Do I need to do a full build to apply this patch?
> Sorry I don't know the idea behind this patch.
> Can you teach me how to test it?

Yeah you need a full build. Here's an existing try run with builds, https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdc44781987e409a94d9fa20115062f722e9c78c
Flags: needinfo?(nchen)
Fix some test failures on mobile for the new date/time controls,

* Because mobile does not use the built-in reset button, tests
  involving the reset button are skipped.

* The mobile-only date/time tests in test_input_typing_sanitization.html
  do not apply to the new date/time controls, so they are removed to make
  mobile match desktop.

* test_input_sanitization.html now takes longer to run on Android and
  may time out, so the timeout limit is increased.

* time-content-left-aligned.html may fail because of different drawing
  behavior on Android, so the test is made fuzzy on Android.
Attachment #8915843 - Flags: review?(jjong)
Comment on attachment 8858915 [details] [diff] [review]
Use Gecko controls for Fennec date/time fields (v1)

I didn't see any trouble with the apk I downloaded from the try server.
r+ for the code. But if there's a guide about how to trace this code is event better. To me on Android java side, it just changes css and pref so it should be fine.
Attachment #8858915 - Flags: review?(cnevinchen) → review+
Hi Jim, the patches look good to me, but I have one more question. In Desktop, the input box listens to click events on the input element [1] and dispatches 'MozOpenDateTimePicker' chrome event [2] in order to open the picker. What will happen to this on Fennec?

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/toolkit/content/widgets/datetimebox.xml#1811
[2] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/html/HTMLInputElement.cpp#2422-2425
Flags: needinfo?(nchen)
Thanks Jessica. We don't handle `MozOpenDateTimePicker` on mobile, but we have a separate click event listener inside `InputWidgetHelper` [1] that is responsible for displaying the native datetime picker on Android (`InputWidgetHelper` also handles 'week', 'month', 'datetime-local' inputs).

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/mobile/android/modules/InputWidgetHelper.jsm#32
Flags: needinfo?(nchen)
Comment on attachment 8915843 [details] [diff] [review]
2. Fix form tests for new mobile date/time controls (v1)

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

Thanks for enabling/fixing the tests.
Attachment #8915843 - Flags: review?(jjong) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #18)
> Thanks Jessica. We don't handle `MozOpenDateTimePicker` on mobile, but we
> have a separate click event listener inside `InputWidgetHelper` [1] that is
> responsible for displaying the native datetime picker on Android
> (`InputWidgetHelper` also handles 'week', 'month', 'datetime-local' inputs).
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/mobile/android/modules/
> InputWidgetHelper.jsm#32

Thanks for the clarification. I guess it's because DateTimePickerHelper is not initialized at all on mobile, right?
Comment on attachment 8858915 [details] [diff] [review]
Use Gecko controls for Fennec date/time fields (v1)

Looks good to me.
Attachment #8858915 - Flags: review?(jjong) → review+
(In reply to Jessica Jong [:jessica] from comment #20)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #18)
> > Thanks Jessica. We don't handle `MozOpenDateTimePicker` on mobile, but we
> > have a separate click event listener inside `InputWidgetHelper` [1] that is
> > responsible for displaying the native datetime picker on Android
> > (`InputWidgetHelper` also handles 'week', 'month', 'datetime-local' inputs).
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/mobile/android/modules/
> > InputWidgetHelper.jsm#32
> 
> Thanks for the clarification. I guess it's because DateTimePickerHelper is
> not initialized at all on mobile, right?

Right
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/562e7fc9a839
1. Use Gecko controls for Fennec date/time fields; r=jessica r=nechen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ba568874b0
2. Fix form tests for new mobile date/time controls; r=jessica
Backed out for frequently failing mochitest dom/html/test/forms/test_input_sanitization.html on Android 4.3 API16+ debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3952ef5411c6b24d181bc6eaf0b35a2cb280fd

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c6ba568874b01e46deb494b75002edeb9892f9dc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=exception&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135980961&repo=mozilla-inbound

[task 2017-10-10T17:36:01.374Z] 17:36:01     INFO -  278 INFO TEST-PASS | dom/html/test/forms/test_input_sanitization.html | type=datetime-local, frame, no editor (1484 subtests)
[task 2017-10-10T17:36:01.374Z] 17:36:01     INFO -  Buffered messages logged at 17:35:50
[task 2017-10-10T17:36:01.375Z] 17:36:01     INFO -  279 INFO TEST-PASS | dom/html/test/forms/test_input_sanitization.html | type=datetime-local, frame, editor (1484 subtests)
[task 2017-10-10T17:36:01.375Z] 17:36:01     INFO -  Buffered messages logged at 17:35:53
[task 2017-10-10T17:36:01.376Z] 17:36:01     INFO -  280 INFO TEST-PASS | dom/html/test/forms/test_input_sanitization.html | type=datetime-local, no frame, editor (1484 subtests)
[task 2017-10-10T17:36:01.376Z] 17:36:01     INFO -  Buffered messages finished
[task 2017-10-10T17:36:01.377Z] 17:36:01     INFO -  281 INFO TEST-UNEXPECTED-FAIL | dom/html/test/forms/test_input_sanitization.html | Test timed out.
[task 2017-10-10T17:36:01.377Z] 17:36:01     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-10-10T17:36:01.377Z] 17:36:01     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2017-10-10T17:36:38.731Z] 17:36:38     INFO -  282 ERROR [SimpleTest.finish()] this test already called finish!
Flags: needinfo?(nchen)
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27116489ad3d
1. Use Gecko controls for Fennec date/time fields; r=jessica r=nechen
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2f40bc1b13
2. Fix form tests for new mobile date/time controls; r=jessica
I decided to disable "test_input_sanitization.html" for Android debug, for the following reasons,

1) It was already the longest running test in that test chunk, and now it takes an unacceptably long amount of time to run

2) We still run the test for Android opt builds, so we still have test coverage

3) This is not the first time that we disable a test for being slow on Android debug (e.g. we already disabled "test_input_typing_sanitization.html")
Flags: needinfo?(nchen)
Can this test we split up, e.g. one test per input type so we keep the debug coverage?
You need to log in before you can comment on or make changes to this bug.