Closed Bug 536895 Opened 14 years ago Closed 14 years ago

Setting a negative maxLength should throw an exception

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a3

People

(Reporter: ayg, Assigned: mounir)

References

(
URL
)

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 3 obsolete files)

Patch v0.2.2
9.66 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/532.6 (KHTML, like Gecko) Chrome/4.0.266.0 Safari/532.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091227 Minefield/3.7a1pre

Test case:

data:text/html,<!doctype html><script>
var input = document.createElement('input');
var caught = false;
try {
  input.maxLength = -1;
} catch (e) {
  caught = true;
}
if (caught) {
  document.write('<p>Exception thrown, correct');
} else {
  document.write('<p>No exception thrown, bug');
}</script>

WebKit throws an exception, Opera and Firefox do not.  Didn't test IE.

Spec references:

<http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#dom-input-maxlength>

says "The maxLength IDL attribute must reflect the maxlength content attribute, limited to only non-negative numbers."  The last phrase is a link to

<http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#limited-to-only-non-negative-numbers>

which says "On setting, if the value is negative, the user agent must fire an INDEX_SIZE_ERR exception."

This is a follow-up to bug 535043.

Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → mounir.lamouri
URL: http://dev.w3.org/html5/spec/infrastr...
Severity: minor → enhancement
Status: NEW → ASSIGNED
Keywords: html5
Version: unspecified → Trunk
Attached patch Patch v0.1 (obsolete) — — Splinter Review 
This patch is following patch from bug 536891 by throwing an exception if the maxLength is set to a negative value from the DOM.
Attachment #428420 - Flags: review?(Olli.Pettay)
Would it make sense to add some helper macro to nsGenericHTMLElement
Maybe NS_IMPL_UNSIGNED_INT_ATTR or NS_IMPL_NON_NEGATIVE_INT_ATTR?
The only problem I see with a macro is the exception has to be thrown when the attribute is represented as a long in the IDL and should be non negative (like maxLength). If the attribute is represented an unsigned long in the IDL, when trying to set a non negative attribute, the behavior is different.

I think NS_IMPL_NON_NEGATIVE_INT_ATTR would be appropriate if a macro NS_IMPL_NON_NEGATIVE_UNSIGNED_INT_ATTR is used in the future for the other case. Actually, ParseNonNegativeUnsignedIntValue will also have to be created.

Does it make sense for you ?
Comment on attachment 428420 [details] [diff] [review]
Patch v0.1

I assume there will be a new patch coming :)
Attachment #428420 - Flags: review?(Olli.Pettay)
> Actually, ParseNonNegativeUnsignedIntValue will also have to be created.
That is something for a different bug and all. If it will be needed, then it will be.
Attached patch Patch v0.2 (obsolete) — — Splinter Review 
Creating and using NS_IMPL_NON_NEGATIVE_INT_ATTR macro.
Attachment #428420 - Attachment is obsolete: true
Attachment #428441 - Flags: review?(Olli.Pettay)
Attachment #428441 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 428441 [details] [diff] [review]
Patch v0.2

>+ * A macro to implement the getter and setter for a given content
>+ * property that needs to set a non-negative integer. The method
>+ * uses the generic GetAttr and SetAttr methods. This macro is much
>+ * like the NS_IMPL_INT_ATTR macro except we throw an exception if
>+ * the set value is negative.
>+ */
>+#define NS_IMPL_NON_NEGATIVE_INT_ATTR(_class, _method, _atom)                    \
Some extra spaces before \

(No need to ask re-review)
Blocks: 344614
Attached patch Patch v0.2.1 (obsolete) — — Splinter Review 
jst, may you sr this patch ? It is following patch from bug 536891.
Attachment #428441 - Attachment is obsolete: true
Attachment #428738 - Flags: superreview?
Attachment #428738 - Flags: superreview? → superreview?(jst)
Keywords: dev-doc-needed
Attachment #428738 - Flags: superreview?(jst) → superreview+
Marking checkin-needed with r=Olli.Pettay, sr=jst
Keywords: checkin-needed
this patch has bitrotted
Keywords: checkin-needed
After pushing Bug 536891, this should apply with some fuzz
Keywords: checkin-needed
Attached patch Patch v0.2.2 — — Splinter Review 
I've just generated a new patch. It should be easier to apply.
Please, apply the patch from bug 536891 before.
Attachment #428738 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/1a99610b2502
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a3
OK, in combination with bug 536891, is the new behavior that setting maxlength to a negative number sets it to unspecified length and throws an exception both, or...?
If you do :
input.maxLength = -2;
it will throw an exception and the maxLength will not change.

If you do:
input.setAttribute('maxLength', -10);
it will work but when you will get it with:
input.maxLength
you will get -1 (as unspecified).
You need to log in before you can comment on or make changes to this bug.