Skip to content

Commit

Permalink
fix: do not set placeholder to empty string by default (#7573)
Browse files Browse the repository at this point in the history
  • Loading branch information
web-padawan authored Jul 25, 2024
1 parent 580f225 commit 4d80c5c
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,6 @@ class MultiSelectComboBox extends ResizeMixin(InputControlMixin(ThemableMixin(El
*/
placeholder: {
type: String,
value: '',
observer: '_placeholderChanged',
},

Expand Down Expand Up @@ -810,9 +809,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
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
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

0 comments on commit 4d80c5c

Please sign in to comment.