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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla2.0b8
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
This has been recently introduced in WHATWG HTML specifications (r5539).
Aryeh, feel like writing a patch?
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.
Assignee: nobody → Simetrical+ff
Status: NEW → ASSIGNED

Updated

7 years ago
Blocks: 344614
(Assignee)

Comment 3

7 years ago
(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?
Created attachment 481609 [details] [diff] [review]
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.
Status: ASSIGNED → NEW
Assignee: Simetrical+ff → nobody
(Assignee)

Comment 5

7 years ago
'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.
I probably don't have time to do anything like this right now.
(Assignee)

Comment 7

7 years ago
Created attachment 485582 [details] [diff] [review]
Patch v1
Assignee: nobody → mounir.lamouri
Attachment #481609 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #485582 - Flags: review?(Olli.Pettay)
Attachment #485582 - Flags: feedback?(Simetrical+ff)
(Assignee)

Updated

7 years ago
Keywords: dev-doc-needed

Comment 8

7 years ago
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?
(Assignee)

Comment 9

7 years ago
(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 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
Attachment #485582 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

7 years ago
Attachment #485582 - Flags: approval2.0?
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
Attachment #485582 - Flags: feedback?(Simetrical+ff) → feedback-
(Assignee)

Comment 12

7 years ago
(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 :)
(Assignee)

Comment 13

7 years ago
Created attachment 485872 [details] [diff] [review]
Inter diff
(Assignee)

Comment 14

7 years ago
Created attachment 485874 [details] [diff] [review]
Patch v2

r=smaug

This should be better.
Attachment #485582 - Attachment is obsolete: true
Attachment #485874 - Flags: feedback?(Simetrical+ff)
Attachment #485582 - Flags: approval2.0?
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.
Attachment #485874 - Flags: feedback?(Simetrical+ff) → feedback+
(Assignee)

Updated

7 years ago
Attachment #485874 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Attachment #485872 - Attachment is obsolete: true

Updated

7 years ago
Attachment #485874 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 16

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

Thank you for your help Aryeh :)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(Assignee)

Updated

7 years ago
Blocks: 610212
(Assignee)

Updated

7 years ago
Blocks: 610214
Documentation updated:

https://developer.mozilla.org/en/HTML/Element/Input#Attributes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.