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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Oct 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 35 additions & 5 deletions packages/happy-dom/src/nodes/html-option-element/HTMLOptionElement.ts
View file
Open in desktop
@@ -1,6 +1,7 @@
import HTMLElement from '../html-element/HTMLElement';
import IHTMLElement from '../html-element/IHTMLElement';
import IHTMLFormElement from '../html-form-element/IHTMLFormElement';
import IHTMLSelectElement from '../html-select-element/IHTMLSelectElement';
import IHTMLOptionElement from './IHTMLOptionElement';

/**
Expand Down Expand Up @@ -58,7 +59,20 @@ export default class HTMLOptionElement extends HTMLElement implements IHTMLOptio
* @returns Selected.
*/
public get selected(): boolean {
return this.getAttributeNS(null, 'selected') !== null;
const parentNode = <IHTMLSelectElement>this.parentNode;

if (parentNode?.tagName === 'SELECT') {
let index = -1;
for (let i = 0; i < parentNode.options.length; i++) {
if (parentNode.options[i] === this) {
index = i;
break;
}
}
return index !== -1 && parentNode.options.selectedIndex === index;
}

return false;
}

/**
Expand All @@ -67,10 +81,26 @@ export default class HTMLOptionElement extends HTMLElement implements IHTMLOptio
* @param selected Selected.
*/
public set selected(selected: boolean) {
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

const parentNode = <IHTMLSelectElement>this.parentNode;
if (parentNode?.tagName === 'SELECT') {
if (selected) {
let index = -1;

for (let i = 0; i < parentNode.options.length; i++) {
if (parentNode.options[i] === this) {
index = i;
break;
}
}

if (index !== -1) {
parentNode.options.selectedIndex = index;
}
} else if (parentNode.options.length) {
parentNode.options.selectedIndex = 0;
} else {
parentNode.options.selectedIndex = -1;
}
}
}

Expand Down
20 changes: 7 additions & 13 deletions packages/happy-dom/src/nodes/html-option-element/HTMLOptionsCollection.ts
View file
Open in desktop
Expand Up @@ -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';

Expand All @@ -17,6 +16,7 @@ export default class HTMLOptionsCollection
implements IHTMLOptionsCollection
{
private _selectElement: IHTMLSelectElement;
private _selectedIndex = -1;

/**
*
Expand All @@ -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.

return i;
}
}

return -1;
return this._selectedIndex;
}

/**
Expand All @@ -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;
}
}
}
Expand Down
26 changes: 17 additions & 9 deletions packages/happy-dom/src/nodes/html-select-element/HTMLSelectElement.ts
View file
Open in desktop
@@ -1,5 +1,3 @@
import DOMException from '../../exception/DOMException';
import DOMExceptionNameEnum from '../../exception/DOMExceptionNameEnum';
import HTMLElement from '../html-element/HTMLElement';
import IHTMLElement from '../html-element/IHTMLElement';
import IHTMLFormElement from '../html-form-element/IHTMLFormElement';
Expand Down Expand Up @@ -204,13 +202,6 @@ export default class HTMLSelectElement extends HTMLElement implements IHTMLSelec
* @param value Value.
*/
public set selectedIndex(value: number) {
if (value > this.options.length - 1 || value < -1) {
throw new DOMException(
'Select elements selected index must be valid',
DOMExceptionNameEnum.indexSizeError
);
}

this.options.selectedIndex = value;
}

Expand Down Expand Up @@ -296,6 +287,10 @@ export default class HTMLSelectElement extends HTMLElement implements IHTMLSelec

if (element.tagName === 'OPTION' || element.tagName === 'OPTGROUP') {
this.options.push(<IHTMLOptionElement | IHTMLOptGroupElement>element);

if (this.options.length === 1) {
this.options.selectedIndex = 0;
}
}

this._updateIndexProperties(previousLength, this.options.length);
Expand Down Expand Up @@ -337,6 +332,10 @@ export default class HTMLSelectElement extends HTMLElement implements IHTMLSelec
} else {
this.options.push(<IHTMLOptionElement | IHTMLOptGroupElement>newElement);
}

if (this.options.length === 1) {
this.options.selectedIndex = 0;
}
}

this._updateIndexProperties(previousLength, this.options.length);
Expand All @@ -355,9 +354,18 @@ export default class HTMLSelectElement extends HTMLElement implements IHTMLSelec

if (element.tagName === 'OPTION' || element.tagName === 'OPTION') {
const index = this.options.indexOf(<IHTMLOptionElement | IHTMLOptGroupElement>node);

if (index !== -1) {
this.options.splice(index, 1);
}

if (this.options.selectedIndex >= this.options.length) {
this.options.selectedIndex = this.options.length - 1;
}

if (!this.options.length) {
this.options.selectedIndex = -1;
}
}

this._updateIndexProperties(previousLength, this.options.length);
Expand Down
91 changes: 75 additions & 16 deletions packages/happy-dom/test/nodes/html-option-element/HTMLOptionElement.test.ts
View file
Open in desktop
@@ -1,16 +1,17 @@
import Window from '../../../src/window/Window';
import Document from '../../../src/nodes/document/Document';
import HTMLOptionElement from '../../../src/nodes/html-option-element/HTMLOptionElement';
import IHTMLOptionElement from '../../../src/nodes/html-option-element/IHTMLOptionElement';
import IHTMLSelectElement from '../../../src/nodes/html-select-element/IHTMLSelectElement';

describe('HTMLOptionElement', () => {
let window: Window;
let document: Document;
let element: HTMLOptionElement;
let element: IHTMLOptionElement;

beforeEach(() => {
window = new Window();
document = window.document;
element = <HTMLOptionElement>document.createElement('option');
element = <IHTMLOptionElement>document.createElement('option');
});

describe('Object.prototype.toString', () => {
Expand All @@ -33,20 +34,78 @@ describe('HTMLOptionElement', () => {
});
});

for (const property of ['disabled', 'selected']) {
describe(`get ${property}()`, () => {
it('Returns attribute value.', () => {
expect(element[property]).toBe(false);
element.setAttribute(property, '');
expect(element[property]).toBe(true);
});
describe('get disabled()', () => {
it('Returns the attribute "disabled".', () => {
element.setAttribute('disabled', '');
expect(element.disabled).toBe(true);
});
});

describe(`set ${property}()`, () => {
it('Sets attribute value.', () => {
element[property] = true;
expect(element.getAttribute(property)).toBe('');
});
describe('set disabled()', () => {
it('Sets the attribute "disabled".', () => {
element.disabled = true;
expect(element.getAttribute('disabled')).toBe('');
});
}
});

describe('get selected()', () => {
it('Returns the selected state of the option.', () => {
const select = <IHTMLSelectElement>document.createElement('select');
const option1 = <IHTMLOptionElement>document.createElement('option');
const option2 = <IHTMLOptionElement>document.createElement('option');

expect(option1.selected).toBe(false);
expect(option2.selected).toBe(false);

select.appendChild(option1);
select.appendChild(option2);

expect(option1.selected).toBe(true);
expect(option2.selected).toBe(false);
expect(option1.getAttribute('selected')).toBe(null);
expect(option2.getAttribute('selected')).toBe(null);

select.options.selectedIndex = 1;

expect(option1.selected).toBe(false);
expect(option2.selected).toBe(true);
expect(option1.getAttribute('selected')).toBe(null);
expect(option2.getAttribute('selected')).toBe(null);

select.options.selectedIndex = -1;

expect(option1.selected).toBe(false);
expect(option2.selected).toBe(false);
});
});

describe('set selected()', () => {
it('Sets the selected state of the option.', () => {
const select = <IHTMLSelectElement>document.createElement('select');
const option1 = <IHTMLOptionElement>document.createElement('option');
const option2 = <IHTMLOptionElement>document.createElement('option');

expect(option1.selected).toBe(false);
expect(option2.selected).toBe(false);

option1.selected = true;

expect(select.selectedIndex).toBe(-1);

select.appendChild(option1);
select.appendChild(option2);

option1.selected = true;

expect(select.selectedIndex).toBe(0);

option2.selected = true;

expect(select.selectedIndex).toBe(1);

option2.selected = false;

expect(select.selectedIndex).toBe(0);
});
});
});
59 changes: 29 additions & 30 deletions packages/happy-dom/test/nodes/html-option-element/HTMLOptionsCollection.test.ts
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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);
});
});

Expand Down Expand Up @@ -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);
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 •
edited

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 😅


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);
});
});
Expand Down