Closed
Bug 551846
Opened 14 years ago
Closed 14 years ago
select element size attribute default should be 4 when multiple attribute is present
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
7.48 KB,
patch
|
Details | Diff | Splinter Review | |
7.39 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
If we do this we can probably simplify a bunch of code in CSS frame constructor and forms.css (and resolve some old bugs).
Comment 2•14 years ago
|
||
At least assuming the getAttribute() values are affected. If they're not we can only simplify frame constructor code.
Assignee | ||
Comment 3•14 years ago
|
||
As I understand it, getAttribute should not be affected but only the reflecting IDL attribute.
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #432589 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #432589 -
Flags: review?(bzbarsky) → review?(ehsan.akhgari)
Assignee | ||
Comment 6•14 years ago
|
||
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)
Comment 7•14 years ago
|
||
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.
Comment 8•14 years ago
|
||
ehsan certainly knows form control code nowadays. And I can review this too.
Assignee | ||
Comment 9•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #432589 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
Attachment #432590 -
Flags: review?(Olli.Pettay)
Comment 10•14 years ago
|
||
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 11•14 years ago
|
||
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 12•14 years ago
|
||
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 13•14 years ago
|
||
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)
Assignee | ||
Comment 14•14 years ago
|
||
(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.
Comment 15•14 years ago
|
||
(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.
Assignee | ||
Comment 16•14 years ago
|
||
I've added the requested tests and a test to check the size value after removeAttribute('size').
Attachment #432589 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
Every requested changes have been applied in this patch.
Attachment #432590 -
Attachment is obsolete: true
Attachment #433863 -
Flags: review?(Olli.Pettay)
Comment 18•14 years ago
|
||
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 19•14 years ago
|
||
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+
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #433861 -
Attachment is obsolete: true
Assignee | ||
Comment 21•14 years ago
|
||
Attachment #433863 -
Attachment is obsolete: true
Assignee | ||
Comment 22•14 years ago
|
||
Olli, I let you re-read the patches and commit them as said on IRC.
Keywords: checkin-needed
Comment 23•14 years ago
|
||
Removing checkin-needed so that no one else doesn't push this before my re-review.
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Attachment #433991 -
Flags: review?(Olli.Pettay)
Comment 24•14 years ago
|
||
> /**
>+ * 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?
Assignee | ||
Comment 25•14 years ago
|
||
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.
Assignee | ||
Comment 26•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #434897 -
Flags: review?(bzbarsky)
Comment 27•14 years ago
|
||
> 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?
Assignee | ||
Comment 28•14 years ago
|
||
The tests are now checking the case mentionned by Boris in comment 27.
Attachment #433990 -
Attachment is obsolete: true
Assignee | ||
Comment 29•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #435191 -
Flags: review?(Olli.Pettay)
Comment 30•14 years ago
|
||
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+
Assignee | ||
Comment 31•14 years ago
|
||
r=smaug
Attachment #435191 -
Attachment is obsolete: true
Attachment #435191 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 32•14 years ago
|
||
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?
Assignee | ||
Comment 33•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•14 years ago
|
||
I have been told I should ask a sr on this. So let's do it. Removing checkin-needed keyword.
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Attachment #435187 -
Flags: superreview?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #435569 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 36•14 years ago
|
||
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)
Assignee | ||
Comment 37•14 years ago
|
||
Boris, do you think you will be able to sr this soon or I should ask someone else ?
Assignee | ||
Updated•14 years ago
|
Attachment #435569 -
Flags: superreview?(bzbarsky) → superreview?(jst)
Comment 38•14 years ago
|
||
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+
Assignee | ||
Comment 39•14 years ago
|
||
r=smaug Updated patch against the current tip.
Attachment #435187 -
Attachment is obsolete: true
Assignee | ||
Comment 40•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/680805a771ab http://hg.mozilla.org/mozilla-central/rev/9ee12ee8bf8e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 42•13 years ago
|
||
Updated doc to new behavior: https://developer.mozilla.org/en/HTML/Element/select#attr-size
Assignee | ||
Comment 43•13 years ago
|
||
(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]
Comment 44•13 years ago
|
||
OK, reverted doc likewise.
You need to log in
before you can comment on or make changes to this bug.
Description
•