Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify a limit in HTMLOptionsCollection.length setter. #8347

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Oct 2, 2022

This aligns with WebKit's behavior as per the discussion in #8337.


/common-dom-interfaces.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just updates the "For web developers (non-normative)" text; you need to update the normative algorithm which says

On setting, the behavior depends on whether the new value is equal to, greater than, or less than

Feel free to rewrite that as a real algorithm with steps and stuff, if that'd make things easier...

@emilio emilio force-pushed the HTMLOptionsCollection-length-setter branch from 43746da to 001c6c0 Compare October 6, 2022 23:01
@emilio
Copy link
Contributor Author

emilio commented Oct 6, 2022

Feel free to rewrite that as a real algorithm with steps and stuff, if that'd make things easier...

I think it's done, please take a look. Thanks!

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! But it seems discussions with @mfreed7 have not fully settled in the issue thread; if it's OK, let's wait until that is done?

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 7, 2022

LGTM! But it seems discussions with @mfreed7 have not fully settled in the issue thread; if it's OK, let's wait until that is done?

Thanks. My only request would be change 10,000 –> 100,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]>
@emilio emilio force-pushed the HTMLOptionsCollection-length-setter branch from 2c70ce4 to 703a99f Compare October 17, 2022 08:38
@emilio
Copy link
Contributor Author

emilio commented Oct 17, 2022

Okay, I updated the limit to 100k, as per discussion here and in the issue.

@domenic
Copy link
Member

domenic commented Oct 19, 2022

I'll merge this now!

I did notice that, by putting the return-if-over-100K line inside the "If the given value is greater than current" line, means that if I have a <select> with 101K options and I set length to length - 1, this will delete select elements. Can we be sure to add a test for that?

@domenic domenic merged commit ec4196a into whatwg:main Oct 19, 2022
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 28, 2022
As per the discussion in whatwg/html#8347

Differential Revision: https://phabricator.services.mozilla.com/D160544

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1797792
gecko-commit: bdd3a81d328efa4627647bcc54e8b09878014c92
gecko-reviewers: smaug
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 28, 2022
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 28, 2022
As per the discussion in whatwg/html#8347

Differential Revision: https://phabricator.services.mozilla.com/D160544

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1797792
gecko-commit: bdd3a81d328efa4627647bcc54e8b09878014c92
gecko-reviewers: smaug
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 31, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 10, 2022
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 11, 2022
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 11, 2022
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}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 21, 2022
…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
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 21, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants