fix: Fix Radio Group Accessibility for core and platform and code improvement #3036

DeepakSap14 · 2020-08-17T04:08:45Z

Please provide a link to the associated issue.

#2999

Please provide a brief summary of this pull request.

Accessibility for Radio button and Radio Button Group.
Includes core Radio Button accessibility fix
improves code to remove bugs for radio group
Includes unit tests for changes
fix broken tests in platform
stackblitz example fix

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

netlify · 2020-08-17T04:15:27Z

Deploy preview for fundamental-ngx ready!

Built with commit 3a6ed5f

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

KevinOkamoto

See comment.

libs/platform/src/lib/components/form/radio-group/radio/radio.component.html

KevinOkamoto

I don't think fd-radio-button has aria-label, aria-labelledby, etc. as property bindings.

libs/platform/src/lib/components/form/radio-group/radio/radio.component.html

fkolar · 2020-08-21T08:10:34Z

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

    @Input()
+    ariaLabel: string;


Let's move this up as we already commented in the other PR2989

fkolar · 2020-08-21T08:11:01Z

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

+     * Includes the Radio in the page tab sequence.
+     */
+    @Input()
+    tabIndex = -1;


Just like my comment in the other PR .. for the tabindex

tabindex for Radio button changes.
for selected radio it should be 0 and for not selected radios, it should be -1.
it will change dynamically once selection of choice changes.
So have to keep it as @input().
same is done in materials design implementation as well.

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

libs/core/src/lib/radio/radio-button/radio-button.component.ts

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


The same comment for ariaLabels like on the other places

fkolar · 2020-08-21T08:11:48Z

libs/core/src/lib/radio/radio-button/radio-button.component.ts

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


tabindex for Radio button changes.
for selected radio it should be 0 and for not selected radios, it should be -1.
it will change dynamically once selection of choice changes.
So have to keep it as @input().
same is done in materials design implementation as well.

DeepakSap14 · 2020-08-31T08:35:25Z

This can be reviewed now.

JKMarkowski

Hello @DeepakSap14 I added some minor comments. I see you modified keyboard behaviour for radio group. Default one provided by browsers is not valid?

JKMarkowski · 2020-09-01T10:14:21Z

libs/core/src/lib/radio/radio-button/radio-button.component.ts

@@ -159,9 +181,10 @@ export class RadioButtonComponent implements OnChanges, AfterViewInit, CssClassB
    }

    /** @hidden */
-    labelClicked(): void {
+    labelClicked(event: any): void {


Let's add typing any => MouseEvent

Added MouseEvent and KeyboardEvent. list item is clicking Radio button on "keydown" event as well. so have to support "KeyboardEvent". have to modify list-item radioClicked() as well, to pass event.

Done.

JKMarkowski · 2020-09-01T10:50:46Z

libs/core/src/lib/radio/radio-button/radio-button.component.html

@@ -4,11 +4,17 @@
    [disabled]="disabled"
    [id]="id"
    [name]="name"
+    [attr.tabIndex]="tabIndex"


It should be [attr.tabindex]="tabIndex", with lowercase

Thanks.
corrected for checkbox and radio.

JKMarkowski · 2020-09-01T11:07:02Z

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

+        this.contentRadioButtons.forEach((button) => {
+            button.stateType = this.status;
+        });


You can remove brackets and do it in 1 line

DeepakSap14 · 2020-09-01T16:05:14Z

Hello @DeepakSap14 I added some minor comments. I see you modified keyboard behaviour for radio group. Default one provided by browsers is not valid?

keyboard arrow keys, by default is not supported in browsers like "firefox, safari, edge etc", only "chrome" is supporting default behaviour. so have to support it.

DeepakSap14 · 2020-09-03T04:57:53Z

@JKMarkowski please review the changes for your review.

JKMarkowski

Focus works weird there. There is no way to focus some of radio groups.
From core perspective everything works fine, so I I'll approve it

fix: initial implementation,known bugs

DeepakSap14 · 2020-09-04T05:59:00Z

corrected focus issue while page loading.
ran lint, test and build. all passing.

DeepakSap14 · 2020-09-04T06:21:56Z

Radio group focus works in Firefox.
In Chrome, this seems to be issue only when more than one Radio group present in adjacent manner.
will raise issue for that and address in different PR.

DeepakSap14 added accessibility use this label for any issue or enhancement related to screenreader/keyboard/etc support platform platform core Core library specific issues labels Aug 17, 2020

DeepakSap14 self-assigned this Aug 17, 2020

DeepakSap14 added this to In progress in Development via automation Aug 17, 2020

DeepakSap14 moved this from In progress to Review in progress in Development Aug 17, 2020

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

DeepakSap14 force-pushed the fix/radioA11y branch from b8de248 to 45b6d8a Compare August 17, 2020 04:32

DeepakSap14 requested review from fkolar, KevinOkamoto, sKudum, kavya-b and manu-kr August 17, 2020 04:45

DeepakSap14 changed the title ~~fix: (platform) Fix Radio Group Accessibility and code improvement~~ [WIP] fix: (platform) Fix Radio Group Accessibility and code improvement Aug 17, 2020

KevinOkamoto suggested changes Aug 18, 2020

View reviewed changes

libs/platform/src/lib/components/form/radio-group/radio/radio.component.html Outdated Show resolved Hide resolved

DeepakSap14 requested a review from KevinOkamoto August 19, 2020 15:29

KevinOkamoto reviewed Aug 19, 2020

View reviewed changes

DeepakSap14 changed the title ~~[WIP] fix: (platform) Fix Radio Group Accessibility and code improvement~~ [WIP] fix: Fix Radio Group Accessibility for core and platform and code improvement Aug 20, 2020

DeepakSap14 requested a review from KevinOkamoto August 20, 2020 07:14

KevinOkamoto approved these changes Aug 20, 2020

View reviewed changes

libs/platform/src/lib/components/form/radio-group/radio/radio.component.html Outdated Show resolved Hide resolved

Development automation moved this from Review in progress to Reviewer approved Aug 20, 2020

Platform Development automation moved this from In Review to Approved by Reviewer Aug 20, 2020

Development automation moved this from Reviewer approved to Review in progress Aug 21, 2020

fkolar requested changes Aug 21, 2020

View reviewed changes

Platform Development automation moved this from Approved by Reviewer to In Review Aug 21, 2020

DeepakSap14 force-pushed the fix/radioA11y branch from 89bba7a to db2b3fa Compare August 26, 2020 05:36

DeepakSap14 requested a review from fkolar August 26, 2020 06:24

DeepakSap14 force-pushed the fix/radioA11y branch from db2b3fa to f1d65ad Compare August 31, 2020 04:17

DeepakSap14 changed the title ~~[WIP] fix: Fix Radio Group Accessibility for core and platform and code improvement~~ fix: Fix Radio Group Accessibility for core and platform and code improvement Sep 1, 2020

DeepakSap14 requested a review from a team September 1, 2020 07:34

fkolar approved these changes Sep 1, 2020

View reviewed changes

Development automation moved this from Review in progress to Reviewer approved Sep 1, 2020

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

JKMarkowski suggested changes Sep 1, 2020

View reviewed changes

Development automation moved this from Reviewer approved to Review in progress Sep 1, 2020

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

DeepakSap14 force-pushed the fix/radioA11y branch from f1d65ad to bd24e97 Compare September 1, 2020 16:01

DeepakSap14 requested review from JKMarkowski and a team September 1, 2020 16:05

JKMarkowski approved these changes Sep 3, 2020

View reviewed changes

Development automation moved this from Review in progress to Reviewer approved Sep 3, 2020

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

DeepakSap14 added 8 commits September 4, 2020 09:28

tests: fixing platform tests

c7365d7

fix: initial implementation,known bugs

fix:accessibility fixed

6fd5c25

fix: bugs fixed

1abfe6a

fix: validation error example corrected

1cf663c

Removed separator. causing Stackblitz run issue [ci skip]

48e97ae

fix: error state fix,tabIndex fix

e72242b

fix: review comments

e482482

fix:remove unNecessary focus on active item

3a6ed5f

DeepakSap14 force-pushed the fix/radioA11y branch from bd24e97 to 3a6ed5f Compare September 4, 2020 05:58

DeepakSap14 merged commit 06cc3b8 into master Sep 4, 2020

Development automation moved this from Reviewer approved to Done Sep 4, 2020

DeepakSap14 deleted the fix/radioA11y branch September 4, 2020 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix Radio Group Accessibility for core and platform and code improvement #3036

fix: Fix Radio Group Accessibility for core and platform and code improvement #3036

DeepakSap14 commented Aug 17, 2020 •

edited

netlify bot commented Aug 17, 2020 •

edited

KevinOkamoto left a comment

KevinOkamoto left a comment

fkolar Aug 21, 2020

DeepakSap14 Aug 26, 2020

fkolar Aug 21, 2020

DeepakSap14 Aug 26, 2020

fkolar Aug 21, 2020

DeepakSap14 Aug 26, 2020

fkolar Aug 21, 2020

DeepakSap14 Aug 26, 2020

DeepakSap14 commented Aug 31, 2020

JKMarkowski left a comment

JKMarkowski Sep 1, 2020

DeepakSap14 Sep 1, 2020

JKMarkowski Sep 1, 2020

DeepakSap14 Sep 1, 2020

JKMarkowski Sep 1, 2020

DeepakSap14 Sep 1, 2020

DeepakSap14 commented Sep 1, 2020

DeepakSap14 commented Sep 3, 2020

JKMarkowski left a comment

DeepakSap14 commented Sep 4, 2020

DeepakSap14 commented Sep 4, 2020

fix: Fix Radio Group Accessibility for core and platform and code improvement #3036

fix: Fix Radio Group Accessibility for core and platform and code improvement #3036

Conversation

DeepakSap14 commented Aug 17, 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

netlify bot commented Aug 17, 2020 • edited

KevinOkamoto left a 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

DeepakSap14 commented Aug 31, 2020

JKMarkowski 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

DeepakSap14 commented Sep 1, 2020

DeepakSap14 commented Sep 3, 2020

JKMarkowski left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Sep 4, 2020

DeepakSap14 commented Sep 4, 2020

DeepakSap14 commented Aug 17, 2020 •

edited

netlify bot commented Aug 17, 2020 •

edited