Implement valueAsNumber and valueAsDate for <input type=week>

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Priority: -- → P3
Assignee: nobody → jjong
Posted patch patch, v1. (obsolete) — Splinter Review
Attachment #8789730 - Flags: review?(bugs)
Comment on attachment 8789730 [details] [diff] [review]
patch, v1.


>+        double year = JS::YearFromTime(aValue.toDouble());
>+        double month = JS::MonthFromTime(aValue.toDouble());
>+        double day = JS::DayFromTime(aValue.toDouble());
>+        double yearDay = JS::DayWithinYear(aValue.toDouble(), year) + 1;
 + 1 could use some explanation.

Variable names could perhaps indicate which date format they are using. ISO or something else.

>+
>+        uint32_t weekDay = DayOfWeek(year, month + 1, day);
>+        weekDay = (weekDay == SUNDAY) ? 7 : weekDay;
Also this, that JS starts weeks from sunday, but indexing them as 0.


>+        // Target on Wednesday since ISO 8601 states that week 1 is the week
>+        // with the first Thursday of that year.
>+        uint32_t week = (yearDay - weekDay + 10) / 7;
Took a bit time to understand how this works :)
Hopefully we have tests for all the cases when 1. day is any week day.
And when last day is any week day. Both including leap years.


(The Shire calendar system Hobbits have is so much easier. Every year every day of month is the say week day, and there are just couple of extra days which don't belong to any week. And on leap year one extra such day.  :)
 https://en.wikipedia.org/wiki/Middle-earth_calendar#Shire_calendar  )




>+HTMLInputElement::DaysSinceEpochFromWeek(uint32_t aYear, uint32_t aWeek) const
>+{
>+  double days = JS::DayFromYear(aYear) + (aWeek - 1) * 7;
>+  uint32_t dayOne = DayOfWeek(aYear, 1, 1);
>+  dayOne = (dayOne == SUNDAY) ? 7 : dayOne;
Hmm, would it make sense to pass some enum param to DayOfWeek to indicate what should be the first day and what should be the index of it
It is error prone to check everywhere == SUNDAY ... then do something special

>+function checkWeekGet()
...

>+function checkWeekSet()
Could you possibly add tests in both these functions testing years start and ending with all different week days and also in leap years.
So I guess totally 28 tests. (I assume writing such tests could be automated)

>+function checkWeekGet()
...
>+function checkWeekSet()
I wouldn't mind also these having such tests.

>+      ok(caught, "valueAsNumber should have trhown");
thrown


So, I'd like to see some more tests (and could you perhaps check that other browsers behave the same way) and possibly a new enum param to DayOfWeek indicating what kind of value should be returned (in that case WeekDay enum might become useless and confusing)




>+++ b/js/public/Date.h
>@@ -144,11 +144,21 @@ YearFromTime(double time);
> JS_PUBLIC_API(double)
> MonthFromTime(double time);
> 
> // Takes an integer number of milliseconds since the epoch and returns the
> // day (1-based).  Can return NaN, and will do so if NaN is passed in.
> JS_PUBLIC_API(double)
> DayFromTime(double time);
> 
>+// Takes an integer year and returns the number of days from epoch to the given
>+// year.
>+JS_PUBLIC_API(double)
>+DayFromYear(double year);
>+
>+// Takes an integer number of milliseconds since the epoch and an integer year,
>+// returns the number of days in that year.
>+JS_PUBLIC_API(double)
>+DayWithinYear(double time, double year);
>+
> } // namespace JS
> 
> #endif /* js_Date_h */
>diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp
>--- a/js/src/jsdate.cpp
>+++ b/js/src/jsdate.cpp
>@@ -370,16 +370,28 @@ JS::MonthFromTime(double time)
> }
> 
> JS_PUBLIC_API(double)
> JS::DayFromTime(double time)
> {
>     return DateFromTime(time);
> }
> 
>+JS_PUBLIC_API(double)
>+JS::DayFromYear(double year)
>+{
>+    return ::DayFromYear(year);
>+}
>+
>+JS_PUBLIC_API(double)
>+JS::DayWithinYear(double time, double year)
>+{
>+    return ::DayWithinYear(time, year);
>+}
>+
This needs a review from a JS peer. Perhaps waldo could review the js/* parts
Attachment #8789730 - Flags: review?(jwalden+bmo)
Attachment #8789730 - Flags: review?(bugs)
Attachment #8789730 - Flags: review-
Comment on attachment 8789730 [details] [diff] [review]
patch, v1.

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

I mostly just looked at the JSAPI bits, plus a little around the edges to understand the need for them -- as apparently was desired.  Just an r+ on those bits, with the necessary changes made.

::: dom/html/HTMLInputElement.cpp
@@ +2161,5 @@
> +
> +        double year = JS::YearFromTime(aValue.toDouble());
> +        double month = JS::MonthFromTime(aValue.toDouble());
> +        double day = JS::DayFromTime(aValue.toDouble());
> +        double yearDay = JS::DayWithinYear(aValue.toDouble(), year) + 1;

Ugh, we need to give people better APIs for exploding a time into its constituent components.  Works for now, tho.

::: dom/html/HTMLInputElement.h
@@ +1505,5 @@
>    // Float value returned by GetStep() when the step attribute is set to 'any'.
>    static const Decimal kStepAny;
>  
> +  // Minimum year limited by HTML standard, year >= 1.
> +  static const double kMinimumYear;

I think you could consider using |static constexpr| for these so that you could define them inline, if you wanted.

::: dom/html/test/forms/test_valueasdate_attribute.html
@@ +527,5 @@
> +    [ "275760-W38", true ],
> +  ];
> +
> +  element.type = "week";
> +  for (data of validData) {

Do you mean for this |data| and in the other for-of loops -- and seemingly in all the rest of the code pre-existing in these two test files -- to be a global variable?  Should make them declarations -- any of var/let/const is equally good -- to reduce reader confusion.

::: dom/html/test/forms/test_valueasnumber_attribute.html
@@ +674,5 @@
> +      caught = true;
> +    }
> +
> +    if (data[2]) {
> +      ok(caught, "valueAsNumber should have trhown");

thrown

::: js/public/Date.h
@@ +149,5 @@
>  JS_PUBLIC_API(double)
>  DayFromTime(double time);
>  
> +// Takes an integer year and returns the number of days from epoch to the given
> +// year.

Add onto this, in a separate paragraph:

"""
NOTE: The calculation performed by this function is literally that given in the ECMAScript specification.  Nonfinite years, years containing fractional components, and years outside ECMAScript's date range are not handled with any particular intelligence.  Garbage in, garbage out.
"""

@@ +154,5 @@
> +JS_PUBLIC_API(double)
> +DayFromYear(double year);
> +
> +// Takes an integer number of milliseconds since the epoch and an integer year,
> +// returns the number of days in that year.

Add onto this (same paragraph)

"""
If |time| is nonfinite, returns NaN.  Otherwise |time| *must* correspond to a time within the valid year |year|.  This should usually be ensured by computing |year| as |JS::DayFromYear(time)|.
"""
Attachment #8789730 - Flags: review?(jwalden+bmo) → review+
Posted patch patch, v2. (obsolete) — Splinter Review
Thanks Olli and Waldo for the review.

Added an extra param @isoWeek for DayOfWeek(), but it's a boolean instead of an enum. And for variable names, I only added the 'iso' for week related variables, cause I think only week it's different from what we expect (starting on Monday and how it considers week 1).

Added tests for years starting and ending with all different week days and also in leap years.
Attachment #8789730 - Attachment is obsolete: true
Attachment #8793674 - Flags: review?(bugs)
Comment on attachment 8793674 [details] [diff] [review]
patch, v2.

>+    case NS_FORM_INPUT_WEEK:
>+      {
>+        aValue = aValue.floor();
>+
>+        // Based on ISO 8601 date.
>+        double year = JS::YearFromTime(aValue.toDouble());
>+        double month = JS::MonthFromTime(aValue.toDouble());
>+        double day = JS::DayFromTime(aValue.toDouble());
>+        // Adding 1 since day starts from 0.
>+        double dayInYear = JS::DayWithinYear(aValue.toDouble(), year) + 1;
>+
>+        uint32_t isoWeekday = DayOfWeek(year, month + 1, day, true);
Could use a comment why month + 1 here.

>+HTMLInputElement::DayOfWeek(uint32_t aYear, uint32_t aMonth, uint32_t aDay,
>+                            bool isoWeek) const
> {
>   // Tomohiko Sakamoto algorithm.
>   int monthTable[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
>   aYear -= aMonth < 3;
> 
>-  return (aYear + aYear / 4 - aYear / 100 + aYear / 400 +
>-          monthTable[aMonth - 1] + aDay) % 7;
>+  int day = (aYear + aYear / 4 - aYear / 100 + aYear / 400 +
Hmm, nit, int32_t, not int
Attachment #8793674 - Flags: review?(bugs) → review+
Posted patch patch, v3.Splinter Review
addressed review comment 5, carrying r+.
Attachment #8793674 - Attachment is obsolete: true
Attachment #8796055 - Flags: review+

Comment 8

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2614d75c2100
Implement valueAsNumber and valueAsDate for <input type=week>. r=smaug,Waldo
Keywords: checkin-needed

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2614d75c2100
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.