HTMLOptionsCollection length setter should specify what happens when you go over the option limit. #8337
Comments
|
The comment in the blink code doesn't make sense to me: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_select_element.cc;l=93;drc=a432cd59d51281057ba2a2673ca645a9600bb927 |
|
I found when Blink changed: https://source.chromium.org/chromium/chromium/src/+/a736194f01cfbb2688134cdfdf66f85996719ee4
I guess the point is it'd be weird if you had |
|
In Gecko and WebKit it wouldn't truncate, it just won't do anything. |
This aligns with WebKit's behavior as per the discussion in whatwg#8337.
|
#8347 is the PR for this. Tests are up in the mozilla bug (a volunteer contributed the patch). |
|
I'm ok with the spec and behavior having a reasonable limit, much smaller than INT_MAX. However, I don't think we should start throwing when that limit is exceeded, since that could break things. I think a console warning and a refusal to set lengths > SPEC_LIMIT should hopefully be enough to keep people from having problems, right?
In Chromium also, I think: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_select_element.cc;l=485;drc=a2a75509d6058498d6e00ca225455a53b1006188 Given the comment that options lists of 40k+ were observed (5 years ago), perhaps we should choose a limit like 100,000? |
This aligns with WebKit's behavior as per the discussion in whatwg#8337.
|
I don't feel too strongly about the precise limit. 10000 matches two of the three engines and I've never seen a bug about people requesting more, so I don't think we need to raise it, but I wouldn't be opposed to do so... How strongly do you feel about it @mfreed7? |
|
Ok as per #8347 (comment), raising it to 100000 sounds good to me. @rniwa @smfr @nt1m, would WebKit be fine with raising it? It'd be just adding one zero here: https://searchfox.org/wubkat/rev/e8e9be7353991aee7df45aa62f0e0148f16e6f9f/Source/WebCore/html/HTMLSelectElement.cpp#69 |
I’m just worried about compat, given that it’s lower than before and the comment in our codebase suggesting problems with 40,000. |
This aligns with WebKit's behavior, but bumps the limit with 100k, as per the discussion in whatwg#8337. Co-authored-by: Domenic Denicola <[email protected]>
Update the const kMaxListItems to be 100,000 in HTMLOptionsCollections to reflect updated spec. Also, this max should only be used when new length is greater than current length. [1] https://html.spec.whatwg.org/#dom-htmloptionscollection-length [2] whatwg/html#8337 [3] whatwg/html#8347 Change-Id: I7ff54e9cfdcb2eb014ad508485eda6908308314b Fixed: 1370370
Update the const kMaxListItems to be 100,000 in HTMLOptionsCollections to reflect updated spec. Also, this max should only be used when new length is greater than current length. [1] https://html.spec.whatwg.org/#dom-htmloptionscollection-length [2] whatwg/html#8337 [3] whatwg/html#8347 Change-Id: I7ff54e9cfdcb2eb014ad508485eda6908308314b Fixed: 1370370 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4015681 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Di Zhang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1070388}
Update the const kMaxListItems to be 100,000 in HTMLOptionsCollections to reflect updated spec. Also, this max should only be used when new length is greater than current length. [1] https://html.spec.whatwg.org/#dom-htmloptionscollection-length [2] whatwg/html#8337 [3] whatwg/html#8347 Change-Id: I7ff54e9cfdcb2eb014ad508485eda6908308314b Fixed: 1370370 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4015681 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Di Zhang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1070388}
…ection.length setter, a=testonly Automatic update from web-platform-tests Restore 100,000 limit in HTMLOptionsCollection.length setter Update the const kMaxListItems to be 100,000 in HTMLOptionsCollections to reflect updated spec. Also, this max should only be used when new length is greater than current length. [1] https://html.spec.whatwg.org/#dom-htmloptionscollection-length [2] whatwg/html#8337 [3] whatwg/html#8347 Change-Id: I7ff54e9cfdcb2eb014ad508485eda6908308314b Fixed: 1370370 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4015681 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Di Zhang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1070388} -- wpt-commits: 893a5f3fa49a459b94526de606e89856dcbb5dae wpt-pr: 36917
…ection.length setter, a=testonly Automatic update from web-platform-tests Restore 100,000 limit in HTMLOptionsCollection.length setter Update the const kMaxListItems to be 100,000 in HTMLOptionsCollections to reflect updated spec. Also, this max should only be used when new length is greater than current length. [1] https://html.spec.whatwg.org/#dom-htmloptionscollection-length [2] whatwg/html#8337 [3] whatwg/html#8347 Change-Id: I7ff54e9cfdcb2eb014ad508485eda6908308314b Fixed: 1370370 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4015681 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Di Zhang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1070388} -- wpt-commits: 893a5f3fa49a459b94526de606e89856dcbb5dae wpt-pr: 36917
https://bugs.webkit.org/show_bug.cgi?id=252983 Reviewed by Chris Dumez. This patch is to align WebKit with Blink / Chromium and Gecko / Firefox. Merge - https://chromium.googlesource.com/chromium/src/+/f27e6ea87ecf211c8b8644813422a9e7f19cd1cc This patch updates 'maxSelectItems' to new value of 100,000 to reflect update in spec. Further, it is updated to be only used when new length is greater than current length. Additionally, add comments to reflect the details as needed. Web-Spec: https://html.spec.whatwg.org/#dom-htmloptionscollection-length Issue: whatwg/html#8337 * Source/WebCore/html/HTMLSelectElement.cpp: (maxSelectItems): Update constant value (HTMLSelectElement::setItem): Remove '=' and update comments and console message (HTMLSelectElement::setLength): Add comment and update console message * LayoutTests/imported/w3c/web-platform-tests/html/select/options-length-too-large.html: Add Test Case * LayoutTests/imported/w3c/web-platform-tests/html/select/options-length-too-large-expected.txt: Add Test Case Expectation * LayoutTests/fast/forms/select-max-length-expected.txt: Rebaselined * LayoutTests/fast/dom/HTMLSelectElement/select-selectedIndex-multiple-expected.txt: Rebaselined * LayoutTests/fast/dom/HTMLSelectElement/select-selectedIndex-expected.txt: Rebaselined Canonical link: https://commits.webkit.org/260896@main
https://html.spec.whatwg.org/#dom-htmloptionscollection-length has no limit on the number of options. That means that
select.options.length = -1would create INT32_MAX options, which is not great.All browsers have an implementation limit to avoid blowing up with a snippet like the one below. WebKit and Gecko agree on that limit being 10000, but Blink seems to have changed that limit for some reason?
Test-case is:
The behavior where the limit is surpassed is undefined: Gecko throws, WebKit/Blink just warn to the console. We can probably standardize on the WebKit / Blink behavior (just do nothing), but the spec should probably explicitly mention this.
I'm also curious about why the limit was changed in Blink?
cc @mfreed7
The text was updated successfully, but these errors were encountered: