GSA / GSA/sam-ui-elements

disablePageTabIndex() sets aria-hidden on the focused calendar button, and enablePageTabIndex() never removes it

Open
#682 0 comments 0 reactions 0 assignees View on GitHub
bugfix
Dominant language
TypeScript
Stars
8
Forks
33
Avg merge
2d 51m
Merged PRs (30d)
65

Description

`DatepickerComponent.disablePageTabIndex()` stamps `tabindex="-1"` and `aria-hidden="true"` onto **every** focusable element on the page when the calendar opens — including the datepicker's own calendar button, which the user has just clicked and which the browser has focused. Chrome rejects the attribute and logs:

```
Blocked aria-hidden on an element because its descendant retained focus. The focus must
not be hidden from assistive technology users. [...] Consider using the inert attribute
instead, which will also prevent focus.
Element with focus:
Ancestor with aria-hidden:
```

Reproduced in Chromium on the `/datepicker` gallery route (added in #666), immediately after a real click on the calendar icon:

```json
{
"activeElement": "",
"iconAria": "true",
"iconTabindex": "-1"
}
```

## Cause

The selector matches the component's own subtree:

```ts
_focusableString: string =
'a[href], area, button, select, textarea, *[tabindex], input:not([type="hidden"])';
```

`picker.template.html` gives the calendar button `tabindex="0"`, so `*[tabindex]` selects it. `disablePageTabIndex()` then walks `document.querySelectorAll(this._focusableString)` with no exclusion for `this` component, hiding the trigger the user is standing on. The masked input inside `sam-input-mask` is hidden for the same reason.

## Second defect in the same pair of methods

`enablePageTabIndex()` does not restore the prior `aria-hidden` state — it unconditionally sets `aria-hidden="false"`:

```ts
el.removeAttribute("data-sam-tabindex");
el.setAttribute("aria-hidden", "false"); // should be removeAttribute
```

`tabindex` is correctly restored (`data-sam-noinitial-tabindex` tracks elements that had none and removes the attribute), but `aria-hidden` has no equivalent bookkeeping. So every focusable element on the page is left permanently carrying `aria-hidden="false"` after the first open/close cycle — DOM pollution that also destroys any `aria-hidden` an element legitimately had before the calendar opened.

Note `test-app/e2e/datepicker.spec.ts` currently **asserts** `aria-hidden="false"` as the restored state, with a comment explaining the `tabindex` asymmetry. Fixing this requires updating those assertions to expect the attribute to be absent.

## Suggested direction

- Exclude the component's own subtree when disabling — e.g. skip any element where `this.wrapper`/host `.contains(el)`, or narrow `_focusableString` and handle the trigger separately.
- Record prior `aria-hidden` alongside `data-sam-tabindex` (mirroring `data-sam-noinitial-tabindex`) and remove vs. restore accordingly.
- Chrome's own suggestion — the `inert` attribute on a wrapper — would replace both loops with one attribute and is worth evaluating, though it changes the DOM contract these two methods expose.

## Scope note

These methods are `public` on `DatepickerComponent` and are the general page-locking mechanism for this component, so the fix is not datepicker-cosmetic — it affects the a11y of any page that opens the calendar. Independent of #681 (missing CSS): this reproduces regardless of stylesheets.

## Acceptance criteria

- [ ] Opening the calendar does not set `aria-hidden="true"` on the calendar button or any other element inside the datepicker
- [ ] No `Blocked aria-hidden on an element because its descendant retained focus` warning in the browser console on open
- [ ] Closing the calendar removes `aria-hidden` from elements that did not have it beforehand, rather than setting `"false"`
- [ ] An element that legitimately had `aria-hidden="true"` before the calendar opened still has it afterwards
- [ ] `test-app/e2e/datepicker.spec.ts`'s restore assertions are updated accordingly (they currently expect `aria-hidden="false"`)
- [ ] Playwright coverage asserts the console produces no `aria-hidden` violation — this is a real-focus/real-browser behaviour jsdom cannot reproduce, per AGENTS.md's Vitest/Playwright boundary

## Context

Surfaced while manually testing #666 on the new `/datepicker` route. Pre-existing and unrelated to that PR's two-line fix (`git diff` on `picker.component.ts` touches only the `@ViewChild` decorator and the `contains()` comparison); kept separate to kept #666 scoped.

Contributor guide

Open the contributing guide

Research direction

Start in the DatepickerComponent page-locking methods in picker.component.ts, then run the datepicker Playwright coverage in test-app/e2e/datepicker.spec.ts and review AGENTS.md for the Vitest/Playwright boundary. The change is done when the calendar subtree is not hidden, prior aria-hidden states are preserved, restored attributes are absent when appropriate, and real-browser console coverage reports no violation.

Written by the indexing model from the issue text.

Assessment

Tech stack
playwright, typescript
Domain
accessibility, frontend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.