601061 - input.size should be limited to non negative numbers greater than zero with a default value of 20

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

This has been recently introduced in WHATWG HTML specifications (r5539).

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Aryeh, feel like writing a patch?

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Only fair, since I instigated the spec change.  :)  If anyone else wants to do it, though, fine by me -- I don't know when I'll get to it, could be a while.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> Only fair, since I instigated the spec change.  :)  If anyone else wants to do
> it, though, fine by me -- I don't know when I'll get to it, could be a while.

Do you think you can do that for beta8?

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch that works, but isn't the right way to do it

This patch works, in that it replicates the specced behavior in a black-box way, but it's not the right way to do it.  Basically, all the helper functions here assume that everything is signed, so the only way to get this to work trivially was to change the IDL to make this a signed value, and copy the code for select.size -- which is long in Firefox's IDLs per DOM 2 HTML, but HTML5 more sensibly says it's unsigned long: <http://www.w3.org/Bugs/Public/show_bug.cgi?id=10345>.  The right thing to do would be to add the appropriate support functions for unsigned longs and move everything over to use that as HTML5 says, or at least change select.size.  I could do this in principle, but I don't have the time right now, so I'll unassign this from me.

I could still do a more limited patch that just changed the default value, I guess, if that's desired.  But cleanup is needed either way.  The spec's model of how reflected attributes work seems to differ considerably from Gecko's model, and AFAICT the spec's is more reasonable and consistent.  E.g., in the spec, all signed longs are treated identically except for default value and whether they're limited to non-negative values, and similarly for unsigned longs.  Gecko has a bunch of places where it clamps particular attributes and so on, like longs where it silently ignores negative values instead of just using an unsigned long.

Comment 5

[Mounir Lamouri (:mounir)]

'unsigned long' should be reflected in the range 0 to 2147483647. Which is the positive value of a 'long'. I think we only need a GetUIntAttr() and SetUIntAttr() to make sure the values are correct. IOW, GetUIntAttr() shouldn't return a negative value (but that would be correctly parsed) and SetUIntAttr() should change the value if bigger than 2147483647.

Aryeh, if you still don't have time to work on that, let me know so I will do it.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I probably don't have time to do anything like this right now.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 485582 [details] [diff] [review]
Patch v1


>-//NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 0)
>+NS_IMPL_UINT_ATTR_NON_NULL_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 20)
I think you should use DEFAULT_COLS (which is 20)



>+  var nonValidValues = [ -1, 3147483647 ];
Could you please test also PR_INT32_MIN. And perhaps
even something smaller.


I couldn't find the email thread about this.
Why is the change needed?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #8)
> >-//NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 0)
> >+NS_IMPL_UINT_ATTR_NON_NULL_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 20)
> I think you should use DEFAULT_COLS (which is 20)

Ok. Actually, GetCols() could probably be merged with GetSize(). They do the same think now. I should look at that.

> >+  var nonValidValues = [ -1, 3147483647 ];
> Could you please test also PR_INT32_MIN. And perhaps
> even something smaller.

Ok.

> I couldn't find the email thread about this.
> Why is the change needed?

The change was needed because Trident (since IE8) and Webkit do that and that's the default size (in the behavior). See http://www.w3.org/Bugs/Public/show_bug.cgi?id=10517

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 485582 [details] [diff] [review]
Patch v1


> nsresult
>+nsGenericHTMLElement::GetUnsignedIntAttr(nsIAtom* aAttr, PRUint32 aDefault,
>+                                         PRUint32* aResult)
>+{
>+  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(aAttr);
>+  if (attrVal && attrVal->Type() == nsAttrValue::eInteger) {
>+    *aResult = attrVal->GetIntegerValue();
>+  }
>+  else {
} else {


>+#define NS_IMPL_UINT_ATTR_NON_NULL(_class, _method, _atom)                \
Maybe
          NS_IMPL_NON_ZERO_UINT_ATTR

>+#define NS_IMPL_UINT_ATTR_NON_NULL_DEFAULT_VALUE(_class, _method, _atom, _default) \
And here
          NS_IMPL_NON_ZERO_UINT_ATTR_DEFAULT_VALUE

+the old comments

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 485582 [details] [diff] [review]
Patch v1

>+nsGenericHTMLElement::GetUnsignedIntAttr(nsIAtom* aAttr, PRUint32 aDefault,
>+                                         PRUint32* aResult)
>+{
>+  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(aAttr);
>+  if (attrVal && attrVal->Type() == nsAttrValue::eInteger) {
>+    *aResult = attrVal->GetIntegerValue();
>+  }
>+  else {
>+    *aResult = aDefault;
>+  }
>+  return NS_OK;
>+}

I'm not sure what this does.  The spec says that if the value is negative or greater than 2147483647, this has to return the default value.  Does this actually do that?  I don't see where.

>+nsresult
>+nsGenericHTMLElement::SetUnsignedIntAttr(nsIAtom* aAttr, PRUint32 aValue)
>+{
>+  nsAutoString value;
>+  value.AppendInt(PR_MIN(aValue, PR_INT32_MAX));
>+
>+  return SetAttr(kNameSpaceID_None, aAttr, value, PR_TRUE);
>+}

The clamping is wrong here, per discussion in #whatwg.  If you set to something in [2147483648, 4294967295], then the content attribute must be set to that attribute, but a subsequent get must return the default value.  This looks like it will set the content attribute to 2147483647, which is wrong.  After input.size = 2147483648, input.getAttribute("size") is "2147483648" and input.size is 20.

>+#define NS_IMPL_UINT_ATTR_NON_NULL_DEFAULT_VALUE(_class, _method, _atom, _default) \
>+  NS_IMETHODIMP                                                           \
>+  _class::Get##_method(PRUint32* aValue)                                  \
>+  {                                                                       \
>+    return GetUnsignedIntAttr(nsGkAtoms::_atom, _default, aValue);        \
>+  }                                                                       \

This is incorrect if the value is 0 -- is that possible somehow, maybe by setAttribute()?  Or is it impossible?

>+    is(aElement[aAttr], aValue, "." + aAttr + " should be equals " + aDefault);
>+    is(aElement.getAttribute(aAttr), aValue,
>+       "@" + aAttr + " should be equals " + aDefault);

Maybe " should equal "?  (Also later on.)

>+  for each (var value in nonValidValues) {
>+    aElement[aAttr] = value;
>+    checkGetter(aElement, aAttr, 2147483647);
>+  }

This is wrong, as noted.  For negative values, it should be equal to aDefault.  For 3147483647, .getAttribute("size") should return 3147483647 but .size should return aDefault.


I don't see any other problems, although I didn't understand some of it.  There are a lot of other cases where Gecko implements stuff as long instead of unsigned long -- now that you've defined the macros, maybe you could fix bug 586126 at the same time so you can use the same test for both.

Also, I already wrote a fairly comprehensive test suite for HTML5 reflection, which is how I found all these bugs in the first place.  Maybe you could adapt that:

http://dvcs.w3.org/hg/html/raw-file/tip/tests/submission/AryehGregor/reflection.html

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> Comment on attachment 485582 [details] [diff] [review]
> Patch v1
> 
> >+nsGenericHTMLElement::GetUnsignedIntAttr(nsIAtom* aAttr, PRUint32 aDefault,
> >+                                         PRUint32* aResult)
> >+{
> >+  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(aAttr);
> >+  if (attrVal && attrVal->Type() == nsAttrValue::eInteger) {
> >+    *aResult = attrVal->GetIntegerValue();
> >+  }
> >+  else {
> >+    *aResult = aDefault;
> >+  }
> >+  return NS_OK;
> >+}
> 
> I'm not sure what this does.  The spec says that if the value is negative or
> greater than 2147483647, this has to return the default value.  Does this
> actually do that?  I don't see where.

It can't be greater that 2147483647 because we save the value in a PRInt32. The <0 is done when parsing the value. If not a positive value, it will not be valid thus |attrVal && attrVal->Type() == nsAttrValue::eInteger| will be false.

> >+nsresult
> >+nsGenericHTMLElement::SetUnsignedIntAttr(nsIAtom* aAttr, PRUint32 aValue)
> >+{
> >+  nsAutoString value;
> >+  value.AppendInt(PR_MIN(aValue, PR_INT32_MAX));
> >+
> >+  return SetAttr(kNameSpaceID_None, aAttr, value, PR_TRUE);
> >+}
> 
> The clamping is wrong here, per discussion in #whatwg.  If you set to something
> in [2147483648, 4294967295], then the content attribute must be set to that
> attribute, but a subsequent get must return the default value.  This looks like
> it will set the content attribute to 2147483647, which is wrong.  After
> input.size = 2147483648, input.getAttribute("size") is "2147483648" and
> input.size is 20.

I'm working on fixing that.

> >+#define NS_IMPL_UINT_ATTR_NON_NULL_DEFAULT_VALUE(_class, _method, _atom, _default) \
> >+  NS_IMETHODIMP                                                           \
> >+  _class::Get##_method(PRUint32* aValue)                                  \
> >+  {                                                                       \
> >+    return GetUnsignedIntAttr(nsGkAtoms::_atom, _default, aValue);        \
> >+  }                                                                       \
> 
> This is incorrect if the value is 0 -- is that possible somehow, maybe by
> setAttribute()?  Or is it impossible?

It's not because it's parsed. As you can see, tests check that.

> >+  for each (var value in nonValidValues) {
> >+    aElement[aAttr] = value;
> >+    checkGetter(aElement, aAttr, 2147483647);
> >+  }
> 
> This is wrong, as noted.  For negative values, it should be equal to aDefault. 
> For 3147483647, .getAttribute("size") should return 3147483647 but .size should
> return aDefault.

Ok.

> I don't see any other problems, although I didn't understand some of it.  There
> are a lot of other cases where Gecko implements stuff as long instead of
> unsigned long -- now that you've defined the macros, maybe you could fix bug
> 586126 at the same time so you can use the same test for both.

I will try to fix this bug later.

> Also, I already wrote a fairly comprehensive test suite for HTML5 reflection,
> which is how I found all these bugs in the first place.  Maybe you could adapt
> that:
> 
> http://dvcs.w3.org/hg/html/raw-file/tip/tests/submission/AryehGregor/reflection.html

I had a look. By the way, we need to talk about how select.size is reflected.

Thanks for this feedback. It was very useful :)

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

r=smaug

This should be better.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 485874 [details] [diff] [review]
Patch v2

Based on the interdiff, the new patch looks fine, although I didn't look too closely.

(In reply to comment #12)
> I had a look. By the way, we need to talk about how select.size is reflected.

Any time.

Comment 16

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/aeba37b12aed

Thank you for your help Aryeh :)

Comment 17

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/HTML/Element/Input#Attributes