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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ import DOMException from '../../exception/DOMException'; | |
import HTMLCollection from '../element/HTMLCollection'; | ||
import IHTMLOptGroupElement from '../html-opt-group-element/IHTMLOptGroupElement'; | ||
import IHTMLSelectElement from '../html-select-element/IHTMLSelectElement'; | ||
import HTMLOptionElement from './HTMLOptionElement'; | ||
import IHTMLOptionElement from './IHTMLOptionElement'; | ||
import IHTMLOptionsCollection from './IHTMLOptionsCollection'; | ||
|
||
|
@@ -17,6 +16,7 @@ export default class HTMLOptionsCollection | |
implements IHTMLOptionsCollection | ||
{ | ||
private _selectElement: IHTMLSelectElement; | ||
private _selectedIndex = -1; | ||
|
||
/** | ||
* | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return i; | ||
} | ||
} | ||
|
||
return -1; | ||
return this._selectedIndex; | ||
} | ||
|
||
/** | ||
|
@@ -50,10 +43,11 @@ export default class HTMLOptionsCollection | |
* @param selectedIndex SelectedIndex. | ||
*/ | ||
public set selectedIndex(selectedIndex: number) { | ||
for (let i = 0; i < this.length; i++) { | ||
const item = this[i]; | ||
if (item instanceof HTMLOptionElement) { | ||
this[i].selected = i === selectedIndex; | ||
if (typeof selectedIndex === 'number' && !isNaN(selectedIndex)) { | ||
if (selectedIndex >= 0 && selectedIndex < this.length) { | ||
this._selectedIndex = selectedIndex; | ||
} else { | ||
this._selectedIndex = -1; | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import IWindow from '../../../src/window/IWindow'; | |
import IDocument from '../../../src/nodes/document/IDocument'; | ||
import HTMLSelectElement from '../../../src/nodes/html-select-element/HTMLSelectElement'; | ||
import HTMLOptionElement from '../../../src/nodes/html-option-element/HTMLOptionElement'; | ||
import { DOMException } from '../../../src'; | ||
import DOMException from '../../../src/exception/DOMException'; | ||
|
||
describe('HTMLOptionsCollection', () => { | ||
let window: IWindow; | ||
|
@@ -18,57 +18,51 @@ describe('HTMLOptionsCollection', () => { | |
jest.restoreAllMocks(); | ||
}); | ||
|
||
describe('get selectedindex()', () => { | ||
it('Returns the index of the first option element in the list of options in tree order that has its selectedness set to true.', () => { | ||
const select = <HTMLSelectElement>document.createElement('select'); | ||
const option1 = <HTMLOptionElement>document.createElement('option'); | ||
const option2 = <HTMLOptionElement>document.createElement('option'); | ||
option1.selected = true; | ||
option1.value = 'option1'; | ||
option2.value = 'option2'; | ||
select.appendChild(option1); | ||
select.appendChild(option2); | ||
|
||
expect(select.options.selectedIndex).toBe(0); | ||
}); | ||
|
||
describe('get selectedIndex()', () => { | ||
it('Returns -1 if there are no options.', () => { | ||
const select = <HTMLSelectElement>document.createElement('select'); | ||
expect(select.options.selectedIndex).toBe(-1); | ||
}); | ||
|
||
it('Returns -1 if no option is selected.', () => { | ||
it('Returns 0 by default.', () => { | ||
const select = <HTMLSelectElement>document.createElement('select'); | ||
const option1 = <HTMLOptionElement>document.createElement('option'); | ||
const option2 = <HTMLOptionElement>document.createElement('option'); | ||
|
||
option1.value = 'option1'; | ||
option2.value = 'option2'; | ||
|
||
select.appendChild(option1); | ||
select.appendChild(option2); | ||
|
||
expect(select.options.selectedIndex).toBe(-1); | ||
expect(select.options.selectedIndex).toBe(0); | ||
}); | ||
}); | ||
|
||
describe('set selectedindex()', () => { | ||
describe('set selectedIndex()', () => { | ||
it('Updates option.selected', () => { | ||
const select = <HTMLSelectElement>document.createElement('select'); | ||
select.appendChild(document.createElement('option')); | ||
select.appendChild(document.createElement('option')); | ||
document.body.appendChild(select); | ||
const option1 = <HTMLOptionElement>document.createElement('option'); | ||
const option2 = <HTMLOptionElement>document.createElement('option'); | ||
|
||
expect(option1.selected).toBe(false); | ||
expect(option2.selected).toBe(false); | ||
|
||
select.appendChild(option1); | ||
select.appendChild(option2); | ||
|
||
expect((<HTMLOptionElement>select.options[0]).selected).toBe(false); | ||
expect((<HTMLOptionElement>select.options[1]).selected).toBe(false); | ||
expect(option1.selected).toBe(true); | ||
expect(option2.selected).toBe(false); | ||
|
||
select.options.selectedIndex = 1; | ||
|
||
expect((<HTMLOptionElement>select.options[0]).selected).toBe(false); | ||
expect((<HTMLOptionElement>select.options[1]).selected).toBe(true); | ||
expect(option1.selected).toBe(false); | ||
expect(option2.selected).toBe(true); | ||
|
||
select.options.selectedIndex = -1; | ||
|
||
expect((<HTMLOptionElement>select.options[0]).selected).toBe(false); | ||
expect((<HTMLOptionElement>select.options[1]).selected).toBe(false); | ||
expect(option1.selected).toBe(false); | ||
expect(option2.selected).toBe(false); | ||
}); | ||
}); | ||
|
||
|
@@ -134,16 +128,21 @@ describe('HTMLOptionsCollection', () => { | |
const select = <HTMLSelectElement>document.createElement('select'); | ||
const option = document.createElement('option'); | ||
const option2 = document.createElement('option'); | ||
|
||
expect(select.options.selectedIndex).toBe(-1); | ||
|
||
select.appendChild(option); | ||
select.appendChild(option2); | ||
document.body.appendChild(select); | ||
expect(select.options.selectedIndex).toBe(-1); | ||
|
||
expect(select.options.selectedIndex).toBe(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 |
||
|
||
select.options.selectedIndex = 1; | ||
expect(select.options.selectedIndex).toBe(1); | ||
|
||
// No option is selected after removing the selected option | ||
select.options.remove(1); | ||
expect(select.options.selectedIndex).toBe(0); | ||
|
||
select.options.remove(0); | ||
expect(select.options.selectedIndex).toBe(-1); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 theHTMLOptionElement
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
There was a problem hiding this comment.
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