834302 - Separate the concept of "the maximum" and "the max content attribute" in nsHTMLInputElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

We should separate the concept of "the maximum" and "the max content attribute" (terms used in the HTML5 spec) in nsHTMLInputElement.

Spinning this work out into a separate bug from my work on bug 344618. This is relevant for type=range since that type has a default minimum and default maximum, unlike the currently implemented types.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 706309 [details] [diff] [review]
patch

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

Sorry for the delay :(

So, I think you are right: we need to make it clear that we follow the "mininum" or "maximum" concepts. However, I'm not sure we need two methods (GetMinAsDouble() vs GetMinimum()). I'm fine with having GetMinAsDouble() being renamed GetMinimum() if you prefer. We should definitely add a comment in the method's documentation saying that it is following the 'minimum'/'maximum' concept from the HTML spec. I do not think there is a need to rename all the variables to minimum/maximum though.

However, if it happens that we use GetMinAsDouble() or GetMaxAsDouble() somewhere to only get the attribute converted as a double, we might need two methods but I doubt this is the case (for the moment).

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #2)

[snip]

> I'm not sure we need two methods (GetMinAsDouble() vs GetMinimum()).

[snip]

> However, if it happens that we use GetMinAsDouble() or GetMaxAsDouble()
> somewhere to only get the attribute converted as a double, we might need two
> methods but I doubt this is the case (for the moment).

Actually it _is_ the case currently, and the patch results in there being two methods as a result. In the patch you can see I kept GetMinAsDouble since we need both GetMinAsDouble and GetMinimumAsDouble. (I removed GetMaxAsDouble since it's not needed.)

> We should definitely add a comment
> in the method's documentation saying that it is following the
> 'minimum'/'maximum' concept from the HTML spec.

That's already in the patch.

> I do not think there is a
> need to rename all the variables to minimum/maximum though.

In the case that we have GetMinAsDouble and GetMinimum[AsDouble] it would be misleading to leave the variables looking as if they contain the value returned by one, when they actually contain the value returned by the other.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 706309 [details] [diff] [review]
patch

So I'd suggest the patch as it currently is, perhaps with GetMinimumAsDouble renamed to GetMinimum.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 706392 [details] [diff] [review]
patch

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

r=me with the comments below applied. Feel free to ignore the nits though ;)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1448,5 @@
>    // Should only be used for <input type='number'/'date'> for the moment.
>    MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE);
>  
> +  // Once we add support for types that have a default minimum/maximum, take
> +  // account of the default minimum here.

nit: I like to open bugs and put a TODO with the bug number when I add that kind of comments. It's up to you given that you will anyway handle default values for type='range'.

@@ +1468,5 @@
>    // Should only be used for <input type='number'/'date'> for the moment.
>    MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE);
>  
> +  // Once we add support for types that have a default minimum/maximum, take
> +  // account of the default maximum here.

nit: ditto

@@ +1494,5 @@
> +  // Do NOT use GetMinimum here - the spec says to use "the min content
> +  // attribute", not "the minimum".
> +  nsAutoString minStr;
> +  if (GetAttr(kNameSpaceID_None, nsGkAtoms::min, minStr)) {
> +    if (ConvertStringToNumber(minStr, stepBase)) {

nit:
if (GetAttr() && ConvertStringToNumber()) {
 return stepBase;
}

@@ +1502,2 @@
>  
>    // If @min is not a double, we should use defaultValue.

nit: s/defaultValue/@value/

@@ +1502,5 @@
>  
>    // If @min is not a double, we should use defaultValue.
> +  nsAutoString valueStr;
> +  if (GetAttr(kNameSpaceID_None, nsGkAtoms::value, valueStr)) {
> +    if (ConvertStringToNumber(valueStr, stepBase)) {

nit:
if (GetAttr() && ConvertStringToNumber()) {
  return stepBase;
}

@@ +1508,4 @@
>      }
>    }
>  
> +  return stepBase;

I think doing:
return kDefaultStepBase;
would be better.

@@ +1575,5 @@
>    // When stepUp() is called and the value is below min, we should clamp on
>    // min unless stepUp() moves us higher than min.
>    if (GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) && aStep > 0 &&
> +      value <= minimum) {
> +    MOZ_ASSERT(!MOZ_DOUBLE_IS_NaN(minimum)); // min can't be NaN if we are here!

nit: update the comment.

@@ +1581,4 @@
>    // Same goes for stepDown() and max.
>    } else if (GetValidityState(VALIDITY_STATE_RANGE_OVERFLOW) && aStep < 0 &&
> +             value >= maximum) {
> +    MOZ_ASSERT(!MOZ_DOUBLE_IS_NaN(maximum)); // max can't be NaN if we are here!

nit: update the comment.

::: content/html/content/src/nsHTMLInputElement.h
@@ +287,5 @@
> +  /**
> +   * Returns the input's "minimum" (as defined by the HTML5 spec) as a double.
> +   * Note this takes account of any default minimum that the type may have.
> +   * Returns NaN if the min attribute isn't a valid floating point number and
> +   * the input's type does not have a default minimum.

Could you add a note saying that it has to be called only when min/max applies.

Also, keep this method as protected (ideally at the same place in the header). I guess you will need it public for another patch in your queue but I prefer the change to happen in the patch that needs it public instead of doing it now.

@@ +295,5 @@
> +  /**
> +   * Returns the input's "maximum" (as defined by the HTML5 spec) as a double.
> +   * Note this takes account of any default maximum that the type may have.
> +   * Returns NaN if the max attribute isn't a valid floating point number and
> +   * the input's type does not have a default maximum.

Same as above.

Comment 7

[Mounir Lamouri (:mounir)]

BTW, I'm finishing the min/max implementation for <input type='time'> so one of us would likely have to rebase. Given that I haven't get that patch reviewed, it might be me :)

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #7)
> BTW, I'm finishing the min/max implementation for <input type='time'> so one
> of us would likely have to rebase. Given that I haven't get that patch
> reviewed, it might be me :)

Sorry about that. If it's any consolation pulling this stuff out conflicts with my own patch queue. ;)

I addressed all your comments except the TODO nit, since I hope to have landed a patch next week to overwrite that comment.

Comment 9

[Jonathan Watt [:jwatt]]

Pushed in https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ace0f3ee89

Unfortunately the change in nsHTMLInputElement::GetStepBase from using ToDouble to using ConvertStringToNumber in order to get @value resulted in some slightly different rounding which caused some orange. I reverted that part back to using ToDouble:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcf7ce80fec

Comment 10

[Jonathan Watt [:jwatt]]

At some point we'll want to using ConvertStringToNumber to get HTML5 compliant parsing of numbers, so for the record these were the test failures:

test_stepup_stepdown.html | The value should be 1970-01-03 - got 1970-01-02, expected 1970-01-03
test_stepup_stepdown.html | The value should be 1970-01-07 - got 1970-01-06, expected 1970-01-07
test_stepup_stepdown.html | The value should be 1970-01-03 - got 1970-01-04, expected 1970-01-03
test_stepup_stepdown.html | The value should be 1970-01-07 - got 1970-01-08, expected 1970-01-07
test_stepup_stepdown.html | The value should be 1970-01-04 - got 1970-01-06, expected 1970-01-04

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4ace0f3ee89
https://hg.mozilla.org/mozilla-central/rev/1bcf7ce80fec

Comment 12

[Mounir Lamouri (:mounir)]

bug 837767 takes care of the issue seen in comment 9.