Closed Bug 551846 Opened 10 years ago Closed 10 years ago

select element size attribute default should be 4 when multiple attribute is present

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, html5, Whiteboard: [see comment 43])

Attachments

(2 files, 11 obsolete files)

According to the specs, the size attribute default value is 1 when multiple is absent and 4 when multiple is present.
A patch will probably come tomorrow.
If we do this we can probably simplify a bunch of code in CSS frame constructor and forms.css (and resolve some old bugs).
At least assuming the getAttribute() values are affected.  If they're not we can only simplify frame constructor code.
As I understand it, getAttribute should not be affected but only the reflecting IDL attribute.
Attached patch Test v0.1 (obsolete) — Splinter Review
Attachment #432589 - Flags: review?(bzbarsky)
Attached patch Patch v0.1 (obsolete) — Splinter Review
This patch is fixing the bug and making the size attribute behaves like it should [1]. However, it is not cleaning up the frame constructor. I would like to do that in a follow-up bug if you agree.

[1] http://dev.w3.org/html5/spec/infrastructure.html#limited-to-only-non-negative-numbers-greater-than-zero
Attachment #432590 - Flags: review?(bzbarsky)
Attachment #432589 - Flags: review?(bzbarsky) → review?(ehsan.akhgari)
Comment on attachment 432590 [details] [diff] [review]
Patch v0.1

Changing the review to ehsan bcause Boris is out until 22/03.
Attachment #432590 - Flags: review?(bzbarsky) → review?(ehsan.akhgari)
I don't think Ehsan is a peer who can grant reviews on this code (Document Object Model) :
http://www.mozilla.org/about/owners.html

I know I'm not one.
ehsan certainly knows form control code nowadays.
And I can review this too.
I choosed ehsan because he reviewed my placeholder patch so I thought he may be able to review this one too. Let's add Olli and have two reviewers.
Attachment #432589 - Flags: review?(Olli.Pettay)
Attachment #432590 - Flags: review?(Olli.Pettay)
Comment on attachment 432589 [details] [diff] [review]
Test v0.1

>+  element.setAttribute('size', 2147483647); /* PR_INT32_MAX */
>+  is(element.size, 2147483647, "PR_INT32_MAX should be considered as a valid value");
>+
>+  element.setAttribute('size', -2147483648); /* PR_INT32_MIN */
>+  is(element.size, defaultValue,
>+    "The reflecting IDL attribute should return the default value when content attribute value is invalid");
>+
>+  element.setAttribute('size', 'non-numerical-value');
>+  is(element.size, defaultValue,
>+    "The reflecting IDL attribute should return the default value when content attribute value is invalid");

Could you check that getAttribute returns the right value
with int32max/min and 'non-numerical-value'
Attachment #432589 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 432590 [details] [diff] [review]
Patch v0.1

>nsAttrValue::ParseNonNegativeIntValue
Why you need to change this, since all the callers seem to expect -1 as
the default value.


>+PRBool
>+nsAttrValue::ParseNonNegativeUnsignedIntValue(const nsAString& aString,
>+                                              PRInt32 aDefaultValue)
>+{
>+  ResetIfSet();
>+
>+  PRInt32 ec;
>+  PRBool strict;
>+  PRInt32 originalVal = StringToInteger(aString, &strict, &ec);
>+  if (NS_FAILED(ec)) {
>+    return PR_FALSE;
>+  }
>+
>+  PRInt32 val = (originalVal <= 0)? aDefaultValue : originalVal;
>   strict = strict && (originalVal == val);
>   SetIntValueAndType(val, eInteger, strict ? nsnull : &aString);
> 
>   return PR_TRUE;
> }
Should this method be caller ParsePositiveIntValue?
There is the (originalVal <= 0) check, so default value is used
if the parsed value isn't positive.
And I don't understand the why the method name has
"Unsigned".

> /**
>+ * A macro to implement the getter and setter for a given content
>+ * property that needs to set a non-negative unsigned integer. The method
>+ * uses the generic GetAttr and SetAttr methods. This macro is much
>+ * like the NS_IMPL_NON_NEGATIVE_INT_ATTR macro except the exception is
>+ * thrown also when the value is equal to 0.
>+ */
>+#define NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR(_class, _method, _atom)    \
>+  NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR_DEFAULT_VALUE(_class, _method, _atom, 1)
>+
>+#define NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR_DEFAULT_VALUE(_class, _method, _atom, _default)  \
Perhaps NS_IMPL_POSITIVE_INT_ATTR and NS_IMPL_POSITIVE_INT_ATTR_DEFAULT_VALUE


> nsHTMLSelectElement::ParseAttribute(PRInt32 aNamespaceID,
>                                     nsIAtom* aAttribute,
>                                     const nsAString& aValue,
>                                     nsAttrValue& aResult)
> {
>   if (aAttribute == nsGkAtoms::size && kNameSpaceID_None == aNamespaceID) {
>-    return aResult.ParseIntWithBounds(aValue, 0);
>+    return aResult.ParseNonNegativeUnsignedIntValue(aValue, HasAttr(kNameSpaceID_None, nsGkAtoms::multiple)?4:1);

Space before and after '?' and ':'
Attachment #432590 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 432590 [details] [diff] [review]
Patch v0.1

This is all content, Smaug's review should be enough here.
Attachment #432590 - Flags: review?(ehsan.akhgari)
Comment on attachment 432589 [details] [diff] [review]
Test v0.1

It would also be interesting to test values higher than intmax and lower than intmin.
Attachment #432589 - Flags: review?(ehsan.akhgari)
(In reply to comment #10)
> (From update of attachment 432589 [details] [diff] [review])
> Could you check that getAttribute returns the right value
> with int32max/min and 'non-numerical-value'

If you want.

(In reply to comment #13)
> (From update of attachment 432589 [details] [diff] [review])
> It would also be interesting to test values higher than intmax and lower than
> intmin.

Ok.

(In reply to comment #11)
> (From update of attachment 432590 [details] [diff] [review])
> >nsAttrValue::ParseNonNegativeIntValue
> Why you need to change this, since all the callers seem to expect -1 as
> the default value.

Because the specs say "If, on the other hand, it fails or returns an out of range value, or if the attribute is absent, the default value must be returned instead, or −1 if there is no default value.".
I can add -1 as a default value for the default value ;)

> >+PRBool
> >+nsAttrValue::ParseNonNegativeUnsignedIntValue(const nsAString& aString,
> >+                                              PRInt32 aDefaultValue)
> >+{
> >+  ResetIfSet();
> >+
> >+  PRInt32 ec;
> >+  PRBool strict;
> >+  PRInt32 originalVal = StringToInteger(aString, &strict, &ec);
> >+  if (NS_FAILED(ec)) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  PRInt32 val = (originalVal <= 0)? aDefaultValue : originalVal;
> >   strict = strict && (originalVal == val);
> >   SetIntValueAndType(val, eInteger, strict ? nsnull : &aString);
> > 
> >   return PR_TRUE;
> > }
> Should this method be caller ParsePositiveIntValue?
> There is the (originalVal <= 0) check, so default value is used
> if the parsed value isn't positive.
> And I don't understand the why the method name has
> "Unsigned".

I tried to follow the words from the specification "If a reflecting IDL attribute is an unsigned integer type (unsigned long) that is limited to only non-negative numbers greater than zero, then [..]".
ParseNonNegativeIntValue is for parsing a non-negative number for an integer attribute. ParseNonNegativeUnsignedIntValue is for parsing a non-negative number for an unsigned int attribute.

Do you still want me to change the function name ?

> > /**
> >+ * A macro to implement the getter and setter for a given content
> >+ * property that needs to set a non-negative unsigned integer. The method
> >+ * uses the generic GetAttr and SetAttr methods. This macro is much
> >+ * like the NS_IMPL_NON_NEGATIVE_INT_ATTR macro except the exception is
> >+ * thrown also when the value is equal to 0.
> >+ */
> >+#define NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR(_class, _method, _atom)    \
> >+  NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR_DEFAULT_VALUE(_class, _method, _atom, 1)
> >+
> >+#define NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR_DEFAULT_VALUE(_class, _method, _atom, _default)  \
> Perhaps NS_IMPL_POSITIVE_INT_ATTR and NS_IMPL_POSITIVE_INT_ATTR_DEFAULT_VALUE
> 
> 
> > nsHTMLSelectElement::ParseAttribute(PRInt32 aNamespaceID,
> >                                     nsIAtom* aAttribute,
> >                                     const nsAString& aValue,
> >                                     nsAttrValue& aResult)
> > {
> >   if (aAttribute == nsGkAtoms::size && kNameSpaceID_None == aNamespaceID) {
> >-    return aResult.ParseIntWithBounds(aValue, 0);
> >+    return aResult.ParseNonNegativeUnsignedIntValue(aValue, HasAttr(kNameSpaceID_None, nsGkAtoms::multiple)?4:1);
> 
> Space before and after '?' and ':'

Ok.
(In reply to comment #14)
> ParseNonNegativeIntValue is for parsing a non-negative number for an integer
> attribute. ParseNonNegativeUnsignedIntValue is for parsing a non-negative
> number for an unsigned int attribute.
This is not very clear.
To me ParsePositiveIntValue is pretty clear. The result is > 0.
For ParseNonNegativeUnsignedIntValue and ParseNonNegativeIntValue
I would expect result to be >= 0 in both cases.
I not talking about html5 terminology, which may not be very well
defined; the html5 wording could be changed, I hope.
Blocks: 553957
Attached patch Test v0.2 (obsolete) — Splinter Review
I've added the requested tests and a test to check the size value after removeAttribute('size').
Attachment #432589 - Attachment is obsolete: true
Attached patch Patch v0.2 (obsolete) — Splinter Review
Every requested changes have been applied in this patch.
Attachment #432590 - Attachment is obsolete: true
Attachment #433863 - Flags: review?(Olli.Pettay)
Comment on attachment 433861 [details] [diff] [review]
Test v0.2

Could you please check that element.size is correct after removing some invalid attribute:
element.setAttribute("size", "foobar");
ok(element.size, ...)
element.removeAttribute("size")
ok(element.size, ....)
Comment on attachment 433863 [details] [diff] [review]
Patch v0.2

> PRBool
>-nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString)
>+nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString,
>+                                      PRInt32 aDefaultValue /* = -1 */)
> {
>   ResetIfSet();
> 
>   PRInt32 ec;
>   PRBool strict;
>   PRInt32 originalVal = StringToInteger(aString, &strict, &ec);
>   if (NS_FAILED(ec)) {
>     return PR_FALSE;
>   }
> 
>-  PRInt32 val = PR_MAX(originalVal, -1);
>+  PRInt32 val = (originalVal < 0)? aDefaultValue : originalVal;
There should be a space before '?'

>+nsAttrValue::ParsePositiveIntValue(const nsAString& aString,
>+                                   PRInt32 aDefaultValue)
>+{
>+  ResetIfSet();
>+
>+  PRInt32 ec;
>+  PRBool strict;
>+  PRInt32 originalVal = StringToInteger(aString, &strict, &ec);
>+  if (NS_FAILED(ec)) {
>+    return PR_FALSE;
>+  }
>+
>+  PRInt32 val = (originalVal <= 0)? aDefaultValue : originalVal;
Same here.

>+NS_IMPL_POSITIVE_INT_ATTR_DEFAULT_VALUE(nsHTMLSelectElement, Size, size, HasAttr(kNameSpaceID_None, nsGkAtoms::multiple)?4:1)
This looks a bit too long line.
And add space before and after '?' and ':'.
NS_IMPL_POSITIVE_INT_ATTR_DEFAULT_VALUE(nsHTMLSelectElement, Size, size,
                                        HasAttr(kNameSpaceID_None, nsGkAtoms::multiple) ? 4 : 1)


> nsHTMLSelectElement::ParseAttribute(PRInt32 aNamespaceID,
>                                     nsIAtom* aAttribute,
>                                     const nsAString& aValue,
>                                     nsAttrValue& aResult)
> {
>   if (aAttribute == nsGkAtoms::size && kNameSpaceID_None == aNamespaceID) {
>-    return aResult.ParseIntWithBounds(aValue, 0);
>+    return aResult.ParsePositiveIntValue(aValue, HasAttr(kNameSpaceID_None,
>+                                         nsGkAtoms::multiple)?4:1);
Add space before and after '?' and ':'.

Perhaps you could add a simple helper method to the .h
PRInt32 GetDefaultSize()
{
  return HasAttr(kNameSpaceID_None, nsGkAtoms::multiple) ? 4 : 1;
}
and use that in ParseAttribute and with NS_IMPL_POSITIVE_INT_ATTR_DEFAULT_VALUE.
Attachment #433863 - Flags: review?(Olli.Pettay) → review+
Attached patch Tests v0.3 (obsolete) — Splinter Review
Attachment #433861 - Attachment is obsolete: true
Attached patch Patch v0.2.1 (obsolete) — Splinter Review
Attachment #433863 - Attachment is obsolete: true
Olli, I let you re-read the patches and commit them as said on IRC.
Keywords: checkin-needed
Removing checkin-needed so that no one else doesn't push this before my
re-review.
Keywords: checkin-needed
Attachment #433991 - Flags: review?(Olli.Pettay)
> /**
>+ * A macro to implement the getter and setter for a given content
>+ * property that needs to set a non-negative unsigned integer. 

That should be "positive integer", right?

I don't see anything in the patch that updates the "size" atribute when the "multiple" attribute is set or unset.  Am I just missing it?
Actually, there is no need to update the size attribute when the multiple attribute is changed. The size IDL attribute default value depends on the multiple value but it is check dynamically.
Attached patch Patch v0.2.2 (obsolete) — Splinter Review
This patch is fixing a comment (see previous comment from bz).
Attachment #433991 - Attachment is obsolete: true
Attachment #434897 - Flags: review?(Olli.Pettay)
Attachment #433991 - Flags: review?(Olli.Pettay)
Attachment #434897 - Flags: review?(bzbarsky)
> The size IDL attribute default value depends on the multiple value but it is 
> check dynamically.

Is it?  Say I have multiple set and I set size to "-1".  We'll call ParsePositiveIntValue, which will put "4" into the nsAttrValue, unless I'm missing something.  Then I remove the multiple attr.  Then I query .size, which looks like it'll just get the value out of the nsAttrValue, hence return 4.  Unless I'm missing something?
Attached patch Tests v0.3.1 (obsolete) — Splinter Review
The tests are now checking the case mentionned by Boris in comment 27.
Attachment #433990 - Attachment is obsolete: true
Attached patch Patch v0.3 (obsolete) — Splinter Review
As usual, you were right.
I realized the default attribute wasn't really needed in the parsing function and removing it makes GetIntAttr() behaves like I want. I am surprised because I was using the default attribute in the parsing function because ParseIntWithBounds() was doing that. Anyway, tests are green and everything seems okay so I suppose it can't be completely wrong.
Attachment #434897 - Attachment is obsolete: true
Attachment #435191 - Flags: review?(bzbarsky)
Attachment #434897 - Flags: review?(bzbarsky)
Attachment #434897 - Flags: review?(Olli.Pettay)
Attachment #435191 - Flags: review?(Olli.Pettay)
Comment on attachment 435191 [details] [diff] [review]
Patch v0.3

> 
>+  /**
>+   * Helper method to get the default size.
>+   */
>+  PRInt32 GetDefaultSize() const;
>+
Perhaps
PRInt32 GetDefaultSize() const
{
  return HasAttr(kNameSpaceID_None, nsGkAtoms::multiple) ? 4 : 1;
}
Attachment #435191 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v0.3.1 (obsolete) — Splinter Review
r=smaug
Attachment #435191 - Attachment is obsolete: true
Attachment #435191 - Flags: review?(bzbarsky)
Keywords: checkin-needed
Comment on attachment 435562 [details] [diff] [review]
Patch v0.3.1


> PRBool
>-nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString)
>+nsAttrValue::ParseNonNegativeIntValue(const nsAString& aString,
>+                                      PRInt32 aDefaultValue /* = -1 */)
Actually, do we need aDefaultValue here?
Like for ParsePositiveValue(), we don't but it's working at the moment and I will provide a patch removing that unless you want it in this one.
Keywords: checkin-needed
Attached patch Patch v0.3.2 (obsolete) — Splinter Review
r=smaug
Attachment #435562 - Attachment is obsolete: true
Keywords: checkin-needed
I have been told I should ask a sr on this. So let's do it.
Removing checkin-needed keyword.
Keywords: checkin-needed
Attachment #435187 - Flags: superreview?(bzbarsky)
Attachment #435569 - Flags: superreview?(bzbarsky)
Comment on attachment 435187 [details] [diff] [review]
Tests v0.3.1

Canceling the sr request on the tests. They probably don't need to be sr.
Attachment #435187 - Flags: superreview?(bzbarsky)
Boris, do you think you will be able to sr this soon or I should ask someone else ?
Attachment #435569 - Flags: superreview?(bzbarsky) → superreview?(jst)
Comment on attachment 435569 [details] [diff] [review]
Patch v0.3.2

+   * Parse a string value into a positive integer.
+   * This method follows the rules for parsing non-negative integer from:
+   * http://dev.w3.org/html5/spec/infrastructure.html#rules-for-parsing-non-negative-integers
+   *
+   * @param aString       the string to parse
+   * @return              whether the value was valid
+   */
+  PRBool ParsePositiveIntValue(const nsAString& aString);

The URL referenced in the comment clearly states that zero is a valid value, but the implementation of this method rejects zero.

sr=jst with that comment addressed
Attachment #435569 - Flags: superreview?(jst) → superreview+
Attached patch Tests v3.2Splinter Review
r=smaug

Updated patch against the current tip.
Attachment #435187 - Attachment is obsolete: true
Attached patch Patch v3.3Splinter Review
r=smaug sr=jst

Updated patch against the current tip.
The comment is now more clear to explain it is parsing a non-negative integer which has to be greater than zero.
Attachment #435569 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 603141
Depends on: 639175
Depends on: 640636
Depends on: 640800
(In reply to comment #42)
> Updated doc to new behavior:
> https://developer.mozilla.org/en/HTML/Element/select#attr-size

This bug has actually been reverted by bug 603141. The attribute size default is now 0.
Whiteboard: [see comment 43]
OK, reverted doc likewise.
You need to log in before you can comment on or make changes to this bug.