Pr/cdr 2153 refactor cdr input #39

KenjiCrosland · 2022-03-08T19:03:26Z

Refactor CdrInput to use <script setup> syntax.
Fixed v-model issue by referencing a new computed ‘inputModel’ with its own custom getter and setter.
Ensured all fallthrough attributes except class get applied to the input element. The class attribute is bound to the outermost element.
Added a uid module to provide a unique id for each component should an id not be provided by the user.
Fixed some implementations issue on the CdrInput example page.
Fixed a typo in CdrLabelStandalone.

…s with CdrInput. Add uids Refactor CdrInput to use <script setup> syntax. Fixed v-model issue by referencing a new computed ‘inputModel’ with its own custom getter and setter. Ensured all fallthrough attributes except class get applied to the input element. The class attribute is bound to the outermost element. Added a uid module to provide a unique id for each component should an id not be provided by the user. Fixed some implementations issue on the CdrInput example page. Fixed a typo in CdrLabelStandalone.

src/components/input/examples/Inputs.vue

src/utils/uid.js

mhewson · 2022-03-08T20:46:50Z

pulling this down to do some screen reader validation

mhewson · 2022-03-08T23:12:22Z

src/components/input/examples/Inputs.vue

@@ -172,7 +172,7 @@
        <span id="errorMessage">you have added too many characters, remove some</span>


AXE Dev tools is flagging how we use aria=errormessage
Fix the following:
aria-errormessage value undefined-error must use a technique to announce the message (e.g., aria-live, aria-describedby, role=alert, etc.)

here is some additional context I tracked down

dequelabs/axe-core#499

Good catch. I fixed the undefined-error. It looks like cdr-form-error isn't structured so that the id of the span containing the error message has the same value as aria-errormessage on the input element. Rather it's referencing the div containing the span. Is that ok or do we need the id to be on the element that contains the text node of the error message itself?

Was this test run via npm test? I saw a11y tests pop up but I'm having chromedriver issues.

No I manually tested this in Axe dev tools - you may need a license for that still, if so we can reach out to Eric

As far as if the id needs to be on the text node, https://rei.github.io/rei-cedar-docs/patterns/form-validation/#error-detection. I believe that it is fine on the containing wrapper though we can run some screen reader tests to be sure

Yes it does looks like I need a license for Axe Dev Tools. Last time I used it it was free but rather clunky IIRC

OH and this will need to be a unique ID, not "errormessage" but maybe {{input-lable}}-errormessage or something?

Yes I fixed that. I got a trial account of axe and it looks like the issue isn't popping up anymore. The issue was twofold. I neglected to rename all instances of a variable (I need better refactoring extensions in vscode) and also there was an issue in the implementation of the form on the example page. Will double check after office hours

src/components/input/examples/Inputs.vue

…ove tabindex from form error Removed ids from the spans slotted to the form error. These ids are automatically provided by the parent component. Remove tab-index=0 to form errors as form errors should not be tabbable.

Fixed issues with id mismatches which caused problems with axe and screenreaders. Also removed `aria-label=required` from cdr-label-standalone since this is superflous when the input is already labeled required

…tandalone snapshot

KenjiCrosland · 2022-03-10T19:40:12Z

src/components/formError/CdrFormError.vue

@@ -1,5 +1,5 @@
 <template>
-  <div :class="style[baseClass]" role="status" tabindex="0">


This fixes the accessibility issue where the error was tabbable.

KenjiCrosland · 2022-03-10T19:42:02Z

src/components/input/examples/Inputs.vue

@@ -169,10 +169,10 @@
      </template>

      <template #error>
-        <span id="errorMessage">you have added too many characters, remove some</span>
+        <span>you have added too many characters, remove some</span>


This id is superfluous and is the cause of some axe accessibility issues.

KenjiCrosland · 2022-03-10T19:43:59Z

src/components/labelStandalone/CdrLabelStandalone.vue

@@ -8,7 +8,6 @@
        {{ label }}{{ required || optional ? '' : '' }}
        <span
          v-if="required"
-          aria-label="required"


aria-label="required" is not needed here. Since the input element already has aria-required=true all this does is make screen readers say "Required" twice.

More on this issue here: https://axe.deque.com/issues/20da7889-f026-402a-8886-3d5df7d2d83a

benjag · 2022-03-10T21:57:03Z

src/components/input/CdrInput.vue

+  inheritAttrs: false,
+  customOptions: {}
+}
+</script>


Interesting, I was wondering when we might run in to needing to do this with the setup syntax

Kenji Crosland added 2 commits March 8, 2022 13:56

test(test snapshots): fix test snapshots

9f69c73

KenjiCrosland requested review from benjag and mhewson March 8, 2022 19:03

remove unneeded comments

fe2c23b

benjag reviewed Mar 8, 2022

View reviewed changes

src/components/input/examples/Inputs.vue Outdated Show resolved Hide resolved

benjag reviewed Mar 8, 2022

View reviewed changes

src/utils/uid.js Outdated Show resolved Hide resolved

benjag reviewed Mar 8, 2022

View reviewed changes

src/utils/uid.js Outdated Show resolved Hide resolved

Kenji Crosland added 3 commits March 8, 2022 14:18

Add the proper setBackground parameters

3e62e0e

Small uid fixes

4b88702

setBackground fix

f88add0

mhewson reviewed Mar 8, 2022

View reviewed changes

mhewson reviewed Mar 9, 2022

View reviewed changes

src/components/input/examples/Inputs.vue Show resolved Hide resolved

Kenji Crosland added 4 commits March 9, 2022 16:18

test(input snapshot files): remove tabindex from snapshot files

5db9cdc

fix(cdr-input and cdr-label-standalone): fix axe and screenreader issues

3e13ef6

Fixed issues with id mismatches which caused problems with axe and screenreaders. Also removed `aria-label=required` from cdr-label-standalone since this is superflous when the input is already labeled required

test(fix label snapshot): remove aria-label=required from cdr-label-s…

932affa

…tandalone snapshot

KenjiCrosland commented Mar 10, 2022

View reviewed changes

mhewson approved these changes Mar 10, 2022

View reviewed changes

benjag reviewed Mar 10, 2022

View reviewed changes

benjag approved these changes Mar 10, 2022

View reviewed changes

KenjiCrosland merged commit 3159879 into main Mar 11, 2022

benjag deleted the pr/CDR-2153-refactor-cdr-input branch June 6, 2023 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr/cdr 2153 refactor cdr input #39

Pr/cdr 2153 refactor cdr input #39

KenjiCrosland commented Mar 8, 2022

mhewson commented Mar 8, 2022

mhewson Mar 8, 2022

KenjiCrosland Mar 9, 2022

mhewson Mar 9, 2022

mhewson Mar 9, 2022

KenjiCrosland Mar 9, 2022

mhewson Mar 9, 2022

KenjiCrosland Mar 9, 2022 •

edited

Loading

KenjiCrosland Mar 10, 2022

KenjiCrosland Mar 10, 2022

KenjiCrosland Mar 10, 2022 •

edited

Loading

benjag Mar 10, 2022

		@@ -172,7 +172,7 @@
		<span id="errorMessage">you have added too many characters, remove some</span>

		@@ -1,5 +1,5 @@
		<template>
		<div :class="style[baseClass]" role="status" tabindex="0">

Pr/cdr 2153 refactor cdr input #39

Pr/cdr 2153 refactor cdr input #39

Conversation

KenjiCrosland commented Mar 8, 2022

mhewson commented Mar 8, 2022

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

KenjiCrosland Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KenjiCrosland Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KenjiCrosland Mar 9, 2022 •

edited

Loading

KenjiCrosland Mar 10, 2022 •

edited

Loading