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

#635@patch: Multiples fixes regarding how HTMLSelectElement.selectedI… #636

Conversation

capricorn86
Copy link
Owner

…ndex is handled.

@capricorn86 capricorn86 self-assigned this Oct 24, 2022
@capricorn86 capricorn86 merged commit 76c09bb into master Oct 24, 2022
@capricorn86 capricorn86 deleted the task/635-setting-an-invalid-value-to-htmlselectelementselectedindex-causes-a-crash branch October 24, 2022 23:48
document.body.appendChild(select);
expect(select.options.selectedIndex).toBe(-1);

expect(select.options.selectedIndex).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This has me confused: the spec says it should be -1 in this situation (ref), but the browser says 0 😖. Did you find a place in the spec indicating it should actually be 0, or are you changing this purely based on observations of how browsers actually act?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I found it by testing the behavior in the browser, but there is a spec for it.

Spec:
https://html.spec.whatwg.org/multipage/form-elements.html#selectedness-setting-algorithm

Copy link
Contributor

@IGx89 IGx89 Oct 25, 2022

Choose a reason for hiding this comment

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

Ahh, thanks a lot! So it looks like -1 is only a valid initial value if every option is disabled (or there are no options)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes it seems like that. I will try to fix it so that it works according to spec now. Wish me luck 😅

@@ -34,14 +34,7 @@ export default class HTMLOptionsCollection
* @returns SelectedIndex.
*/
public get selectedIndex(): number {
for (let i = 0; i < this.length; i++) {
const item = this[i];
if (item instanceof HTMLOptionElement && item.selected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned that by removing this behavior you're going to break some things. If the HTML was the following:

<select>
	<option>1</option>
	<option selected>2</option>
</select>

The browser says selectedIndex is 1, but with this change it'll wrongly be 0, won't it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@IGx89 Yes you are right. I will fix it now directly. I wanted to correct the selectedIndex behavior as the current behavior does not follow the spec or the browser behavior, but I missed the "selected" attribute.

if (!selected) {
this.removeAttributeNS(null, 'selected');
} else {
this.setAttributeNS(null, 'selected', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional that you're not adding/removing the selected DOM attribute anymore? Isn't that necessary? This'll make it harder to implement multi-selects in the future too I'm guessing.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Changing the selectedIndex property should not modify the attribute value. It should set the HTMLOptionElement to be dirty and then it overrides the attribute.

It is not entirely correct now as I missed handling the attribute, sorry about that.

https://html.spec.whatwg.org/multipage/form-elements.html#dom-select-selectedindex
https://html.spec.whatwg.org/multipage/form-elements.html#concept-option-dirtiness

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I assumed setting "selectedness" means setting the selected attribute, which does seem wrong. I can't find anything on those links that specifically details what happens when an option's selectedness is set, which is strange, but what you're saying definitely makes sense

@capricorn86
Copy link
Owner Author

@IGx89 I have released a fix now. Hopefully everything regarding "selectedness" is fixed now. Thanks for reporting the problem 🙂

Here is the pull request:
#638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting an invalid value to HTMLSelectElement.selectedIndex causes a crash
2 participants