fix: Includes accessibility requirements for checkbox in core and platform #2989

DeepakSap14 · 2020-08-05T07:16:17Z

Please provide a link to the associated issue.

#2602
#2866

Please provide a brief summary of this pull request.

This PR provides checkbox accessibility requirements

includes minimum aria-* tags required,
tabIndex to put checkbox in tab order,
title to put tooltip on checkbox,
fix for Issue fix: Stop Label click event propagation for Checkbox #2866 by muting the event,
fixing accessibility in core checkbox, platform checkbox and platform checkbox group.

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist

Documentation checklist:

Documentation Examples
Stackblitz works for all examples

sKudum

please update the pr details

netlify · 2020-08-05T07:23:54Z

Deploy preview for fundamental-ngx ready!

Built with commit 3aa5e23

https://deploy-preview-2989--fundamental-ngx.netlify.app

sKudum · 2020-08-05T07:24:03Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+     * take precedence so this may be omitted.
+     */
+    @Input('aria-label')
+    ariaLabel = '';


aria-label, aria-labelled by,aria-disabled is widely used along most of the components, Won't it help to have them in base class

@fkolar @KevinOkamoto , what do you suggest.

I don't think an input is necessary, on core, application developer can simply use , so maybe the same thing here?

tried without Input aria-label by passing aria-label/attr.aria-label to fdp-checkbox, but it does not work.
Need to paas aria-label to native html element, so the screen reader can work correctly.

I agree we probably should put ariaLabel, ariaLabelledBy, etc. in the BaseInput class. However, I think we should do that in a separate PR. This will effect a lot of components, and I don't think it's appropriate to add to the file changes in this PR.

Let's create a GitHub issue for the ariaLabel, etc. in BaseInput, and leave this code the way it is.

I was about to ask dont we have these on BaseInput?

@mikerodonnell89 you need that in core as you have wrapping component

Ok that makes sense... needs to be on the input element specifically

sKudum

If we had few test scripts to. check accessibility that will add on.

sKudum · 2020-08-05T07:30:09Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.ts

@@ -235,6 +282,11 @@ export class CheckboxComponent implements ControlValueAccessor {
        return event.keyCode === 32;
    }

+    /** @hidden Determines event source based on key code */
+    private _isEnterKeyEvent(event: KeyboardEvent): boolean {
+        return event.keyCode === 13;


Hardcoding hinders flexibility and adaptability for future changes. We should try to avoid it as much. as possible.

yes, can use CDK keycodes instead

it is core/checkbox, if you see at line 282. they have used event.keycode, so i kept in same format. if core team suggest the same then i will change it.

you should use KeyUtil from the core library

mikerodonnell89 · 2020-08-10T17:55:02Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.ts

@@ -235,6 +282,11 @@ export class CheckboxComponent implements ControlValueAccessor {
        return event.keyCode === 32;
    }

+    /** @hidden Determines event source based on key code */
+    private _isEnterKeyEvent(event: KeyboardEvent): boolean {
+        return event.keyCode === 13;


you should use KeyUtil from the core library

mikerodonnell89 · 2020-08-10T18:09:49Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+     * take precedence so this may be omitted.
+     */
+    @Input('aria-label')
+    ariaLabel = '';


I don't think an input is necessary, on core, application developer can simply use , so maybe the same thing here?

KevinOkamoto

Try to avoid property binding to functions and getters, as Angular will inefficiently call these after every change detection cycle. See https://angular.io/guide/interpolation#quick-execution

KevinOkamoto · 2020-08-10T18:00:23Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.html

+    [attr.tabIndex]="tabIndex"
+    [attr.aria-label]="ariaLabel || null"
+    [attr.aria-labelledby]="ariaLabelledby"
+    [attr.aria-checked]="_getAriaChecked()"


Instead of using the function call _getAriaChecked() here, you should create a component property here, ariaChecked and then update it's value in the nextValue() method.

public ariaChecked: 'true' | 'false' | 'mixed';

KevinOkamoto · 2020-08-10T18:03:13Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.html

+    [attr.aria-checked]="_getAriaChecked()"
+    [attr.aria-describedby]="ariaDescribedby"
+    [attr.aria-disabled]="ariaDisabled"
+    [attr.title]="title"
    [class.fd-checkbox--compact]="compact"
    [ngClass]="state ? 'is-' + state : ''"
    [ngModel]="isChecked"


I see that isChecked is a getter method. You should change it to a boolean property, and update it in the nextValue() method ... do the same as _getAriaChecked() above.

changing isChecked will introduce breaking change. I have made changes for _getAriaChecked().

KevinOkamoto

Looks good.

fkolar · 2020-08-21T08:00:40Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.ts

@@ -11,7 +11,7 @@ import {
 } from '@angular/core';
 import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
 import { FdCheckboxValues } from './fd-checkbox-values.interface';
-import { compareObjects } from '../../utils/public_api';
+import { compareObjects, KeyUtil } from '../../utils/public_api';


Dont use public_api for imports, referene these files.

fkolar · 2020-08-21T08:05:58Z

libs/core/src/lib/checkbox/checkbox/checkbox.component.ts

+     * Includes the checkbox in the page tab sequence.
+     */
+    @Input()
+    tabIndex = 0;


Let's switch this to @Attribute decorator, as tab index will have always constant value.

using @Attribute in constructor now.
Done

fkolar · 2020-08-21T08:07:45Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+     * take precedence so this may be omitted.
+     */
+    @Input('aria-label')
+    ariaLabel = '';


I was about to ask dont we have these on BaseInput?

fkolar · 2020-08-21T08:08:16Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+     * take precedence so this may be omitted.
+     */
+    @Input('aria-label')
+    ariaLabel = '';


@mikerodonnell89 you need that in core as you have wrapping component

fkolar · 2020-08-21T08:08:28Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+     * Includes the checkbox in the page tab sequence.
+     */
+    @Input()
+    tabIndex: string;


the same as in core

fkolar · 2020-08-21T08:09:35Z

libs/platform/src/lib/components/form/checkbox-group/checkbox-group.component.ts

+     * CBG label for accessibility
+     */
+    @Input()
+    ariaLabel: string;


Let's move this up. do we have some usecase aribaLabels also for component inheriting BaseComponent ?

Moved these to base class created for all components.
Components other than form components will also need these aria-* tags for accessibility.
Done

libs/core/src/lib/checkbox/checkbox/checkbox.component.ts

DeepakSap14 · 2020-08-28T04:12:35Z

waiting #3083 to be merged. Need to rebase this PR on top of #3083.
review comments has been addressed. once rebase is done, will ask for review.

DeepakSap14 · 2020-08-28T17:47:25Z

This can be reviewed now.

mikerodonnell89

This looks good to me now, wondering if we should have an example for core as well or if the API docs should be enough

DeepakSap14 added the platform platform label Aug 5, 2020

DeepakSap14 requested review from fkolar, KevinOkamoto, sKudum, kavya-b, manu-kr and Lokanathan-k August 5, 2020 07:16

DeepakSap14 self-assigned this Aug 5, 2020

sKudum reviewed Aug 5, 2020

View reviewed changes

DeepakSap14 added this to In Review in Platform Development Aug 5, 2020

This was linked to issues Aug 5, 2020

Accessibility implementation in checkbox #2602

Closed

fix: Stop Label click event propagation for Checkbox #2866

Closed

sKudum reviewed Aug 5, 2020

View reviewed changes

DeepakSap14 force-pushed the fix/checkboxA11y branch from c1372c4 to 2be2f88 Compare August 10, 2020 14:46

mikerodonnell89 requested changes Aug 10, 2020

View reviewed changes

KevinOkamoto reviewed Aug 10, 2020

View reviewed changes

DeepakSap14 force-pushed the fix/checkboxA11y branch from 2be2f88 to 109d7ea Compare August 11, 2020 14:28

DeepakSap14 requested review from mikerodonnell89, KevinOkamoto and sKudum August 12, 2020 06:41

DeepakSap14 force-pushed the fix/checkboxA11y branch from 2880b0f to 5b7b222 Compare August 17, 2020 04:53

KevinOkamoto approved these changes Aug 17, 2020

View reviewed changes

DeepakSap14 changed the title ~~[WIP] fix: (platform) Includes accessibility requirements for checkbox~~ fix: (platform) Includes accessibility requirements for checkbox Aug 18, 2020

DeepakSap14 changed the title ~~fix: (platform) Includes accessibility requirements for checkbox~~ fix: Includes accessibility requirements for checkbox in core and platform Aug 20, 2020

DeepakSap14 force-pushed the fix/checkboxA11y branch from b745ac8 to 8ac74da Compare August 20, 2020 09:50

fkolar requested changes Aug 21, 2020

View reviewed changes

salarenko suggested changes Aug 21, 2020

View reviewed changes

salarenko added the core Core library specific issues label Aug 21, 2020

DeepakSap14 force-pushed the fix/checkboxA11y branch from 8ac74da to 31e736c Compare August 25, 2020 12:26

DeepakSap14 requested review from salarenko and fkolar August 26, 2020 06:22

DeepakSap14 added 10 commits August 28, 2020 18:56

fix:Accessibility for checkbox

f7b773f

fix:stopping click event

a6c86ee

fix:formatted example

5a11fe0

fix:CBG accessibility added

1b20af9

fix:review comments, unit tests

ea4e4bb

fix:formatted page [ci skip]

0ab0f3a

fix: moved tooltip to label

277840d

fix: stackblitz example fixes

6859122

fix: included missed aria-label

7a638c0

fix: review implementation

3aa5e23

DeepakSap14 force-pushed the fix/checkboxA11y branch from 31e736c to 3aa5e23 Compare August 28, 2020 17:46

mikerodonnell89 approved these changes Aug 31, 2020

View reviewed changes

fkolar approved these changes Sep 1, 2020

View reviewed changes

Platform Development automation moved this from In Review to Approved by Reviewer Sep 1, 2020

salarenko approved these changes Sep 1, 2020

View reviewed changes

DeepakSap14 merged commit b3f5ac2 into master Sep 1, 2020

DeepakSap14 deleted the fix/checkboxA11y branch September 1, 2020 07:33

fix: Includes accessibility requirements for checkbox in core and platform #2989

fix: Includes accessibility requirements for checkbox in core and platform #2989

Conversation

DeepakSap14 commented Aug 5, 2020 • edited

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

sKudum left a comment

Choose a reason for hiding this comment

netlify bot commented Aug 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinOkamoto Aug 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sKudum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinOkamoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinOkamoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepakSap14 commented Aug 28, 2020

DeepakSap14 commented Aug 28, 2020

mikerodonnell89 left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Aug 5, 2020 •

edited

netlify bot commented Aug 5, 2020 •

edited

KevinOkamoto Aug 17, 2020 •

edited