[DateTimeInput] Consider also @step when deciding to show or hide second/millisecond field

VERIFIED FIXED in Firefox 56

Status

()

enhancement
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Blocks 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Currently, we only show second/millisecond field when @value has second/millisecond part. But actually, we should also consider @step, for example, if step is "0.001", then user should be able to change the millisecond field.

Another improvement is to enable/disable fields based on step, for example, if step is 3600 (seconds), then the minute can be disabled, since changing the minute field would be invalid. This could be done in another bug.
Assignee: nobody → jjong
We should consider step and step base when deciding whether to show second and
millisecond field, since step and step base can affect the valid time intervals,
and the valid intervals may have second/millsecond part.
Attachment #8881973 - Flags: review?(bugs)
Attachment #8881970 - Flags: review?(bugs) → review+
Comment on attachment 8881973 [details] [diff] [review]
Part 2: Consider step when deciding whether to show second/millisecond field.

># HG changeset patch
># Parent  bb194483c8ecf2c7bd47dc57dd94182cba7efe50
>Bug 1374967 - Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug
>
>We should consider step and step base when deciding whether to show second and
>millisecond field, since step and step base can affect the valid time intervals,
>and the valid intervals may have second/millsecond part.
millisecond
>+      <method name="reBuildEditFieldsIfNeeded">
I think it is usually rebuild, not reBuild

>-          // Second and millsecond part is optional, rebuild edit fields if
>+          // Second and millsecond part are optional, rebuild edit fields if
millisecond

>           // needed.
>-          if ((this.isEmpty(second) == this.hasSecondField()) ||
>-              (this.isEmpty(millisecond) == this.hasMillisecField())) {
>-            this.log("Edit fields need to be rebuilt.")
>-            this.reBuildEditFields();
oh, reBuild is existing style here. ok fine.

>+      <method name="notifyMinMaxStepAttrChanged">
>+        <body>
>+        <![CDATA[
>+          // Second and millsecond part are optional, rebuild edit fields if
millisecond

Do we have tests when step is something weird, where it ends up increasing/decreasing by, say 61 seconds, or 1001 milliseconds?
Attachment #8881973 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8881973 [details] [diff] [review]
> Part 2: Consider step when deciding whether to show second/millisecond field.
> 
> ># HG changeset patch
> ># Parent  bb194483c8ecf2c7bd47dc57dd94182cba7efe50
> >Bug 1374967 - Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug
> >
> >We should consider step and step base when deciding whether to show second and
> >millisecond field, since step and step base can affect the valid time intervals,
> >and the valid intervals may have second/millsecond part.
> millisecond

will do.

> >+      <method name="reBuildEditFieldsIfNeeded">
> I think it is usually rebuild, not reBuild

yeah, "rebuild" looks better, will change them all.

> 
> >-          // Second and millsecond part is optional, rebuild edit fields if
> >+          // Second and millsecond part are optional, rebuild edit fields if
> millisecond

will do.

> 
> >           // needed.
> >-          if ((this.isEmpty(second) == this.hasSecondField()) ||
> >-              (this.isEmpty(millisecond) == this.hasMillisecField())) {
> >-            this.log("Edit fields need to be rebuilt.")
> >-            this.reBuildEditFields();
> oh, reBuild is existing style here. ok fine.
> 
> >+      <method name="notifyMinMaxStepAttrChanged">
> >+        <body>
> >+        <![CDATA[
> >+          // Second and millsecond part are optional, rebuild edit fields if
> millisecond

will do.

> 
> Do we have tests when step is something weird, where it ends up
> increasing/decreasing by, say 61 seconds, or 1001 milliseconds?

Will add some more tests for this.
Fix typos and added some more tests, carrying r+.
Attachment #8881973 - Attachment is obsolete: true
Attachment #8882268 - Flags: review+

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0299924b1
Part 1: Add helper functions to know if second/millisecond/day period exists. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/876fba84db76
Part 2: Consider step when deciding whether to show second/millisecond field. r=smaug
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0d0299924b1
https://hg.mozilla.org/mozilla-central/rev/876fba84db76
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Verified as fixed using the latest Nightly 56.0a1 (2017-07-09) on Ubuntu 16.04, Mac OS X 10.12 and Windows 10 x64.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.