Comment on attachment 645695 [details] [diff] [review] patch Review of attachment 645695 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLInputElement.cpp @@ +154,5 @@ > // Default autocomplete value is "". > static const nsAttrValue::EnumTable* kInputDefaultAutocomplete = &kInputAutocompleteTable[0]; > > +static const double NS_STEP_SCALE_FACTOR_DATE = 86400000; > +static const double NS_STEP_SCALE_FACTOR_NUMBER = 1; I would prefer nsHTMLInputElement static const values like below. @@ +4151,5 @@ > + NS_ASSERTION(mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE, > + "We can't be there if type!=number or date!"); > + // NOTE: should be defaultStep * stepScaleFactor, > + // which is 1 for type=number and date. > + double step = 1 * mStepScaleFactor; You should not use mStepScaleFactor but GetStepScaleFactor(). To prevent calling the method everywhere you could only use it when calling |return|: |return step * GetStepScaleFactor();|. @@ +4638,5 @@ > + // valueLow and valueHigh because they might suffer from a step mismatch > + // as well. Instead we want the timestamps to correspond to a rounded day. > + // That is, we want a multiple of the step scale factor (1 day) > + // as well as of step. > + step = lcm(step, mStepScaleFactor); I wonder why you only need this here. I would tend to think we should use that for ::ApplyStep() too. Maybe whenever GetStep() is called? Could you check that? ::: content/html/content/src/nsHTMLInputElement.h @@ +584,5 @@ > + * @result the string with the double appended > + * @return whether the function succeded, it will fail if the current input's type > + * is not supported. > + */ > + bool AppendDoubleToString(double aValue, nsAString &aResultString) const; Could you call this |ConvertNumberToString()| so we match the specification naming? @@ +624,5 @@ > > + /** > + * Returns the least common multiple from two doube values. > + */ > + double lcm(double a, double b); I don't want to add that kind of stuff in nsHTMLInputElement. Could you instead add in another bug/patch, the following methods: NS_euclidGcd() and NS_lcm() You could add those to: xpcom/ds/nsMathUtils.h And update layout/style/nsStyleAnimation.cpp to use them. @@ +721,5 @@ > > // Default step base value when a type do not have specific one. > static const double kDefaultStepBase; > + // Step scale factor for the current type. > + double mStepScaleFactor; I would prefer a getter: GetStepScaleFactor() so that way we don't have to update the variable each time there is a type change. It's less error-prone and isn't performance critical so we can take the small overhead. ::: content/html/content/test/forms/test_step_attribute.html @@ +152,5 @@ > + input.value = '2008-03-01'; > + checkValidity(input, true, apply); > + > + input.value = '2008-02-29'; > + checkValidity(input, false, apply, { low: "2008-02-28", high: "2008-03-01" }); Could you add the exact same test with input.min = "2008-02-27" and .value = "2008-02-28" and check that low = "-25" and high = "-29". @@ +170,5 @@ > + checkValidity(input, true, apply); > + > + input.step = '0.9'; > + input.value = '1951-01-02'; > + checkValidity(input, false, apply, { low: "1951-01-01", high: "1951-01-10" }); It makes sense to do that and the specs do the same but we need to do something because Chrome and Opera don't do that. Maybe we should just send an email to the list or file a bug to their bug trackers. @@ +206,3 @@ > } else { > // When step=0, the allowed step is 1. > + input.min = ""; Why is that needed? ::: content/html/content/test/forms/test_stepup_stepdown.html @@ +231,5 @@ > + [ '1970-01-05', '2', '1970-01-02', '1970-01-05', null, '1970-01-04', false ], > + [ '1970-01-01', '5', '1970-01-02', '1970-01-09', 10, '1970-01-01', false ], > + [ '1970-01-07', '5', '1969-12-27', '1970-01-06', 2, '1970-01-01', false ], > + [ '1970-03-08', '3', '1970-02-01', '1970-02-07', 15, '1970-02-01', false ], > + [ '1970-01-10', '3', '1970-01-01', '1970-01-06', 2, '1970-01-04', false ], Do you have a test where you are above the max value and clamp to the max value while calling stepDown() even if stepDown() should put you above the max value? @@ +412,5 @@ > + [ '1970-01-02', '2', '1970-01-01', '1970-01-04', null, '1970-01-03', false ], > + [ '1970-01-01', '5', '1970-01-02', '1970-01-09', 10, '1970-01-07', false ], > + [ '1969-12-28', '5', '1969-12-29', '1970-01-06', 3, '1970-01-03', false ], > + [ '1970-01-01', '3', '1970-02-01', '1970-02-07', 15, '1970-02-07', false ], > + [ '1970-01-01', '3', '1970-01-01', '1970-01-06', 2, '1970-01-04', false ], Same here but for min and below min.

(In reply to Mounir Lamouri (:mounir) from comment #2) > Comment on attachment 645695 [details] [diff] [review] > patch > > @@ +4638,5 @@ > > + // valueLow and valueHigh because they might suffer from a step mismatch > > + // as well. Instead we want the timestamps to correspond to a rounded day. > > + // That is, we want a multiple of the step scale factor (1 day) > > + // as well as of step. > > + step = lcm(step, mStepScaleFactor); > > I wonder why you only need this here. I would tend to think we should use > that for ::ApplyStep() too. Maybe whenever GetStep() is called? Could you > check that? According to the specs, http://www.whatwg.org/specs/web-apps/current-work/#dom-input-stepup, ApplyStep just has to do step*stepScaleFactor +- value, then convert this number to a string according to the current input type. Which means that if stepUp/Down is going to return a timestamp that does not correspond to a date whose time is on midnight UTC, the value will just be rounded down to the date on midnight UTC. And this value might suffer from a step mismatch. > ::: content/html/content/test/forms/test_step_attribute.html > @@ +152,5 @@ > > + input.value = '2008-03-01'; > > + checkValidity(input, true, apply); > > + > > + input.value = '2008-02-29'; > > + checkValidity(input, false, apply, { low: "2008-02-28", high: "2008-03-01" }); > > Could you add the exact same test with input.min = "2008-02-27" and .value = > "2008-02-28" and check that low = "-25" and high = "-29". Hum if min="2008-02-27" low shouldn't be "2008-02-25", it should be -27, and high -29. I added this test and an other one for non leap years. > @@ +170,5 @@ > > + checkValidity(input, true, apply); > > + > > + input.step = '0.9'; > > + input.value = '1951-01-02'; > > + checkValidity(input, false, apply, { low: "1951-01-01", high: "1951-01-10" }); > > It makes sense to do that and the specs do the same but we need to do > something because Chrome and Opera don't do that. Maybe we should just send > an email to the list or file a bug to their bug trackers. From what I saw, chromium and opera are rounding all step values to an integer. This behavior can make sense, I don't really see the use cases in having a step precision that goes beyond 1 day, for an input type that can only represents dates and not time. But this is a broader issue than just the validation message, for example it will also change the stepUp/down behavior : if step = 0.5, stepUp(2) will add two days, because step will be changed to 1 (chromium+opera behavior), while now, stepUp(2) only adds 1 day (spec+this patch behavior). > @@ +206,3 @@ > > } else { > @@ +206,3 @@ > > // When step=0, the allowed step is 1. > > + input.min = ""; > > Why is that needed? > Because the min value stays the same when we change the input type, it's reused for the next test (number), this was not the ideal place to do that though, I moved it to the start of the loop. > ::: content/html/content/test/forms/test_stepup_stepdown.html > @@ +231,5 @@ > > + [ '1970-01-05', '2', '1970-01-02', '1970-01-05', null, '1970-01-04', false ], > > + [ '1970-01-01', '5', '1970-01-02', '1970-01-09', 10, '1970-01-01', false ], > > + [ '1970-01-07', '5', '1969-12-27', '1970-01-06', 2, '1970-01-01', false ], > > + [ '1970-03-08', '3', '1970-02-01', '1970-02-07', 15, '1970-02-01', false ], > > + [ '1970-01-10', '3', '1970-01-01', '1970-01-06', 2, '1970-01-04', false ], > > Do you have a test where you are above the max value and clamp to the max > value while calling stepDown() even if stepDown() should put you above the > max value? Yes but it was in the max test part, I moved it to clamping. > @@ +412,5 @@ > > + [ '1970-01-02', '2', '1970-01-01', '1970-01-04', null, '1970-01-03', false ], > > + [ '1970-01-01', '5', '1970-01-02', '1970-01-09', 10, '1970-01-07', false ], > > + [ '1969-12-28', '5', '1969-12-29', '1970-01-06', 3, '1970-01-03', false ], > > + [ '1970-01-01', '3', '1970-02-01', '1970-02-07', 15, '1970-02-07', false ], > > + [ '1970-01-01', '3', '1970-01-01', '1970-01-06', 2, '1970-01-04', false ], > > Same here but for min and below min. Same thing. Moved it too.

Comment on attachment 650588 [details] [diff] [review] patch Review of attachment 650588 [details] [diff] [review]: ----------------------------------------------------------------- We spoke with Raphael : we want stepUp() and stepDown() to go to the new valid value. We already do that for <input type='number'>.

This patch implement the following behavior for stepUp(n)/stepDown(n): If applying value += step*n returns a value that is not a date, (i.e. a timestamp not corresponding to a date whose time is set on midnight UTC), then we return the next valid date that is not in step mismatch. For example with <input type='date' min='2012-08-17' step='1.5' value='2012-08-18'> stepUp() will return "2012-08-20". I filed Bug 783607 for the precision issue.

Comment on attachment 652843 [details] [diff] [review] patch Review of attachment 652843 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1140,5 @@ > +bool > +nsHTMLInputElement::ConvertNumberToString(double aValue, > + nsAString& aResultString) const > +{ > + NS_ASSERTION(mType == NS_FORM_INPUT_DATE || mType == NS_FORM_INPUT_NUMBER, nit: MOZ_ASSERT @@ +1168,5 @@ > + JS_CallFunctionName(ctx, date, "getUTCFullYear", 0, NULL, &year); > + JS_CallFunctionName(ctx, date, "getUTCMonth", 0, NULL, &month); > + JS_CallFunctionName(ctx, date, "getUTCDate", 0, NULL, &day); > + aResultString.AppendPrintf("%04.0f-%02.0f-%02.0f", year.toNumber(), > + month.toNumber() + 1, day.toNumber()); I think you erased some changes I asked in a previous patch there. @@ +4173,5 @@ > double > nsHTMLInputElement::GetStep() const > { > + NS_ASSERTION(mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE, > + "We can't be there if type!=number or date!"); nit: MOZ_ASSERT. @@ +4395,5 @@ > + // to precision loss, since in most use cases this value should be > + // an integer (millisecond precision), we can get rid of the precision > + // loss by rounding step. This will however lead to erroneous results > + // when step was intented to have a precision superior to a millisecond. > + step = NS_round(step); I'm not sure I understand what that is for... @@ +5128,5 @@ > + case NS_FORM_INPUT_DATE: > + return NS_STEP_SCALE_FACTOR_DATE; > + break; > + default: > + return NS_STEP_SCALE_FACTOR_NUMBER; nit: no need for 'break' after return. You should have an explicit case for number and simply MOZ_NOT_REACHED in the default case. ::: content/html/content/src/nsHTMLInputElement.h @@ +580,5 @@ > > /** > + * Append a double to a string in a type specific way, ie convert a timestamp to a > + * date string if type = date or append the number string representing the value if > + * type = number. I would prefer this method to explicitly convert, not append. @@ +637,5 @@ > */ > double GetMaxAsDouble() const; > > + /** > + * Get the step scale value for the current type. nit: comment is mis-aligned and it would be nice to link to the spec. @@ +712,5 @@ > nsString mFocusedValue; > > + // Step scale factor values, for input types that have one. > + static const double NS_STEP_SCALE_FACTOR_DATE; > + static const double NS_STEP_SCALE_FACTOR_NUMBER; nit: kStepScaleFactor{Date,Number}

I will apply the changes myself and land this with the other patches to m-c.

See bug 866440 for documentation.