| Summary: | <input> maxLength is clamped to 524288 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aryeh Gregor <ayg> | ||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Minor | CC: | ahmad.saleem792, akeerthi, ap, aroben, cdumez, dglazkov, fry.kun, mike, mounir, tabatkins, tkent, webkit.review.bot, xqb748 | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| URL: | http://aryeh.name/tests/reflection.html | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=154906 | ||||||||||
| Bug Depends on: | 14388 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
FWIW, we clamp actual maximum length in TextFieldInputType::sanitizeValue(), too. Created attachment 108972 [details]
Proposed Patch
Comment on attachment 108972 [details] Proposed Patch Attachment 108972 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9877622 New failing tests: fast/forms/input-maxlength.html fast/forms/input-text-paste-maxlength.html fast/forms/paste-multiline-text-input.html fast/forms/input-implicit-length-limit.html Comment on attachment 108972 [details]
Proposed Patch
Canceling the review since some test cases are failing.Will upload a modified patch after resolving the conflicts.
Created attachment 109255 [details]
Patch
Modified Patch.Not setting the r? flag now, would prefer to see it passes tests for all ports first.
Comment on attachment 109255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109255&action=review What's the purpose of this patch? Do you want to remove the internal hard limit, 524288, or want not to expose the internal hard limit via HTMLInputELement::maxLength? > Source/WebCore/html/TextFieldInputType.cpp:304 > - return limitLength(proposedValue.removeCharacters(isASCIILineBreak), HTMLInputElement::maximumLength); > + return limitLength(proposedValue.removeCharacters(isASCIILineBreak), element()->maxLength()); No, this is wrong. We should not clamp the value by maxLength() here. > LayoutTests/fast/forms/input-appearance-maxlength.html:18 > -if (input.value != "12345") { > +if (input.value == "12345") { > testPassed('Maxlength shouldn\'t work for default value.\n'); This is wrong. We must not change this. > LayoutTests/fast/forms/input-text-maxlength.html:20 > -<input type="text" size="5" value="12x̲̅45" maxlength="4" id="input7"> > -<input type="text" size="5" maxlength="4" value="12x̲̅45" id="input8"> > -<input type="text" id="j" size="5" maxlength="4" value="123"> > -<input type="text" id="i" size="5" maxlength="4" value="123"> > +<input type="text" size="5" value="12x̲̅45" maxlength="10" id="input7"> > +<input type="text" size="5" maxlength="10" value="12x̲̅45" id="input8"> > +<input type="text" id="j" size="5" maxlength="10" value="123"> > +<input type="text" id="i" size="5" maxlength="10" value="123"> We should not change this. > LayoutTests/fast/forms/input-text-maxlength.html:38 > -shouldBe('domValueOf("input1")', '"12345"'); > +shouldBe('domValueOf("input1")', '"1234"'); We should not change this file. > LayoutTests/fast/forms/input-text-paste-maxlength.html:17 > -<input type="text" id="i" size="5" maxlength="4"> > +<input type="text" id="i" size="5" maxlength="20"> We should not change this file. > LayoutTests/fast/forms/script-tests/ValidityState-tooLong-input.js:13 > -shouldBe('input.value.length', '5'); > +shouldBe('input.value.length', '3'); We should not change this file. Thanks for the review. (In reply to comment #6) > (From update of attachment 109255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109255&action=review > > What's the purpose of this patch? Do you want to remove the internal hard limit, 524288, or want not to expose the internal hard limit via HTMLInputELement::maxLength? > Do you want to remove the internal hard limit, 524288 ... yes. > > > Source/WebCore/html/TextFieldInputType.cpp:304 > > - return limitLength(proposedValue.removeCharacters(isASCIILineBreak), HTMLInputElement::maximumLength); > > + return limitLength(proposedValue.removeCharacters(isASCIILineBreak), element()->maxLength()); > > No, this is wrong. We should not clamp the value by maxLength() here. > This was required if the maxlength is set with a value which is greater than the current limit 524288. > > LayoutTests/fast/forms/input-appearance-maxlength.html:18 > > -if (input.value != "12345") { > > +if (input.value == "12345") { > > testPassed('Maxlength shouldn\'t work for default value.\n'); > > This is wrong. We must not change this. Ok.I will try to find a better way. > > > LayoutTests/fast/forms/input-text-maxlength.html:20 > > -<input type="text" size="5" value="12x̲̅45" maxlength="4" id="input7"> > > -<input type="text" size="5" maxlength="4" value="12x̲̅45" id="input8"> > > -<input type="text" id="j" size="5" maxlength="4" value="123"> > > -<input type="text" id="i" size="5" maxlength="4" value="123"> > > +<input type="text" size="5" value="12x̲̅45" maxlength="10" id="input7"> > > +<input type="text" size="5" maxlength="10" value="12x̲̅45" id="input8"> > > +<input type="text" id="j" size="5" maxlength="10" value="123"> > > +<input type="text" id="i" size="5" maxlength="10" value="123"> > > We should not change this. Ok. > > > LayoutTests/fast/forms/input-text-maxlength.html:38 > > -shouldBe('domValueOf("input1")', '"12345"'); > > +shouldBe('domValueOf("input1")', '"1234"'); > > We should not change this file. ok. > > > LayoutTests/fast/forms/input-text-paste-maxlength.html:17 > > -<input type="text" id="i" size="5" maxlength="4"> > > +<input type="text" id="i" size="5" maxlength="20"> > > We should not change this file. ok. > > > LayoutTests/fast/forms/script-tests/ValidityState-tooLong-input.js:13 > > -shouldBe('input.value.length', '5'); > > +shouldBe('input.value.length', '3'); > > We should not change this file. ok. (In reply to comment #7) > Thanks for the review. > > (In reply to comment #6) > > (From update of attachment 109255 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=109255&action=review > > > > What's the purpose of this patch? Do you want to remove the internal hard limit, 524288, or want not to expose the internal hard limit via HTMLInputELement::maxLength? > > > > Do you want to remove the internal hard limit, 524288 ... yes. 524,288 was introduced in http://bugs.webkit.org/show_bug.cgi?id=14388 (It seems the URL in the HTMLInputElmenet.cpp is wrong.) Would you explain a reason we can remove the hard limit please? Did you confirmed it was not needed anymore? (In reply to comment #8) > (In reply to comment #7) > > Thanks for the review. > > > > (In reply to comment #6) > > > (From update of attachment 109255 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=109255&action=review > > > > > > What's the purpose of this patch? Do you want to remove the internal hard limit, 524288, or want not to expose the internal hard limit via HTMLInputELement::maxLength? > > > > > > > Do you want to remove the internal hard limit, 524288 ... yes. > > 524,288 was introduced in http://bugs.webkit.org/show_bug.cgi?id=14388 > (It seems the URL in the HTMLInputElmenet.cpp is wrong.) > Would you explain a reason we can remove the hard limit please? Did you confirmed it was not needed anymore? I think firefox and IE don't have this limit. May be Adam Roben can provide some thought whether we still need this restriction. The URL in the comment is: https://bugs.webkit.org/show_bugs.cgi?id=14536 but should have been: https://bugs.webkit.org/show_bug.cgi?id=14536 As that bug explains, there was a performance issue with text fields that contained very long strings. Supposedly that issue has been fixed, so we should retest and adjust the limit as appropriate. Just hit this bug on my internal website -- trying to submit a text value that's 900k long.... I turned the test case from Comment 01 in JS Fiddle below: https://jsfiddle.net/qcaxmpy5/show I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 and all browsers (Chrome Canary 105 and Firefox Nightly 103) including Safari shows same expected output of: -1 0 1000000 I think this got fixed along the line and it can be marked as "RESOLVED CONFIGURATION CHANGED". Thanks! Further, using Wayback Archive, I was able to retrieve old test cases (of URL) and change into following JSFiddle: https://jsfiddle.net/pzkj8xbf/show It shows some failures but those could be due to spec changes since 2010 onward since no other browser achieve 100% pass rate. Safari 15.5 - Passed: 17447 ( 96.8 %). Failed: 584 Firefox Nightly 103 - Passed: 17302 ( 96.6 %). Failed: 617 Chrome Canary 105 - Passed: 17435 ( 96.7 %). Failed: 596 Just sharing it for keep sake. Thanks! There is some subtlety here, and I'm not sure if this can be called fully fixed. In bug 154906, we made the property return -1, but kept 524288 as a maximum input value length internally for performance reasons. Adding more than as many characters silently clamps the value. Chrome and Firefox don't have such a hidden limit. Created attachment 460403 [details]
test for actual value length
|
Test case: <!doctype html> <script> var el = document.createElement("input"); var out = el.maxLength; el.setAttribute("maxlength", "0"); out += " " + el.maxLength; el.setAttribute("maxlength", "1000000"); out += " " + el.maxLength; alert(out); </script> Recentish Firefox nightly and Opera 10.60 alert "-1 0 1000000", per spec. Chrome dev on Linux and Safari 5 on XP alert "524288 524288 524288". IE8 alerts "2147483647 0 1000000". The spec says that this is a long limited to only non-negative numbers, which is defined as follows: """ If a reflecting IDL attribute is a signed integer type (long) that is limited to only non-negative numbers then, on getting, the content attribute must be parsed according to the rules for parsing non-negative integers, and if that is successful, and the value is in the range of the IDL attribute's type, the resulting value must be returned. 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. On setting, if the value is negative, the user agent must fire an INDEX_SIZE_ERR exception. Otherwise, the given value must be converted to the shortest possible string representing the number as a valid non-negative integer and then that string must be used as the new content attribute value. """ http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes