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

fix: do not set placeholder to empty string by default (#7573) (CP: 24.4) #7584

Merged
merged 1 commit into from
Jul 25, 2024
Merged

fix: do not set placeholder to empty string by default (#7573) (CP: 24.4) #7584

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/multi-select-combo-box/src/vaadin-multi-select-combo-box.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ class MultiSelectComboBox extends ResizeMixin(InputControlMixin(ThemableMixin(El
*/
placeholder: {
type: String,
value: '',
observer: '_placeholderChanged',
},

Expand Down Expand Up @@ -791,9 +790,12 @@ class MultiSelectComboBox extends ResizeMixin(InputControlMixin(ThemableMixin(El
// Use placeholder for announcing items
if (this._hasValue) {
const tmpPlaceholder = this._mergeItemLabels(selectedItems);
if (this.__tmpA11yPlaceholder === undefined) {
this.__savedPlaceholder = this.placeholder;
}
this.__tmpA11yPlaceholder = tmpPlaceholder;
this.placeholder = tmpPlaceholder;
} else {
} else if (this.__tmpA11yPlaceholder !== undefined) {
delete this.__tmpA11yPlaceholder;
this.placeholder = this.__savedPlaceholder;
}
Expand Down
34 changes: 24 additions & 10 deletions packages/multi-select-combo-box/test/accessibility.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,46 +35,60 @@ describe('accessibility', () => {

describe('placeholder', () => {
beforeEach(() => {
comboBox = fixtureSync(`
<vaadin-multi-select-combo-box
placeholder="Fruits"
></vaadin-multi-select-combo-box>
`);
// Do not use `fixtureSync()` helper to test the case where both placeholder
// and selectedItems are set when the component is initialized, to make sure
// that the placeholder is correctly restored after clearing selectedItems.
comboBox = document.createElement('vaadin-multi-select-combo-box');
comboBox.placeholder = 'Fruits';
comboBox.items = ['Apple', 'Banana', 'Lemon', 'Orange'];
comboBox.selectedItems = ['Apple', 'Banana'];
document.body.appendChild(comboBox);
inputElement = comboBox.inputElement;
});

afterEach(() => {
comboBox.remove();
});

it('should set input placeholder when selected items are changed', () => {
comboBox.selectedItems = ['Apple', 'Banana'];
expect(inputElement.getAttribute('placeholder')).to.equal('Apple, Banana');
});

it('should restore original placeholder when selected items are removed', () => {
comboBox.selectedItems = ['Apple', 'Banana'];
comboBox.selectedItems = [];
expect(inputElement.getAttribute('placeholder')).to.equal('Fruits');
});

it('should keep input placeholder when placeholder property is updated', () => {
comboBox.selectedItems = ['Apple', 'Banana'];
comboBox.placeholder = 'Options';
expect(inputElement.getAttribute('placeholder')).to.equal('Apple, Banana');
});

it('should restore updated placeholder when placeholder property is updated', () => {
comboBox.selectedItems = ['Apple', 'Banana'];
comboBox.placeholder = 'Options';
comboBox.selectedItems = [];
expect(inputElement.getAttribute('placeholder')).to.equal('Options');
});

it('should restore placeholder when selected items are updated and removed', () => {
comboBox.selectedItems = ['Apple'];
comboBox.selectedItems = [];
expect(inputElement.getAttribute('placeholder')).to.equal('Fruits');
});

it('should restore empty placeholder when selected items are removed', () => {
comboBox.placeholder = '';
comboBox.selectedItems = ['Apple', 'Banana'];
comboBox.selectedItems = [];
expect(comboBox.placeholder).to.be.equal('');
expect(inputElement.hasAttribute('placeholder')).to.be.false;
});

it('should clear placeholder when set to undefined and selected items are removed', () => {
comboBox.placeholder = undefined;
comboBox.selectedItems = [];
expect(comboBox.placeholder).to.be.undefined;
expect(inputElement.hasAttribute('placeholder')).to.be.false;
});
});
});

Expand Down
25 changes: 5 additions & 20 deletions packages/multi-select-combo-box/test/dom/__snapshots__/multi-select-combo-box.test.snap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
export const snapshots = {};

snapshots["vaadin-multi-select-combo-box host default"] =
`<vaadin-multi-select-combo-box placeholder="">
`<vaadin-multi-select-combo-box>
<label
for="input-vaadin-multi-select-combo-box-4"
id="label-vaadin-multi-select-combo-box-0"
Expand Down Expand Up @@ -38,10 +38,7 @@ snapshots["vaadin-multi-select-combo-box host default"] =
/* end snapshot vaadin-multi-select-combo-box host default */

snapshots["vaadin-multi-select-combo-box host label"] =
`<vaadin-multi-select-combo-box
has-label=""
placeholder=""
>
`<vaadin-multi-select-combo-box has-label="">
<label
for="input-vaadin-multi-select-combo-box-4"
id="label-vaadin-multi-select-combo-box-0"
Expand Down Expand Up @@ -79,10 +76,7 @@ snapshots["vaadin-multi-select-combo-box host label"] =
/* end snapshot vaadin-multi-select-combo-box host label */

snapshots["vaadin-multi-select-combo-box host helper"] =
`<vaadin-multi-select-combo-box
has-helper=""
placeholder=""
>
`<vaadin-multi-select-combo-box has-helper="">
<label
for="input-vaadin-multi-select-combo-box-4"
id="label-vaadin-multi-select-combo-box-0"
Expand Down Expand Up @@ -128,7 +122,6 @@ snapshots["vaadin-multi-select-combo-box host error"] =
`<vaadin-multi-select-combo-box
has-error-message=""
invalid=""
placeholder=""
>
<label
for="input-vaadin-multi-select-combo-box-4"
Expand Down Expand Up @@ -169,10 +162,7 @@ snapshots["vaadin-multi-select-combo-box host error"] =
/* end snapshot vaadin-multi-select-combo-box host error */

snapshots["vaadin-multi-select-combo-box host required"] =
`<vaadin-multi-select-combo-box
placeholder=""
required=""
>
`<vaadin-multi-select-combo-box required="">
<label
for="input-vaadin-multi-select-combo-box-4"
id="label-vaadin-multi-select-combo-box-0"
Expand Down Expand Up @@ -212,7 +202,6 @@ snapshots["vaadin-multi-select-combo-box host disabled"] =
`<vaadin-multi-select-combo-box
aria-disabled="true"
disabled=""
placeholder=""
>
<label
for="input-vaadin-multi-select-combo-box-4"
Expand Down Expand Up @@ -252,10 +241,7 @@ snapshots["vaadin-multi-select-combo-box host disabled"] =
/* end snapshot vaadin-multi-select-combo-box host disabled */

snapshots["vaadin-multi-select-combo-box host readonly"] =
`<vaadin-multi-select-combo-box
placeholder=""
readonly=""
>
`<vaadin-multi-select-combo-box readonly="">
<label
for="input-vaadin-multi-select-combo-box-4"
id="label-vaadin-multi-select-combo-box-0"
Expand Down Expand Up @@ -333,7 +319,6 @@ snapshots["vaadin-multi-select-combo-box host opened default"] =
`<vaadin-multi-select-combo-box
focused=""
opened=""
placeholder=""
>
<label
for="input-vaadin-multi-select-combo-box-4"
Expand Down
Loading