836314 - Implement the DOM pieces of <input type=range>

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Spinning this out from bug 344618 since this can be landed as soon as it has reviews.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Err, I completely messed up rejuggling my mq. Here's the patch that actually contains the changes for this bug.

Comment 3

[Mounir Lamouri (:mounir)]

Sorry if the review is taking so long. The DOM work week made things a bit crazy.
The ETA for this is tomorrow.

Comment 4

[Mounir Lamouri (:mounir)]

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

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

Could you add a line in:
embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:3275
regarding NS_FORM_INPUT_RANGE?

Also, I'm not sure if I agree with the :SanitizeValue() change. I wonder if it wouldn't be simpler by changing the value if the element happens to be invalid. In other words, when you check if the element has an overflow, if that's the case, you could set the value to maximum. Same for underflow and minimum and step mismatch. I think that might require some re-ordering in how we check validity though.

I tried to ping you on IRC to discuss that bet you were offline. Feel free to ping me later.

::: content/html/content/public/nsIFormControl.h
@@ +242,5 @@
>           // TODO: those are temporary until bug 773205 is fixed.
>           aType == NS_FORM_INPUT_DATE ||
>           aType == NS_FORM_INPUT_TIME ||
> +         // TODO: layout/UI parts for range
> +         aType == NS_FORM_INPUT_RANGE ||

Don't add that. There is no plan to ship a <input type='range'> with only a text field, right?

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +190,5 @@
>    case NS_FORM_TEXTAREA :
>    case NS_FORM_FIELDSET :
>    case NS_FORM_OBJECT :
>    case NS_FORM_OUTPUT :
> +  case NS_FORM_INPUT_RANGE :

Could you keep this with the other NS_FORM_INPUT_* ?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +135,5 @@
>    { "tel", NS_FORM_INPUT_TEL },
>    { "text", NS_FORM_INPUT_TEXT },
>    { "time", NS_FORM_INPUT_TIME },
>    { "url", NS_FORM_INPUT_URL },
> +  { "range", NS_FORM_INPUT_RANGE },

Please, keep the list alphabetically ordered.

@@ +1022,5 @@
>    nsresult rv = GetValueInternal(aValue);
>  
>    // Don't return non-sanitized value for number inputs.
> +  if (mType == NS_FORM_INPUT_NUMBER ||
> +      mType == NS_FORM_INPUT_RANGE) {

You do not need that. This line is a workaround for <input type='number'>. Actually, it's more clear with the current tip of the tree that has it for experimental mobile types.

@@ +1464,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 ||
> +             mType == NS_FORM_INPUT_RANGE);

Current tip changed that. No need to modify this anymore.

@@ +1468,5 @@
> +             mType == NS_FORM_INPUT_RANGE);
> +
> +  // Only type=range has a default minimum
> +  double defaultMinimum =
> +    (mType == NS_FORM_INPUT_RANGE) ? 0.0 : MOZ_DOUBLE_NaN();

nit: no need for the parenthesis.

@@ +1487,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 ||
> +             mType == NS_FORM_INPUT_RANGE);

As above, this is no longer needed.

@@ +1491,5 @@
> +             mType == NS_FORM_INPUT_RANGE);
> +
> +  // Only type=range has a default minimum
> +  double defaultMaximum =
> +    (mType == NS_FORM_INPUT_RANGE) ? 100.0 : MOZ_DOUBLE_NaN();

nit: no need for the parenthesis.

@@ +1509,5 @@
>  nsHTMLInputElement::GetStepBase() const
>  {
>    MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER ||
> +             mType == NS_FORM_INPUT_DATE ||
> +             mType == NS_FORM_INPUT_RANGE,

Could you change that to:
MOZ_ASSERT(DoesStepApply(), ...);

@@ +1704,5 @@
>  nsHTMLInputElement::MozIsTextField(bool aExcludePassword, bool* aResult)
>  {
>    // TODO: temporary until bug 635240 and 773205 are fixed.
>    if (mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE ||
> +      mType == NS_FORM_INPUT_TIME   || mType == NS_FORM_INPUT_RANGE) {

No need for that, IsSingleLineTextControl should return the expected value.

@@ +2508,5 @@
>    // Fire onchange (if necessary), before we do the blur, bug 357684.
>    if (aVisitor.mEvent->message == NS_BLUR_CONTENT) {
>      // In number inputs we can't allow the user to set an invalid value.
> +    if (mType == NS_FORM_INPUT_NUMBER ||
> +        mType == NS_FORM_INPUT_RANGE) {

You shouldn't do that here. The layout code should take care of this.

@@ +2800,5 @@
>                 (IsSingleLineTextControl(false, mType) ||
>                  mType == NS_FORM_INPUT_NUMBER ||
>                  mType == NS_FORM_INPUT_TIME ||
> +                mType == NS_FORM_INPUT_DATE ||
> +                mType == NS_FORM_INPUT_RANGE)) {

ditto.

@@ +3104,5 @@
> +                   "type=range should have a default maximum/minimum");
> +
> +        // We use this to avoid modifying the string unnecessarily, since that
> +        // may introduce rounding. Set to true if the value needs adjustment.
> +        bool serializeFromDouble = false;

I think the name of the variable is confusing. I actually understood what it was used for while reading the code. I think it would be better to name it "shouldUpdate" or "needSanitization" or whatever like that. We indeed serialize but I'm not sure this is relevant.

@@ +3107,5 @@
> +        // may introduce rounding. Set to true if the value needs adjustment.
> +        bool serializeFromDouble = false;
> +
> +        nsresult ec;
> +        double value = PromiseFlatString(aValue).ToDouble(&ec);

You should use ConvertStringToNumber(aValue, value).

@@ +3111,5 @@
> +        double value = PromiseFlatString(aValue).ToDouble(&ec);
> +        if (NS_FAILED(ec)) {
> +          serializeFromDouble = true;
> +          // set to midway between minimum and maximum:
> +          value = maximum <= minimum ? minimum : minimum + (maximum - minimum)/2;

Change 2 to 2.0 to keep the constant as a double.

@@ +4534,5 @@
>  nsHTMLInputElement::PlaceholderApplies() const
>  {
>    if (mType == NS_FORM_INPUT_DATE ||
> +      mType == NS_FORM_INPUT_TIME ||
> +      mType == NS_FORM_INPUT_RANGE) {

No need for that. NS_FORM_INPUT_RANGE isn't a text control.

@@ +4546,5 @@
>  nsHTMLInputElement::DoesPatternApply() const
>  {
>    // TODO: temporary until bug 635240 and bug 773205 are fixed.
>    if (mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE ||
> +      mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_RANGE) {

Same here.

@@ +4597,5 @@
>  nsHTMLInputElement::GetStep() const
>  {
> +  MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER ||
> +             mType == NS_FORM_INPUT_DATE ||
> +             mType == NS_FORM_INPUT_RANGE,

Current tip makes this change no longer needed.

@@ +4912,5 @@
> +  bool hasRangeOverflow;
> +  if (mType == NS_FORM_INPUT_RANGE) {
> +    // The HTML5 spec does not allow the input's value for type=range to suffer
> +    // from overflow/underflow, so this is always false.
> +    hasRangeOverflow = false;

I think it would be interesting to actually call the method in DEBUG and ASSERT if the result is different from false.

@@ +4926,5 @@
> +  bool hasRangeUnderflow;
> +  if (mType == NS_FORM_INPUT_RANGE) {
> +    // The HTML5 spec does not allow the input's value for type=range to suffer
> +    // from overflow/underflow, so this is always false.
> +    hasRangeUnderflow = false;

ditto.

@@ +4938,5 @@
>  nsHTMLInputElement::UpdateStepMismatchValidityState()
>  {
> +  // Note that the HTML5 spec DOES allow the input's value for type=range to
> +  // suffer from a step mismatch (if correcting the mismatch would make its
> +  // value suffer from underflow/overflow).

I'm not certain that this comment is really needed here.

@@ +5578,1 @@
>        return kStepScaleFactorNumber;

nit: you might want to rename the constant kStepScaleFactorNumberRange.

@@ +5604,5 @@
>  {
>    mHasRange = false;
>  
> +  if (mType != NS_FORM_INPUT_NUMBER && mType != NS_FORM_INPUT_DATE &&
> +      mType != NS_FORM_INPUT_RANGE) {

Current tip makes this change no longer needed.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Also, I'm not sure if I agree with the :SanitizeValue() change. I wonder if
> it wouldn't be simpler by changing the value if the element happens to be
> invalid. In other words, when you check if the element has an overflow, if
> that's the case, you could set the value to maximum. Same for underflow and
> minimum and step mismatch. I think that might require some re-ordering in
> how we check validity though.

I'm not sure exactly how you see that working differently to what I have. I tried to do pretty much that, while being very careful to honor the HTML5 priority rules. Anyway, we can discuss this tomorrow.

> @@ +1509,5 @@
> >  nsHTMLInputElement::GetStepBase() const
> >  {
> >    MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER ||
> > +             mType == NS_FORM_INPUT_DATE ||
> > +             mType == NS_FORM_INPUT_RANGE,
> 
> Could you change that to:
> MOZ_ASSERT(DoesStepApply(), ...);

Actually, I was very specifically asserting the types here, since it would be easy for someone to add a type to DoesStepApply, without checking here that they are using the correct kDefaultStepBase.

> @@ +4534,5 @@
> >  nsHTMLInputElement::PlaceholderApplies() const
> >  {
> >    if (mType == NS_FORM_INPUT_DATE ||
> > +      mType == NS_FORM_INPUT_TIME ||
> > +      mType == NS_FORM_INPUT_RANGE) {
> 
> No need for that. NS_FORM_INPUT_RANGE isn't a text control.
> 
> @@ +4546,5 @@
> >  nsHTMLInputElement::DoesPatternApply() const
> >  {
> >    // TODO: temporary until bug 635240 and bug 773205 are fixed.
> >    if (mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE ||
> > +      mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_RANGE) {
> 
> Same here.

If we're not expected to get to this code for 'range', then it seems like there should be assertions here that assert that. Should I should be adding some sort of assert here that asserts |IsSingleLineTextControl()| or |MozIsTextField()| ... or something like that?

> @@ +4938,5 @@
> >  nsHTMLInputElement::UpdateStepMismatchValidityState()
> >  {
> > +  // Note that the HTML5 spec DOES allow the input's value for type=range to
> > +  // suffer from a step mismatch (if correcting the mismatch would make its
> > +  // value suffer from underflow/overflow).
> 
> I'm not certain that this comment is really needed here.

I think it's useful since it's unclear from the spec and someone might think it's a bug that we're not just hardcoding it to false as we do for UpdateRangeUnderflowValidityState/UpdateRangeOverflowValidityState. I'm happy to remove the comment if that's what you prefer though.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Updated with most review comments addressed. (Will discuss anything outstanding tomorrow.)

Comment 7

[Mounir Lamouri (:mounir)]

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

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

r-, because of the step mismatch thing.

We should figure out the best way to fix that.

Also, could you open a follow-up for @list support?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +891,5 @@
> +        // whether we have a step mismatch or not. E.g. consider:
> +        // <input type=range value=-1 max=1 step=3>. The valid range is 0 to 1
> +        // while the nearest valid steps are -1 and 2 (the max value having
> +        // prevented there being a valid step in range).
> +        UpdateStepMismatchValidityState();

Unfortunately, I think this is not going to be enough, we never set the value to the new correct value. This call might be a no-op. We could call SetValue(GetValue()); to force sanitization at that point or we could make UpdateRangeOVerflowValidityState() changes the value. We should think about the best way to do that.

@@ +4894,5 @@
>  {
> +  bool hasRangeOverflow;
> +  if (mType == NS_FORM_INPUT_RANGE) {
> +    MOZ_ASSERT(!IsRangeOverflow(), "HTML5 spec does not allow this");
> +    hasRangeOverflow = false;

I realise that technically, we could even do an early return here but we need to set the validity back to true when we change the type. I guess keeping this code as is is less disruptive and simpler to understand and it shouldn't eat too much cycles.

@@ +4907,5 @@
>  {
> +  bool hasRangeUnderflow;
> +  if (mType == NS_FORM_INPUT_RANGE) {
> +    MOZ_ASSERT(!IsRangeUnderflow(), "HTML5 spec does not allow this");
> +    hasRangeUnderflow = false;

ditto

@@ +4919,5 @@
>  nsHTMLInputElement::UpdateStepMismatchValidityState()
>  {
> +  // Note that the HTML5 spec DOES allow the input's value for type=range to
> +  // suffer from a step mismatch (if correcting the mismatch would make its
> +  // value suffer from underflow/overflow).

I really prefer to have this comment removed. I fully agree that the specs are not clear regarding this but if someone reads this comment without reading the specs, it would be quite not understandable.

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> (In reply to Mounir Lamouri (:mounir) from comment #4)
> > @@ +1509,5 @@
> > >  nsHTMLInputElement::GetStepBase() const
> > >  {
> > >    MOZ_ASSERT(mType == NS_FORM_INPUT_NUMBER ||
> > > +             mType == NS_FORM_INPUT_DATE ||
> > > +             mType == NS_FORM_INPUT_RANGE,
> > 
> > Could you change that to:
> > MOZ_ASSERT(DoesStepApply(), ...);
> 
> Actually, I was very specifically asserting the types here, since it would
> be easy for someone to add a type to DoesStepApply, without checking here
> that they are using the correct kDefaultStepBase.

Good point.

> > @@ +4534,5 @@
> > >  nsHTMLInputElement::PlaceholderApplies() const
> > >  {
> > >    if (mType == NS_FORM_INPUT_DATE ||
> > > +      mType == NS_FORM_INPUT_TIME ||
> > > +      mType == NS_FORM_INPUT_RANGE) {
> > 
> > No need for that. NS_FORM_INPUT_RANGE isn't a text control.
> > 
> > @@ +4546,5 @@
> > >  nsHTMLInputElement::DoesPatternApply() const
> > >  {
> > >    // TODO: temporary until bug 635240 and bug 773205 are fixed.
> > >    if (mType == NS_FORM_INPUT_NUMBER || mType == NS_FORM_INPUT_DATE ||
> > > +      mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_RANGE) {
> > 
> > Same here.
> 
> If we're not expected to get to this code for 'range', then it seems like
> there should be assertions here that assert that. Should I should be adding
> some sort of assert here that asserts |IsSingleLineTextControl()| or
> |MozIsTextField()| ... or something like that?

No need to. I mean, we can ASSERT but we just generally don't.

> > @@ +4938,5 @@
> > >  nsHTMLInputElement::UpdateStepMismatchValidityState()
> > >  {
> > > +  // Note that the HTML5 spec DOES allow the input's value for type=range to
> > > +  // suffer from a step mismatch (if correcting the mismatch would make its
> > > +  // value suffer from underflow/overflow).
> > 
> > I'm not certain that this comment is really needed here.
> 
> I think it's useful since it's unclear from the spec and someone might think
> it's a bug that we're not just hardcoding it to false as we do for
> UpdateRangeUnderflowValidityState/UpdateRangeOverflowValidityState. I'm
> happy to remove the comment if that's what you prefer though.

It's too close to the specs and comes out of the blue when simply reading the specs IMO.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 10

[Daniel Holbert [:dholbert]]

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

> double
> nsHTMLInputElement::GetMaximum() const
> {
>   MOZ_ASSERT(DoesValueAsNumberApply(),
>              "GetMaxAsDouble() should only be used for types that allow .valueAsNumber");
> 
>-  // Once we add support for types that have a default minimum/maximum, take
>-  // account of the default maximum here.
>+  // Only type=range has a default minimum
>+  double defaultMaximum =
>+    mType == NS_FORM_INPUT_RANGE ? 100.0 : MOZ_DOUBLE_NaN();

nit: s/minimum/maximum/ in that comment.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch to make sure GetMinimum/Maximum can't return +/-Infinity

Comment 12

[Jonathan Watt [:jwatt]]

Addressing comment 4 to make this a non-text based type means that this bug now depends on bug 665655. Mounir is going to clean up and land his patches for that soon.

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 713027 [details] [diff] [review]
patch to make sure GetMinimum/Maximum can't return +/-Infinity

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

ConvertStringToNumber() should return a finite value when it succeed (returns true).
I agree that nothing proves us that .ToDouble() doesn't return NaN but I doubt this is possible to parse a string to a NaN or Infinity. If you really want to, you can add an assert somewhere.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 713027 [details] [diff] [review]
patch to make sure GetMinimum/Maximum can't return +/-Infinity

Fine by me. I'll add an assert to ConvertStringToNumber in the main patch here.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:mounir) from comment #13)
> I agree that nothing proves us that .ToDouble() doesn't return NaN but I
> doubt this is possible to parse a string to a NaN or Infinity.

It's absolutely possible to parse infinity.  This minimal testcase gets us an infinite value in ConvertStringToNumber (with dom.experimental_forms turned on):
  <!DOCTYPE html><input type="number" max="1e3000">

(Note that the maximum representable double value is approximately 1.7e308, per http://msdn.microsoft.com/en-us/library/s086ab1z%28v=vs.71%29.aspx )

Comment 16

[Daniel Holbert [:dholbert]]

(To be clear, I don't think we necessarily need to wring our hands super-much about having awesome behavior when values like 1e3000 are involved, or necessarily catching *every* possible place where we might end up accidentally getting infinity.

But when we've got a possibly-bogus user-provided value that's definitely going to make our arithmetic explode, and we can come up with reasonable fallback behavior, as jwatt has, I think we might as well add a check.)

Comment 17

[Jonathan Watt [:jwatt]]

Good catch Daniel. I filed bug 840706 and bug 840720.

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: patch addressing other offline discussion

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch + fixes for issues caught by tests

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch

Meh. Just mismerged those fixes.

Comment 21

[Daniel Holbert [:dholbert]]

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

>@@ -810,16 +813,25 @@ nsHTMLInputElement::AfterSetAttr(int32_t
[...]
>+    if (aName == nsGkAtoms::value && mType == NS_FORM_INPUT_RANGE){
>+      // For type=range the spec requires the value to be sanitized:

Nit: add space between ) and {

Comment 22

[Mounir Lamouri (:mounir)]

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

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

I will need to have a look at this patch later. I've been reviewing a lot of code today and I need a break to make sure I don't miss something.

Could you update the patch and I will have another look hopefully tonight.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +824,5 @@
> +      aValue->ToString(value);
> +      SetValueInternal(value, false, false);
> +      MOZ_ASSERT(!GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW),
> +                 "HTML5 spec does not allow this");
> +    }

The previous test should actually do what you are expecting: if @value is used for .value, it will be sanitized and set as .value. Don't worry about that for type='range'. If you saw something wrong regarding this, please open a bug.

@@ +905,5 @@
> +        // while the nearest valid steps are -1 and 2 (the max value having
> +        // prevented there being a valid step in range).
> +        UpdateStepMismatchValidityState();
> +        if (GetValidityState(VALIDITY_STATE_STEP_MISMATCH) ||
> +            GetValidityState(VALIDITY_STATE_RANGE_OVERFLOW)) {

I'm not sure how the element can have a step mismatch after changing the max (given that the value hasn't changed yet), max doesn't enter into account into checking the step.

Actually, are there situations were we could need to sanitise even if the element doesn't suffer from an overflow?

@@ +921,5 @@
>        UpdateStepMismatchValidityState();
> +      if (mType == NS_FORM_INPUT_RANGE &&
> +          (GetValidityState(VALIDITY_STATE_STEP_MISMATCH) ||
> +           GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) ||
> +           GetValidityState(VALIDITY_STATE_RANGE_OVERFLOW))) {

Same here for overflow: changing the min shouldn't change the overflow (we can suffer from a stepmismatch though).

@@ +3153,5 @@
> +        bool needSanitization = false;
> +
> +        double value;
> +        bool ok = ConvertStringToNumber(aValue, value);
> +        if (!ok) {

nit: between using a boolean named "ok" and having the call directly in the condition, I wouldn't mind having the call in the condition but I don't care much ;)

@@ +3155,5 @@
> +        double value;
> +        bool ok = ConvertStringToNumber(aValue, value);
> +        if (!ok) {
> +          needSanitization = true;
> +          // set to midway between minimum and maximum:

// Set value to midway between minimum and maximum.

@@ +3180,5 @@
> +            // greater than or equal to the minimum, and, if the maximum is not
> +            // less than the minimum, which is less than or equal to the
> +            // maximum, if there is a number that matches these constraints:
> +            double stepBelow, stepAbove;
> +            if (deltaToStep < 0) {

I think deltaToStep can't be < 0 because allowedValueStep can't be <= 0 and deltaToStep will have the sign of allowedValueStep.

@@ +3188,5 @@
> +              stepAbove = value + deltaToStep;
> +              stepBelow = stepAbove - allowedValueStep;
> +            }
> +            double halfStep = allowedValueStep / 2;
> +            bool stepAboveIsClosest = std::abs(stepAbove - value) <= halfStep;

Why are you calling abs() here given that (stepAbove - value) is known to be > 0 given that stepAbove > value?

Also, maybe you could move this a bit below, so you can do:
  bool stepAboveIsClosest = stepAboveInRange ? (stepAbove - value) <= (allowedValueStep /2) : false;

That could simplify the conditions after.

@@ +3198,5 @@
> +            if ((stepAboveIsClosest || !stepBelowInRange) && stepAboveInRange) {
> +              needSanitization = true;
> +              value = stepAbove;
> +            }
> +            else if ((!stepAboveIsClosest || !stepAboveInRange) && stepBelowInRange) {

nit:
} else if () {

Comment 23

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #22)
> nit: between using a boolean named "ok" and having the call directly in the
> condition, I wouldn't mind having the call in the condition but I don't care
> much ;)

I would normally agree, except where the variable being fetched is used outside the body of the |if| (e.g. in the |else| or later). I'll change it if you insist though.

> I think deltaToStep can't be < 0 because allowedValueStep can't be <= 0 and
> deltaToStep will have the sign of allowedValueStep.

The sign of deltaToStep also depends on the result of |stepBase - value|, and that can be negative (remember |value| in unsanitized at this point). So yes, it can be negative.

> Why are you calling abs() here given that (stepAbove - value) is known to be
> > 0 given that stepAbove > value?

Left over junk from when my logic was wrong - removed.

> Also, maybe you could move this a bit below, so you can do:
>   bool stepAboveIsClosest = stepAboveInRange ? (stepAbove - value) <=
> (allowedValueStep /2) : false;

Hmm, I really don't like that. IMO it makes the logic much harder to understand and reason about than what I currently have and for no real gain. (I think the variable name would also have to change too, since it would no longer be accurate.)

Comment 24

[Jonathan Watt [:jwatt]]

Attachment: patch with inital review comments addressed

Comment 25

[Mounir Lamouri (:mounir)]

Comment on attachment 714322 [details] [diff] [review]
patch with inital review comments addressed

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

I think you forgot to took my comments regarding the step part of ::SanitizeValue() into account. Could you re-attach a patch with those comments applied?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +896,5 @@
> +        // a step mismatch and a value that results in overflow. For example,
> +        // if @max in the example above were to change from 1 to -1.
> +        nsAutoString value;
> +        GetValue(value);
> +        SetValueInternal(value, false, false);

That should work and for a first pass lets consider this enough but please add a TODO comment saying that this should be clean-up, open a bug and put the bug number in the comment.

@@ +908,5 @@
> +      if (mType == NS_FORM_INPUT_RANGE) {
> +        // See @max comment
> +        nsAutoString value;
> +        GetValue(value);
> +        SetValueInternal(value, false, false);

ditto

@@ +918,5 @@
> +      if (mType == NS_FORM_INPUT_RANGE) {
> +        // See @max comment
> +        nsAutoString value;
> +        GetValue(value);
> +        SetValueInternal(value, false, false);

ditto

@@ +3137,5 @@
> +        double value;
> +        bool ok = ConvertStringToNumber(aValue, value);
> +        if (!ok) {
> +          needSanitization = true;
> +          // set value to midway between minimum and maximum:

nit: "// Set ... maximum."

@@ +3162,5 @@
> +            // greater than or equal to the minimum, and, if the maximum is not
> +            // less than the minimum, which is less than or equal to the
> +            // maximum, if there is a number that matches these constraints:
> +            double stepBelow, stepAbove;
> +            if (deltaToStep < 0) {

Did you take my previous comment into account for this?

@@ +3169,5 @@
> +            } else {
> +              stepAbove = value + deltaToStep;
> +              stepBelow = stepAbove - allowedValueStep;
> +            }
> +            double halfStep = allowedValueStep / 2;

and this?

Comment 26

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #25)
> I think you forgot to took my comments regarding the step part of
> ::SanitizeValue() into account.

Did you see comment 23? I replied there.

Comment 27

[Mounir Lamouri (:mounir)]

Oups, I did not see that comment!

(In reply to Jonathan Watt [:jwatt] from comment #23)
> (In reply to Mounir Lamouri (:mounir) from comment #22)
> > nit: between using a boolean named "ok" and having the call directly in the
> > condition, I wouldn't mind having the call in the condition but I don't care
> > much ;)
> 
> I would normally agree, except where the variable being fetched is used
> outside the body of the |if| (e.g. in the |else| or later). I'll change it
> if you insist though.

Ok. That was a "nit" that means a simple meaningless comment. No need to worry about it.

> > I think deltaToStep can't be < 0 because allowedValueStep can't be <= 0 and
> > deltaToStep will have the sign of allowedValueStep.
> 
> The sign of deltaToStep also depends on the result of |stepBase - value|,
> and that can be negative (remember |value| in unsanitized at this point). So
> yes, it can be negative.

AFAIK, the regular fmod() methad would take the sign of x (with x%y) but floorModulo takes the sign of y.

> > Also, maybe you could move this a bit below, so you can do:
> >   bool stepAboveIsClosest = stepAboveInRange ? (stepAbove - value) <=
> > (allowedValueStep /2) : false;
> 
> Hmm, I really don't like that. IMO it makes the logic much harder to
> understand and reason about than what I currently have and for no real gain.
> (I think the variable name would also have to change too, since it would no
> longer be accurate.)

Ok.

Comment 28

[Mounir Lamouri (:mounir)]

Comment on attachment 714322 [details] [diff] [review]
patch with inital review comments addressed

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

r=me with all comments applied.

Also, please open those follow-ups:
- @list support (I know this is more layout than content);
- improve the way to "clamp" the type='range' value;
- refactor a bit some step code (basically, what you wrote in SanitiveValue() is duplicated in various places);
- you might change min/max behaviour (I will send an email to the WHATWG regarding this);
- a bug to enable the pref by default which will have to depend on other bugs (at least the number 2 and 4 of this list, probably the 1);
- actually, we might want two bugs for this: we should enable the pref on Nigthly and Aurora as soon as the element is usable (which means you can select a value).

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +3147,5 @@
> +          needSanitization = true;
> +          value = maximum;
> +        }
> +
> +        double allowedValueStep = GetStep();

Call this 'step', the entire file use that name.

@@ +3162,5 @@
> +            // greater than or equal to the minimum, and, if the maximum is not
> +            // less than the minimum, which is less than or equal to the
> +            // maximum, if there is a number that matches these constraints:
> +            double stepBelow, stepAbove;
> +            if (deltaToStep < 0) {

I still believe that this can't be negative.

@@ +3167,5 @@
> +              stepBelow = value + deltaToStep;
> +              stepAbove = stepBelow + allowedValueStep;
> +            } else {
> +              stepAbove = value + deltaToStep;
> +              stepBelow = stepAbove - allowedValueStep;

Actually, could you do something like:
double deltaToStep = NS_floorModulo(value - stepBase, step);
stepBelow = value - deltaToStep;
stepAbove = value + allowedValueStep - deltaToStep;

Comment 29

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #28)
> Also, please open those follow-ups:
> - @list support (I know this is more layout than content);

Bug 841942.

> - improve the way to "clamp" the type='range' value;

Bug 841943.

> - refactor a bit some step code (basically, what you wrote in
> SanitiveValue() is duplicated in various places);

Bug 841945.

> - you might change min/max behaviour (I will send an email to the WHATWG
> regarding this);

Bug 841946.

> - a bug to enable the pref by default which will have to depend on other
> bugs (at least the number 2 and 4 of this list, probably the 1);

Bug 841948.

> - actually, we might want two bugs for this: we should enable the pref on
> Nigthly and Aurora as soon as the element is usable (which means you can
> select a value).

Bug 841950.

Comment 30

[Jonathan Watt [:jwatt]]

https://bugzilla.mozilla.org/show_bug.cgi?id=836314

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/125640d90631