Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good, added some non-blocking requests!
| * clear them. They stay on the host instead, which only works for global | ||
| * attributes since the host has no role. | ||
| */ | ||
| const HOST_ARIA_ATTRIBUTES = ['aria-describedby', 'aria-invalid']; |
There was a problem hiding this comment.
aria-errormessage and aria-required have the same problem this comment describes: an app can't remove them once they're in the initial markup. Should they be in this list too?
| <h2>Host aria-describedby and aria-invalid</h2> | ||
| <ion-input-otp id="host-aria" length="4" aria-label="Code"></ion-input-otp> | ||
| <p id="server-error">The server rejected this code</p> | ||
| <button id="toggle-host-aria">Toggle host error</button> |
There was a problem hiding this comment.
This only changes ARIA, so clicking it doesn't change anything on screen, and the error message is visible the whole time. Could the button show and hide #server-error along with the attributes, or show the current aria-invalid value next to it? That way it's clear the toggle did something.
| * With a host `aria-labelledby`, each box also references itself so its | ||
| * position is still read. | ||
| */ | ||
| private getInputLabelAttributes(index: number, hostAttributes: Attributes) { |
There was a problem hiding this comment.
A host aria-label produces "PIN, 1 of 4", but a host aria-labelledby produces "Verification code 1 of 4", because the browser joins the references with a space. Should these match? Dropping the comma would make them match, and the aria-labelledby path can't easily add one.
| await expect(page.locator('ion-input-otp .input-otp-group')).toHaveAttribute('aria-label', 'Code'); | ||
| }); | ||
|
|
||
| test('should describe the group, not each input, with the slotted description', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
This is the same check as "should describe the group, not each input, with the slotted description" in input-otp.spec.ts:110. Is the e2e version needed for something the spec can't cover? If it's just the toBeVisible(), the spec version could be dropped instead.
If we keep this, it covers behavior added in this PR rather than something reported in #31619, so I'd drop the issue annotation.
There was a problem hiding this comment.
Same for the annotations on "should forward aria-invalid and aria-describedby set on the host after load", "should clear aria-invalid and aria-describedby from the inputs when removed from the host", "should rename the inputs when the host aria-label changes", and "should restore the default labels when a host aria-label set after load is removed". #31619 reports attributes in the initial markup; changes after load are new behavior from this PR.
There was a problem hiding this comment.
With VoiceOver, typing a digit moves focus to the next box while the digit is still being echoed, so "PIN, 2 of 4" gets cut off by the echo. It happened with "Input 2 of 4" too, but the longer name makes it more noticeable. Dropping the comma would shorten the pause a bit. Not blocking.
| await expect(input).toHaveCSS('border-top-color', 'rgb(255, 0, 0)'); | ||
| }); | ||
|
|
||
| test(`${fill} fill should not inherit --border-color from an ancestor`, async ({ page }, testInfo) => { |
There was a problem hiding this comment.
This covers behavior added in this PR rather than something reported in #31619, so I'd drop the issue annotation.
| await expect(inputOtp).toHaveScreenshot(screenshot(`input-otp-helper-text-wrapping`)); | ||
| }); | ||
|
|
||
| test('should not have visual regressions when rendering error text', async ({ page }) => { |
There was a problem hiding this comment.
The basic page shows error text under a description, but no screenshot covers that combination. Could this one include slotted Description text?
thetaPC
left a comment
There was a problem hiding this comment.
LGTM, left non-blocking comments
There was a problem hiding this comment.
Can we trim the white spacing around the component? I'm not seeing the need to have it.
| return this.el.querySelector('.input-otp-description')?.textContent?.trim(); | ||
| } | ||
|
|
||
| private getHintTextId(): string | undefined { |
There was a problem hiding this comment.
We really should just make this a util. We have a lot of form controls that use this. Could be a follow up.
|
|
||
| const input = page.locator('ion-input-otp input').first(); | ||
| await expect(input).toHaveCSS('border-top-color', 'rgb(255, 0, 0)'); | ||
| }); |
There was a problem hiding this comment.
:host(.ion-touched.ion-invalid)compiles to three classes, so it beatsion-input-otp.custom-borderand the invalid border stays visible over a custom--border-color, which seems right- Only the resting state is tested, so if the invalid border later moves onto
--internal-border-colorlike the fill defaults did, a custom--border-colorwould hide it and no test would fail
| }); | |
| }); | |
| test(`${fill} fill should keep the invalid border over a --border-color set on the host`, async ({ page }) => { | |
| await page.setContent( | |
| ` | |
| <style> | |
| ion-input-otp.custom-border { | |
| --border-color: rgb(255, 0, 0); | |
| --highlight-color-invalid: rgb(0, 0, 255); | |
| } | |
| </style> | |
| <ion-input-otp class="custom-border ion-touched ion-invalid" fill="${fill}"></ion-input-otp> | |
| `, | |
| config | |
| ); | |
| const input = page.locator('ion-input-otp input').first(); | |
| await expect(input).toHaveCSS('border-top-color', 'rgb(0, 0, 255)'); | |
| }); |
| await expect(page.locator('ion-input-otp .input-otp-group')).toHaveAttribute('aria-label', 'Code'); | ||
| }); | ||
|
|
||
| test('should describe the group, not each input, with the slotted description', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
Same for the annotations on "should forward aria-invalid and aria-describedby set on the host after load", "should clear aria-invalid and aria-describedby from the inputs when removed from the host", "should rename the inputs when the host aria-label changes", and "should restore the default labels when a host aria-label set after load is removed". #31619 reports attributes in the initial markup; changes after load are new behavior from this PR.
| await expect(page.locator('ion-input-otp .input-otp-group')).toHaveAttribute('aria-label', 'Code'); | ||
| }); | ||
|
|
||
| test('should describe the group, not each input, with the slotted description', async ({ page }, testInfo) => { |
There was a problem hiding this comment.
With VoiceOver, typing a digit moves focus to the next box while the digit is still being echoed, so "PIN, 2 of 4" gets cut off by the echo. It happened with "Input 2 of 4" too, but the longer name makes it more noticeable. Dropping the comma would shorten the pause a bit. Not blocking.
|
@ShaneK The PR description says |
Issue number: resolves #31619
What is the current behavior?
Currently, the boxes are always named "Input 1 of 4" in English, and host ARIA attributes go on the
role="group"wrapper instead of the inputs. There's no helper or error text, and--border-colorcan't override the fill defaults, so the border contrast can't be fixed without changing--ion-color-step-300.What is the new behavior?
inputLabelFormatter(index, length)for translated box names. A hostaria-labeloraria-labelledbyalso names the boxes ("PIN, 1 of 4").aria-describedby,aria-invalid,aria-errormessageandaria-requirednow go on the inputs.helperTextanderrorText, matchingion-input.--border-coloroverrides every fill and state, and isn't inherited from ancestors likeion-modal.Does this introduce a breaking change?
Other information
Focus is still shown by the border color, but it can be overwritten with a variable now. Apps can set the unfocused border with
--border-colorand the focused one with--highlight-color-focusedto reach 3:1.Overriding
--border-colorneeds a class selector (e.g.,ion-input-otp.custom). Docs: ionic-team/ionic-docs#4809a11y test page (iOS)
a11y test page (MD)