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

Accessibility for Font size/family and Heading dropdowns in toolbar #13893

Merged
merged 22 commits into from
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a3c2c06
PoC v3 - menu.
mmotyczynska Apr 5, 2023
bc885ed
Added accessible labels to font size and family lists.
oleq Apr 5, 2023
d080e30
Make heading more accessible for screen readers.
mmotyczynska Apr 5, 2023
e2a5883
Extend button view with aria-label and aria-labelledby.
mmotyczynska Apr 5, 2023
31592e1
Update heading dropdown button label with the generic feature name.
mmotyczynska Apr 5, 2023
6d440a5
Add missing aria properties to splitbuttonview.
mmotyczynska Apr 5, 2023
ba22bd6
Tests: HeadingUI listview new attributes for accessibility.
mmotyczynska Apr 6, 2023
18aab09
Tests: ButtonView new and updated attributes for accessibility.
mmotyczynska Apr 6, 2023
5112433
Tests: ListItemView new role: presentation.
mmotyczynska Apr 6, 2023
f049734
Tests: ListView new attribute: role.
mmotyczynska Apr 7, 2023
d3bc60a
Tests: font family/size listview properties (role, aria-label).
mmotyczynska Apr 7, 2023
664dd78
Tests: optional attributes for dropdown listview (role, ariaLabel).
mmotyczynska Apr 7, 2023
cc6a385
Merge remote-tracking branch 'origin/master' into ck/13250-accessibil…
mmotyczynska Apr 13, 2023
b65e8cb
Merge remote-tracking branch 'origin/master' into ck/13250-accessibil…
mmotyczynska Apr 17, 2023
4680819
Missing docs.
mmotyczynska Apr 17, 2023
9a14ac0
Missing semicolon.
mmotyczynska Apr 17, 2023
5099dac
Merge branch 'master' into ck/13250-accessibility-selected-state-for-…
oleq Apr 18, 2023
8cb8508
Add missing properties to button interface.
mmotyczynska Apr 19, 2023
592aedf
Test for binding ariaLabelledBy in buttonview.
mmotyczynska Apr 19, 2023
5a17cbb
Missing API docs for role for listview in addListToDropdown.
mmotyczynska Apr 19, 2023
2890137
Missing tests for new aria properties in headingui.
mmotyczynska Apr 19, 2023
ebacd08
Remove unnecessary declarations from splitbuttonview.
mmotyczynska Apr 19, 2023
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
9 changes: 7 additions & 2 deletions packages/ckeditor5-font/src/fontfamily/fontfamilyui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,19 @@ export default class FontFamilyUI extends Plugin {
const options = this._getLocalizedOptions();

const command: FontFamilyCommand = editor.commands.get( FONT_FAMILY )!;
const accessibleLabel = t( 'Font Family' );

// Register UI component.
editor.ui.componentFactory.add( FONT_FAMILY, locale => {
const dropdownView = createDropdown( locale );

addListToDropdown( dropdownView, () => _prepareListOptions( options, command ) );
addListToDropdown( dropdownView, () => _prepareListOptions( options, command ), {
role: 'menu',
ariaLabel: accessibleLabel
} );

dropdownView.buttonView.set( {
label: t( 'Font Family' ),
label: accessibleLabel,
icon: fontFamilyIcon,
tooltip: true
} );
Expand Down Expand Up @@ -110,6 +114,7 @@ function _prepareListOptions( options: Array<FontFamilyOption>, command: FontFam
commandName: FONT_FAMILY,
commandParam: option.model,
label: option.title,
role: 'menuitemradio',
withText: true
} )
};
Expand Down
9 changes: 7 additions & 2 deletions packages/ckeditor5-font/src/fontsize/fontsizeui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,20 @@ export default class FontSizeUI extends Plugin {
const options = this._getLocalizedOptions();

const command: FontSizeCommand = editor.commands.get( FONT_SIZE )!;
const accessibleLabel = t( 'Font Size' );

// Register UI component.
editor.ui.componentFactory.add( FONT_SIZE, locale => {
const dropdownView = createDropdown( locale );

addListToDropdown( dropdownView, () => _prepareListOptions( options, command ) );
addListToDropdown( dropdownView, () => _prepareListOptions( options, command ), {
role: 'menu',
ariaLabel: accessibleLabel
} );

// Create dropdown model.
dropdownView.buttonView.set( {
label: t( 'Font Size' ),
label: accessibleLabel,
icon: fontSizeIcon,
tooltip: true
} );
Expand Down Expand Up @@ -129,6 +133,7 @@ function _prepareListOptions( options: Array<FontSizeOption>, command: FontSizeC
commandParam: option.model,
label: option.title,
class: 'ck-fontsize-option',
role: 'menuitemradio',
withText: true
} )
};
Expand Down
12 changes: 12 additions & 0 deletions packages/ckeditor5-font/tests/fontfamily/fontfamilyui.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,5 +254,17 @@ describe( 'FontFamilyUI', () => {
} );
}
} );

describe( 'listview', () => {
it( 'should have properties set', () => {
// Trigger list view creation (lazy init).
dropdown.isOpen = true;

const listView = dropdown.listView;

expect( listView.element.role ).to.equal( 'menu' );
expect( listView.element.ariaLabel ).to.equal( 'Font Family' );
} );
} );
} );
} );
12 changes: 12 additions & 0 deletions packages/ckeditor5-font/tests/fontsize/fontsizeui.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,5 +293,17 @@ describe( 'FontSizeUI', () => {
} );
}
} );

describe( 'listview', () => {
it( 'should have properties set', () => {
// Trigger list view creation (lazy init).
dropdown.isOpen = true;

const listView = dropdown.listView;

expect( listView.element.role ).to.equal( 'menu' );
expect( listView.element.ariaLabel ).to.equal( 'Font Size' );
} );
} );
} );
} );
12 changes: 9 additions & 3 deletions packages/ckeditor5-heading/src/headingui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export default class HeadingUI extends Plugin {
const t = editor.t;
const options = getLocalizedOptions( editor );
const defaultTitle = t( 'Choose heading' );
const dropdownTooltip = t( 'Heading' );
const accessibleLabel = t( 'Heading' );

// Register UI component.
editor.ui.componentFactory.add( 'heading', locale => {
Expand All @@ -60,6 +60,7 @@ export default class HeadingUI extends Plugin {
model: new Model( {
label: option.title,
class: option.class,
role: 'menuitemradio',
withText: true
} )
};
Expand All @@ -83,12 +84,17 @@ export default class HeadingUI extends Plugin {
}

const dropdownView = createDropdown( locale );
addListToDropdown( dropdownView, itemDefinitions );
addListToDropdown( dropdownView, itemDefinitions, {
ariaLabel: accessibleLabel,
role: 'menu'
} );

dropdownView.buttonView.set( {
ariaLabel: accessibleLabel,
ariaLabelledBy: undefined,
Comment on lines +93 to +94
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes have no test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Added.

isOn: false,
withText: true,
tooltip: dropdownTooltip
tooltip: accessibleLabel
} );

dropdownView.extendTemplate( {
Expand Down
12 changes: 12 additions & 0 deletions packages/ckeditor5-heading/tests/headingui.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,5 +294,17 @@ describe( 'HeadingUI', () => {
] );
} );
} );

describe( 'listview', () => {
it( 'should have properties set', () => {
// Trigger lazy init.
dropdown.isOpen = true;

const listView = dropdown.listView;

expect( listView.element.role ).to.equal( 'menu' );
expect( listView.element.ariaLabel ).to.equal( 'Heading' );
} );
} );
} );
} );
14 changes: 14 additions & 0 deletions packages/ckeditor5-ui/src/button/button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,20 @@ export default interface Button {
*/
class: string | undefined;

/**
* (Optional) The ARIA property reflected by the `aria-label` DOM attribute used by assistive technologies.
*
* @observable
*/
ariaLabel?: string | undefined;

/**
* (Optional) The ARIA property reflected by the `aria-ariaLabelledBy` DOM attribute used by assistive technologies.
*
* @observable
*/
ariaLabelledBy?: string | undefined;

/**
* (Optional) The value of the `style` attribute of the label.
*
Expand Down
37 changes: 31 additions & 6 deletions packages/ckeditor5-ui/src/button/buttonview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,26 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
*/
declare public withKeystroke: boolean;

/**
* @inheritDoc
*/
declare public role: string | undefined;

/**
* @inheritDoc
*/
declare public ariaChecked: boolean | undefined;
Comment on lines +146 to +151
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need these in the Button interface as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do. Added.


/**
* @inheritDoc
*/
declare public ariaLabel?: string | undefined;

/**
* @inheritDoc
*/
declare public ariaLabelledBy: string | undefined;

/**
* Tooltip of the button bound to the template.
*
Expand All @@ -160,6 +180,9 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
const ariaLabelUid = uid();

// Implement the Button interface.
this.set( 'ariaChecked', undefined );
this.set( 'ariaLabel', undefined );
this.set( 'ariaLabelledBy', `ck-editor__aria-label_${ ariaLabelUid }` );
this.set( 'class', undefined );
this.set( 'labelStyle', undefined );
this.set( 'icon', undefined );
Expand All @@ -169,6 +192,7 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
this.set( 'isToggleable', false );
this.set( 'keystroke', undefined );
this.set( 'label', undefined );
this.set( 'role', undefined );
this.set( 'tabindex', -1 );
this.set( 'tooltip', false );
this.set( 'tooltipPosition', 's' );
Expand All @@ -177,7 +201,7 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
this.set( 'withKeystroke', false );

this.children = this.createCollection();
this.labelView = this._createLabelView( ariaLabelUid );
this.labelView = this._createLabelView();

this.iconView = new IconView();
this.iconView.extendTemplate( {
Expand Down Expand Up @@ -209,10 +233,13 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
bind.if( 'withText', 'ck-button_with-text' ),
bind.if( 'withKeystroke', 'ck-button_with-keystroke' )
],
role: bind.to( 'role' ),
type: bind.to( 'type', value => value ? value : 'button' ),
tabindex: bind.to( 'tabindex' ),
'aria-labelledby': `ck-editor__aria-label_${ ariaLabelUid }`,
'aria-label': bind.to( 'ariaLabel' ),
'aria-labelledby': bind.to( 'ariaLabelledBy' ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing test checks if aria-labelledby starts with ck-editor__aria-label. This is a default value but we also need a test to see if the new observable ariaLabelledBy property is actually observable and what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added.

'aria-disabled': bind.if( 'isEnabled', true, value => !value ),
'aria-checked': bind.to( 'isOn' ),
'aria-pressed': bind.to( 'isOn', value => this.isToggleable ? String( !!value ) : false ),
'data-cke-tooltip-text': bind.to( '_tooltipString' ),
'data-cke-tooltip-position': bind.to( 'tooltipPosition' )
Expand Down Expand Up @@ -274,10 +301,8 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto

/**
* Creates a label view instance and binds it with button attributes.
*
* @param ariaLabelUid The aria label UID.
*/
private _createLabelView( ariaLabelUid: string ) {
private _createLabelView() {
const labelView = new View();
const bind = this.bindTemplate;

Expand All @@ -290,7 +315,7 @@ export default class ButtonView extends View<HTMLButtonElement> implements Butto
'ck-button__label'
],
style: bind.to( 'labelStyle' ),
id: `ck-editor__aria-label_${ ariaLabelUid }`
id: this.ariaLabelledBy
},

children: [
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-ui/src/dropdown/button/splitbuttonview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ export default class SplitButtonView extends View<HTMLDivElement> implements Dro
*/
declare public labelStyle: string | undefined;

/**
* @inheritDoc
*/
declare public ariaLabel?: string | undefined;

/**
* @inheritDoc
*/
declare public ariaLabelledBy: string | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't. Last time I checked this I was getting errors regarding not implementing the interface correctly for SplitButtonView. But now it seems to work without these declarations.


/**
* @inheritDoc
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/ckeditor5-ui/src/dropdown/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ export function addListToDropdown(
itemsOrCallback: Collection<ListDropdownItemDefinition> | ( () => Collection<ListDropdownItemDefinition> ),
options: {
ariaLabel?: string;
role?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

} = {}
): void {
if ( dropdownView.isOpen ) {
Expand Down Expand Up @@ -340,6 +341,7 @@ function addListToOpenDropdown(
itemsOrCallback: Collection<ListDropdownItemDefinition> | ( () => Collection<ListDropdownItemDefinition> ),
options: {
ariaLabel?: string;
role?: string;
}
): void {
const locale = dropdownView.locale;
Expand All @@ -348,6 +350,7 @@ function addListToOpenDropdown(
const items = typeof itemsOrCallback == 'function' ? itemsOrCallback() : itemsOrCallback;

listView.ariaLabel = options.ariaLabel;
listView.role = options.role;

listView.items.bindTo( items ).using( def => {
if ( def.type === 'separator' ) {
Expand Down
3 changes: 2 additions & 1 deletion packages/ckeditor5-ui/src/list/listitemview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ export default class ListItemView extends View {
'ck',
'ck-list__item',
bind.if( 'isVisible', 'ck-hidden', value => !value )
]
],
role: 'presentation'
},

children: this.children
Expand Down
9 changes: 9 additions & 0 deletions packages/ckeditor5-ui/src/list/listview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ export default class ListView extends View<HTMLUListElement> implements Dropdown
*/
declare public ariaLabel: string | undefined;

/**
* The property reflected by the `role` DOM attribute to be used by assistive technologies.
*
* @observable
*/
declare public role: string | undefined;

/**
* Helps cycling over focusable {@link #items} in the list.
*/
Expand Down Expand Up @@ -81,6 +88,7 @@ export default class ListView extends View<HTMLUListElement> implements Dropdown
} );

this.set( 'ariaLabel', undefined );
this.set( 'role', undefined );

this.setTemplate( {
tag: 'ul',
Expand All @@ -91,6 +99,7 @@ export default class ListView extends View<HTMLUListElement> implements Dropdown
'ck-reset',
'ck-list'
],
role: bind.to( 'role' ),
'aria-label': bind.to( 'ariaLabel' )
},

Expand Down
28 changes: 28 additions & 0 deletions packages/ckeditor5-ui/tests/button/buttonview.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,18 @@ describe( 'ButtonView', () => {
} );
} );

describe( 'role', () => {
it( 'is not initially set ', () => {
expect( view.element.attributes.role ).to.equal( undefined );
} );

it( 'reacts on view#role', () => {
view.role = 'foo';

expect( view.element.attributes.role.value ).to.equal( 'foo' );
} );
} );

describe( 'text', () => {
it( 'is not initially set ', () => {
expect( view.element.textContent ).to.equal( '' );
Expand Down Expand Up @@ -279,6 +291,22 @@ describe( 'ButtonView', () => {
view.isOn = false;
expect( view.element.hasAttribute( 'aria-pressed' ) ).to.be.false;
} );

it( '-checked reacts on #isOn', () => {
view.isOn = true;
expect( view.element.attributes[ 'aria-checked' ].value ).to.equal( 'true' );

view.isOn = false;
expect( view.element.hasAttribute( 'aria-checked' ) ).to.be.false;
} );

it( '-label reacts on #ariaLabel', () => {
view.ariaLabel = undefined;
expect( view.element.hasAttribute( 'aria-label' ) ).to.be.false;

view.ariaLabel = 'Foo';
expect( view.element.attributes[ 'aria-label' ].value ).to.equal( 'Foo' );
} );
} );

describe( 'mousedown event', () => {
Expand Down
Loading