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

feat!: use role attribute instead of native button #2394

Merged
merged 8 commits into from
Aug 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 14 additions & 13 deletions packages/button/src/vaadin-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
import { PolymerElement, html } from '@polymer/polymer/polymer-element.js';
import { ThemableMixin } from '@vaadin/vaadin-themable-mixin/vaadin-themable-mixin.js';
import { ElementMixin } from '@vaadin/vaadin-element-mixin/vaadin-element-mixin.js';
import { TabindexMixin } from '@vaadin/field-base/src/tabindex-mixin.js';
import { ActiveMixin } from '@vaadin/field-base/src/active-mixin.js';
import { ControlStateMixin } from '@vaadin/vaadin-control-state-mixin/vaadin-control-state-mixin.js';
import { FocusMixin } from '@vaadin/field-base/src/focus-mixin.js';

class Button extends ControlStateMixin(ActiveMixin(ElementMixin(ThemableMixin(PolymerElement)))) {
class Button extends ActiveMixin(TabindexMixin(FocusMixin(ElementMixin(ThemableMixin(PolymerElement))))) {
static get is() {
return 'vaadin-button';
}
Expand Down Expand Up @@ -65,7 +66,7 @@ class Button extends ControlStateMixin(ActiveMixin(ElementMixin(ThemableMixin(Po
text-overflow: ellipsis;
}
</style>
<button id="button" type="button" part="button">
<div part="button">
<span part="prefix">
<slot name="prefix"></slot>
</span>
Expand All @@ -75,19 +76,19 @@ class Button extends ControlStateMixin(ActiveMixin(ElementMixin(ThemableMixin(Po
<span part="suffix">
<slot name="suffix"></slot>
</span>
</button>
</div>
`;
}

/**
* A getter that returns the native button as a focusable element for ControlStateMixin.
*
* @protected
* @override
* @return {HTMLButtonElement}
*/
get focusElement() {
return this.$.button;
/** @protected */
ready() {
super.ready();

// By default, if the user hasn't provided a custom role,
// the role attribute is set to "button".
if (!this.hasAttribute('role')) {
web-padawan marked this conversation as resolved.
Show resolved Hide resolved
this.setAttribute('role', 'button');
}
}
}

Expand Down
155 changes: 127 additions & 28 deletions packages/button/test/button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,33 +42,70 @@ describe('vaadin-button', () => {
});
});

describe('default', () => {
let button, label;
describe('role', () => {
describe('default', () => {
beforeEach(() => {
element = fixtureSync('<vaadin-button>Press me</vaadin-button>');
});

beforeEach(() => {
element = fixtureSync('<vaadin-button>Vaadin <i>Button</i></vaadin-button>');
button = element.$.button;
label = element.shadowRoot.querySelector('[part=label]');
it('should set role attribute to button by default', () => {
expect(element.getAttribute('role')).to.equal('button');
});
});

it('should have [type=button] on the native button', () => {
expect(button.getAttribute('type')).to.be.eql('button');
describe('custom', () => {
beforeEach(() => {
element = fixtureSync('<vaadin-button role="menuitem">Press me</vaadin-button>');
});

it('should not override custom role attribute', () => {
expect(element.getAttribute('role')).to.equal('menuitem');
});
});
});

describe('label', () => {
let label;

beforeEach(() => {
element = fixtureSync('<vaadin-button>Press me</vaadin-button>');
label = element.shadowRoot.querySelector('[part=label]');
});

it('should define the button label using light DOM', () => {
const children = FlattenedNodesObserver.getFlattenedNodes(label);
expect(children[1].textContent).to.be.equal('Vaadin ');
expect(children[2].outerHTML).to.be.equal('<i>Button</i>');
expect(children[1].textContent).to.be.equal('Press me');
});
});

it('should disable the native button', () => {
element.disabled = true;
expect(button.hasAttribute('disabled')).to.be.true;
describe('mixins', () => {
beforeEach(() => {
element = fixtureSync('<vaadin-button>Press me</vaadin-button>');
});

// TODO: Remove when it will be possible to detect whether the button element inherits ActiveMixin.
// A related issue: https://github.com/vaadin/web-components/issues/2357
describe('active', () => {
// TODO: Remove when it would be possible for an element:
// – to detect if it inherits DisabledMixin.
// – or to run a suit of the tests defined in DisabledMixin.
describe('DisabledMixin', () => {
it('should set disabled property to false by default', () => {
expect(element.disabled).to.be.false;
});

it('should reflect disabled property to attribute', () => {
element.disabled = true;
expect(element.hasAttribute('disabled')).to.be.true;
});

it('should set the aria-disabled attribute when disabled', () => {
element.disabled = true;
expect(element.getAttribute('aria-disabled')).to.equal('true');
});
});

// TODO: Remove when it would be possible for an element:
// – to detect if it inherits ActiveMixin.
// – or to run a suit of the tests defined in ActiveMixin.
describe('ActiveMixin', () => {
(isIOS ? it.skip : it)('should have active attribute on mousedown', () => {
mousedown(element);
expect(element.hasAttribute('active')).to.be.true;
Expand Down Expand Up @@ -139,23 +176,85 @@ describe('vaadin-button', () => {
});
});

describe('focus-ring', () => {
beforeEach(async () => {
// Focus on the button
await sendKeys({ press: 'Tab' });
// TODO: Remove when it would be possible for an element:
// – to detect if it inherits TabindexMixin.
// – or to run a suit of the tests defined in TabindexMixin.
describe('TabindexMixin', () => {
it('should set tabindex attribute to 0 by default', () => {
expect(element.getAttribute('tabindex')).to.be.equal('0');
});

it('should reflect tabindex property to the attribute', () => {
element.tabindex = 1;
expect(element.getAttribute('tabindex')).to.be.equal('1');
});

it('should set focus-ring attribute when focusing with Tab', () => {
expect(element.hasAttribute('focus-ring')).to.be.true;
it('should reflect native tabIndex property to the attribute', () => {
element.tabIndex = 1;
expect(element.getAttribute('tabindex')).to.be.equal('1');
});

it('should remove focus-ring attribute when loosing focus with Shift+Tab', async () => {
// Focus out of the button
await sendKeys({ down: 'Shift' });
await sendKeys({ press: 'Tab' });
await sendKeys({ up: 'Shift' });
it('should set tabindex attribute to -1 when disabled', () => {
element.tabIndex = 1;
web-padawan marked this conversation as resolved.
Show resolved Hide resolved
element.disabled = true;
expect(element.getAttribute('tabindex')).to.be.equal('-1');
});

it('should restore tabindex attribute when enabled', () => {
element.tabIndex = 1;
element.disabled = true;
element.disabled = false;
expect(element.getAttribute('tabindex')).to.be.equal('1');
});

it('should restore tabindex attribute with the last known value when enabled', () => {
element.tabIndex = 1;
element.disabled = true;
element.tabIndex = 2;
expect(element.getAttribute('tabindex')).to.be.equal('-1');

element.disabled = false;
expect(element.getAttribute('tabindex')).to.be.equal('2');
});
});

// TODO: Remove when it would be possible for an element:
// – to detect if it inherits FocusMixin.
// – or to run a suit of the tests defined in FocusMixin.
describe('FocusMixin', () => {
describe('focusing with Tab', () => {
beforeEach(async () => {
// Focus on the button
await sendKeys({ press: 'Tab' });
});

it('should set focused attribute', () => {
expect(element.hasAttribute('focused')).to.be.true;
});

it('should set focus-ring attribute', () => {
expect(element.hasAttribute('focus-ring')).to.be.true;
});
});

describe('loosing focus with Shift+Tab', () => {
beforeEach(async () => {
// Focus on the button
await sendKeys({ press: 'Tab' });

// Focus out of the button
await sendKeys({ down: 'Shift' });
await sendKeys({ press: 'Tab' });
await sendKeys({ up: 'Shift' });
});

it('should remove focused attribute', () => {
expect(element.hasAttribute('focused')).to.be.false;
});

expect(element.hasAttribute('focus-ring')).to.be.false;
it('should remove focus-ring attribute', () => {
expect(element.hasAttribute('focus-ring')).to.be.false;
});
});
});
});
Expand Down
2 changes: 2 additions & 0 deletions packages/field-base/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ export { SlotMixin } from './src/slot-mixin.js';
export { TextAreaSlotMixin } from './src/text-area-slot-mixin.js';
export { TextFieldMixin } from './src/text-field-mixin.js';
export { ValidateMixin } from './src/validate-mixin.js';
export { TabindexMixin } from './src/tabindex-mixin.js';
export { ActiveMixin } from './src/active-mixin.js';
2 changes: 2 additions & 0 deletions packages/field-base/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ export { SlotMixin } from './src/slot-mixin.js';
export { TextAreaSlotMixin } from './src/text-area-slot-mixin.js';
export { TextFieldMixin } from './src/text-field-mixin.js';
export { ValidateMixin } from './src/validate-mixin.js';
export { TabindexMixin } from './src/tabindex-mixin.js';
export { ActiveMixin } from './src/active-mixin.js';
29 changes: 29 additions & 0 deletions packages/field-base/src/tabindex-mixin.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* @license
* Copyright (c) 2021 Vaadin Ltd.
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { DisabledMixin } from './disabled-mixin.js';

/**
* A mixin to provide the `tabindex` attribute.
*
* By default, the attribute is set to 0 that makes the element focusable.
*
* The attribute is removed whenever the user disables the element
* and restored with the last known value once the element is enabled again.
*/
declare function TabindexMixin<T extends new (...args: any[]) => {}>(base: T): T & TabindexMixinConstructor;

interface TabindexMixinConstructor {
new (...args: any[]): TabindexMixin;
}

interface TabindexMixin extends DisabledMixin {
/**
* Indicates whether the element can be focused and where it participates in sequential keyboard navigation.
*/
tabindex: number | undefined | null;
}

export { TabindexMixinConstructor, TabindexMixin };
78 changes: 78 additions & 0 deletions packages/field-base/src/tabindex-mixin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* @license
* Copyright (c) 2021 Vaadin Ltd.
* This program is available under Apache License Version 2.0, available at https://vaadin.com/license/
*/
import { dedupingMixin } from '@polymer/polymer/lib/utils/mixin.js';
import { DisabledMixin } from './disabled-mixin.js';

const TabindexMixinImplementation = (superclass) =>
class TabindexMixinClass extends DisabledMixin(superclass) {
static get properties() {
return {
/**
* Indicates whether the element can be focused and where it participates in sequential keyboard navigation.
*/
tabindex: {
type: Number,
value: 0,
reflectToAttribute: true,
observer: '_tabindexChanged'
},

/**
* Stores the last known tabindex since the element has been disabled.
*
* @private
*/
__lastTabIndex: {
type: Number,
value: 0
}
};
}

/**
* When the element gets disabled, the observer saves the last known tabindex
* and makes the element not focusable by setting tabindex to -1.
* As soon as the element gets enabled, the observer restores the last known tabindex
* so that the element can be focusable again.
*
* @protected
* @override
*/
_disabledChanged(disabled) {
super._disabledChanged(disabled);

if (disabled) {
this.__lastTabIndex = this.tabindex;
this.tabindex = -1;
} else {
this.tabindex = this.__lastTabIndex;
}
}

/**
* When the user has changed tabindex while the element is disabled,
* the observer reverts tabindex to -1 and rather saves the new tabindex value to apply it later.
* The new value will be applied as soon as the element becomes enabled.
*
* @protected
*/
_tabindexChanged(tabindex) {
if (this.disabled && tabindex !== -1) {
this.__lastTabIndex = tabindex;
this.tabindex = -1;
}
}
};

/**
* A mixin to provide the `tabindex` attribute.
*
* By default, the attribute is set to 0 that makes the element focusable.
*
* The attribute is removed whenever the user disables the element
* and restored with the last known value once the element is enabled.
*/
export const TabindexMixin = dedupingMixin(TabindexMixinImplementation);
web-padawan marked this conversation as resolved.
Show resolved Hide resolved
Loading