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

refactor(ui5-select): rework value handling #10904

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
59 changes: 58 additions & 1 deletion packages/main/cypress/specs/FormSupport.cy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -563,16 +563,46 @@ describe("Form support", () => {
<Option value="" selected>Option 3</Option>
</Select>

<Select value="Option 1">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<Select value="option2">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>
<Select value="Option 3">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<Select id="select4" name="select4">
<Option selected>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<Select id="select44" name="select44" value="Option 1">
<Option selected>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<Select id="select5" name="select5">
<Option>Option 1</Option>
<Option value="option2" selected>Option 2</Option>
<Option value="">Option 3</Option>
</Select>
<Select id="select55" name="select55" value="option2">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<Select id="select7" name="select7" required>
<Option selected>Option 1</Option>
<Option value="option2">Option 2</Option>
Expand All @@ -589,6 +619,22 @@ describe("Form support", () => {
<Option value="" selected>Option 3</Option>
</Select>

<Select id="select77" name="select77" required value="Option 1">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>
<Select id="select88" name="select88" required value="option2">
<Option>Option 1</Option>
<Option value="option2" selected>Option 2</Option>
<Option value="">Option 3</Option>
</Select>
<Select id="select99" name="select99" required value="Option 3">
<Option>Option 1</Option>
<Option value="option2">Option 2</Option>
<Option value="">Option 3</Option>
</Select>

<button type="submit">Submits forms</button>
</form>);

Expand Down Expand Up @@ -630,6 +676,17 @@ describe("Form support", () => {
.eq(1)
.realClick();

cy.get("#select99")
.realClick();

cy.get("#select99")
.should("have.attr", "opened");

cy.get("#select99")
.find("[ui5-option]")
.eq(1)
.realClick();

cy.get("button")
.realClick();

Expand All @@ -640,7 +697,7 @@ describe("Form support", () => {
.then($el => {
return getFormData($el.get(0));
})
.should("be.equal", "select4=Option 1&select5=option2&select6=&select7=Option 1&select8=option2&select9=option2");
.should("be.equal", "select4=Option 1&select44=Option 1&select5=option2&select55=option2&select6=&select7=Option 1&select8=option2&select9=option2&select77=Option 1&select88=option2&select99=option2");
});

it("ui5-slider in form", () => {
Expand Down
107 changes: 107 additions & 0 deletions packages/main/cypress/specs/Select.cy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,113 @@ import Option from "../../src/Option.js";
import OptionCustom from "../../src/OptionCustom.js";
import Select from "../../src/Select.js";

describe("Select - value handling", () => {
it("tests selection via Select's value", () => {
cy.mount(
<>
<Select value="option2">
<Option id="opt1" value="option1">Option 1</Option>
<Option id="opt2" value="option2">Option 2</Option>
<Option id="opt3" value="option3">Option 3</Option>
</Select>

<Select value="option6">
<Option id="opt4" value="option4">Option 4</Option>
<Option id="opt5" value="option5">Option 5</Option>
<Option id="opt6" value="option6">Option 6</Option>
</Select>
</>
);

cy.get("#opt2").should("have.attr", "selected");
cy.get("#opt6").should("have.attr", "selected");
});

it("tests Select's value has precedence over Option's selected", () => {
cy.mount(
<>
<Select value="option1">
<Option id="opt1" value="option1">Option 1</Option>
<Option id="opt2" value="option2">Option 2</Option>
<Option id="opt3" value="option3" selected={true}>Option 3</Option>
</Select>

<Select value="option6">
<Option id="opt4" value="option4">Option 4</Option>
<Option id="opt5" value="option5" selected={true}>Option 5</Option>
<Option id="opt6" value="option6">Option 6</Option>
</Select>
</>
);

// assert that Select's value is preferred over Option's selected
cy.get("#opt1").should("have.attr", "selected");
cy.get("#opt3").should("not.have.attr", "selected");
cy.get("#opt5").should("not.have.attr", "selected");
cy.get("#opt6").should("have.attr", "selected");
});

it("tests Select's value of an option, added with delay", () => {
const addOption = () => {
const newOption = document.createElement("ui5-option") as Option;
newOption.id = "opt3";
newOption.value = "option3";
newOption.textContent = "Option 3";
document.getElementById("sel")?.appendChild(newOption);
};

cy.mount(
<Select id="sel" value="option3">
<Option id="opt1" value="option1">Option 1</Option>
<Option id="opt2" value="option2">Option 2</Option>
</Select>
);

cy.get("#opt3").should("not.exist");

cy.wrap(addOption).invoke("call");

// assert "Option 3" is selected although added after the Select was mounted
cy.get("#opt3").should("have.attr", "selected");
});

it("tests Select's value set with a delay", () => {
const setValue = () => {
const select = document.getElementById("sel") as Select;
select.value = "option2";
};

cy.mount(
<Select id="sel">
<Option id="opt1" value="option1">Option 1</Option>
<Option id="opt2" value="option2">Option 2</Option>
<Option id="opt3" value="option3">Option 3</Option>
</Select>
);

cy.wrap(setValue).invoke("call");

// assert "Option 2" is selected after dynamic value change
cy.get("#opt2").should("have.attr", "selected");
});

it("tests Select's value of a missing option - auto-selects firsts", () => {
cy.mount(
<Select id="sel" value="option3">
<Option id="opt1" value="option1">Option 1</Option>
<Option id="opt2" value="option2">Option 2</Option>
</Select>
);

// DISCUSSION:
// the UI shows "Option 1" as selected, but the value is still "option3"
cy.get("#sel")
.should("have.attr", "value", "option3")
.invoke("prop", "value", "option3");
cy.get("#opt1").should("have.attr", "selected");
});
});

describe("Select - Accessibility", () => {
it("tests options tooltip is set displayed", () => {
const EXPECTED_TOOLTIP = "Tooltip";
Expand Down
95 changes: 76 additions & 19 deletions packages/main/src/Select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ class Select extends UI5Element implements IFormInputElement {
responsivePopover!: ResponsivePopover;
valueStatePopover?: Popover;

_valueStorage: string | undefined;

/**
* Defines the component options.
*
Expand Down Expand Up @@ -350,9 +352,7 @@ class Select extends UI5Element implements IFormInputElement {
}

get formValidity(): ValidityStateFlags {
const selectedOption = this.selectedOption;

return { valueMissing: this.required && (selectedOption && selectedOption.getAttribute("value") === "") };
return { valueMissing: this.required && (this.selectedOption?.getAttribute("value") === "") };
}

async formElementAnchor() {
Expand All @@ -361,20 +361,18 @@ class Select extends UI5Element implements IFormInputElement {

get formFormattedValue() {
const selectedOption = this.selectedOption;

if (selectedOption) {
if ("value" in selectedOption && selectedOption.value) {
return selectedOption.value;
}

return selectedOption.hasAttribute("value") ? selectedOption.getAttribute("value") : selectedOption.textContent;
}

return "";
}

onBeforeRendering() {
this._ensureSingleSelection();
this._applySelection();

this.style.setProperty(getScopedVarName("--_ui5-input-icons-count"), `${this.iconsCount}`);
}
Expand All @@ -389,9 +387,43 @@ class Select extends UI5Element implements IFormInputElement {
}
}

_ensureSingleSelection() {
// if no item is selected => select the first one
// if multiple items are selected => select the last selected one
/*
* Applies the option selection,
* based on the Select's "value" or the Options' "selected" properties.
*/
_applySelection() {
// "value" property has not been set
if (this._valueStorage === undefined) {
this._applyAutoSelection();
return;
}

// "value" has been used
// - apply pending selection of the option, that hasn't successfully been selected yet
this._applySelectionByValue();

// - apply auto-selection if no option has matched the value
if (!this.selectedOption) {
this._applyAutoSelection();
}
}

/**
* Applies the selection of the option by the given value,
* @private
*/
_applySelectionByValue() {
if (this._valueStorage !== undefined && (this._valueStorage !== (this.selectedOption?.value || this.selectedOption?.textContent))) {
this._selectOptionByValue(this._valueStorage);
}
}

/**
* Selects the first option if no option is selected
* or selects the last option if multiple options are selected.
* @private
*/
_applyAutoSelection() {
let selectedIndex = this.options.findLastIndex(option => option.selected);
selectedIndex = selectedIndex === -1 ? 0 : selectedIndex;
for (let i = 0; i < this.options.length; i++) {
Expand All @@ -402,6 +434,21 @@ class Select extends UI5Element implements IFormInputElement {
}
}

/**
* Selects an option by string value.
* @private
*/
_selectOptionByValue(newValue: string) {
const options = Array.from(this.children) as Array<IOption>;
options.forEach(option => {
option.selected = !!((option.getAttribute("value") || option.textContent) === newValue);
});
}

updateValueByOption(option: IOption) {
this.value = option.value || option.textContent || "";
}

_applyFocus() {
this.focus();
}
Expand All @@ -425,10 +472,12 @@ class Select extends UI5Element implements IFormInputElement {
/**
* Defines the value of the component:
*
* - when get - returns the value of the component, e.g. the `value` property of the selected option or its text content.
*
* - when get - returns the value of the component or the value/text content of tge selected option.
* - when set - selects the option with matching `value` property or text content.
*
* **Note:** Use either the Select's value or the Options' selected property.
* Mixed usage could result in unexpected behavior.
*
* **Note:** If the given value does not match any existing option,
* the first option will get selected.
* @public
Expand All @@ -437,16 +486,16 @@ class Select extends UI5Element implements IFormInputElement {
* @formProperty
* @formEvents change liveChange
*/
@property({ noAttribute: true })
@property()
set value(newValue: string) {
const options = Array.from(this.children) as Array<IOption>;

options.forEach(option => {
option.selected = !!((option.getAttribute("value") || option.textContent) === newValue);
});
this._valueStorage = newValue;
this._selectOptionByValue(newValue);
}

get value(): string {
if (this._valueStorage !== undefined) {
return this._valueStorage;
}
return this.selectedOption?.value || this.selectedOption?.textContent || "";
}

Expand Down Expand Up @@ -601,11 +650,15 @@ class Select extends UI5Element implements IFormInputElement {
this.options[selectedIndex].selected = false;
}

const selectedOption = this.options[index];
if (selectedIndex !== index) {
this.fireDecoratorEvent("live-change", { selectedOption: this.options[index] });
this.fireDecoratorEvent("live-change", { selectedOption });
}

this.options[index].selected = true;
selectedOption.selected = true;
if (this._valueStorage !== undefined) {
this.updateValueByOption(selectedOption);
}
}

/**
Expand Down Expand Up @@ -698,6 +751,10 @@ class Select extends UI5Element implements IFormInputElement {
nextOption.selected = true;
nextOption.focused = true;

if (this._valueStorage !== undefined) {
this.updateValueByOption(nextOption);
}

this.fireDecoratorEvent("live-change", { selectedOption: nextOption });

if (!this._isPickerOpen) {
Expand Down
2 changes: 1 addition & 1 deletion packages/main/test/pages/Select.html
Original file line number Diff line number Diff line change
Expand Up @@ -356,4 +356,4 @@ <h2>popover max-height changed with 'popover' part (120px)</h2>
mySelect7.value = "NAN";
});
</script>
</html>
</html>
Loading
Loading