LYT-1120: give widget dialogs resolving accessible names and announce form states - #676
Open
ashyablok-cs wants to merge 2 commits into
Open
ashyablok-cs wants to merge 2 commits into
ashyablok-cs wants to merge 2 commits into
Conversation
Addresses defects 1 and 2 of #675. Defect 1: form/slideout, subscription/slideout and subscription/bar were not covered by #673 and still rendered with no pf-widget-container, no role and no accessible name. Wrap them the way the message equivalents are wrapped. Defect 2: the aria-labelledby/aria-describedby added in #673 pointed at ids that only existed as class names, so neither reference resolved. Rather than add more static ids, make the id and reference pairing a single JS step and drop both from the templates. setupWidgetAria hands out ids from a counter so that widgets open at the same time cannot collide - the static ids were already a duplicate id bug on any page showing two dialogs - and only sets a reference when the element it points at will hold text, so an empty headline no longer names a dialog with an empty string. Bar layouts have no headline element at all, so their message names the dialog instead. Ids come from a counter rather than the widget id on purpose: prefixing with config.id made pathfora's own [id*="ab-widget"] selectors match the inner elements, and customer code could do the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses defect 3 of #675. A form state is revealed by CSS alone, which is silent, so a screen reader user submitted a lead capture form and heard nothing. Three things were wrong at once: no live region or focus change marked the swap, the dialog's aria references stayed pinned to the original headline and message (still contributing their text through the reference even though the CSS had set them to display: none), and the button the user just activated became display: none, dropping focus to <body>. Dialog layouts now rename the container after the state's own headline and message and take focus, which is what gets it read out. Inline widgets sit in the page's own flow and should not steal focus, so their state is marked role=status and announced politely instead. The modal and gate focus trap recomputed: it captured its set of focusable elements once at open time, so after a state swap Tab called focus() on hidden elements and focus went nowhere. It now filters to elements that are actually rendered, on every tab. Aria reference ids are namespaced under the widget id rather than handed out from a counter. Pathfora already rejects duplicate widget ids, so this is unique, and the form and its states each get their own namespace. Note that a substring match on a widget id - [id*="my-id"] - now also matches the headline and message inside it; the A/B specs did this and are scoped to .pf-widget as a result. Also in this area: - drop transition: opacity from the state classes. Nothing about the swap changes opacity, but declaring a transition on the widget root overrode .slide-transition(), costing slideouts and bars their slide-out animation when the state delay closed them. - scope the showDelay focus call to its own widget. It used a bare document.querySelector('.pf-widget-ok'), which focuses whichever widget comes first in the document and throws outright when the widget was configured with okShow: false. Not covered here: field validation failures are a separate path that returns silently after adding CSS classes, and announcing those needs new user facing copy. Filed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes all three defects in #675 (LYT-1120 / ZD 22915, Clorox). Two commits, one per
concern —
545bb25covers defects 1 and 2,440e4f0covers defect 3.#673closed #670 by adding dialog semantics to themessageslideout and bar. It didnot deliver a working accessible name on any layout it touched, and it did not reach the
formandsubscriptionlayouts at all — which is why Clorox retested v1.2.21, sawtheir "present message" Experiences improve, and their "capture lead" Experiences still
read out as nothing.
Defect 1 — form and subscription layouts had no dialog semantics
src/templates/form/slideout.html,subscription/slideout.htmlandsubscription/bar.htmlrendered with nopf-widget-container, noroleand noaccessible name. Each is now wrapped the way the
messageequivalents already are.This changes the DOM shape of three templates, so it is the riskiest part of the diff.
Checks done before making it:
.pf-widget-containeris only styled under.pf-widget-modal/.pf-widget-gate, no LESS rule in the slideout or bar paths uses achild selector, and nothing in
src/rollup/**traverses the widget root's directchildren. All three were then rendered in Chrome against the built bundle — layout
unchanged.
Defect 2 — the aria references did not resolve
message/slideout.htmlandmessage/bar.htmlcarriedaria-labelledby="pf-widget-headline", but no element had that id — the stringexisted only as a class name. Both references dangled.
baradditionally pointed at aheadline element that does not exist in that layout.
Rather than scatter more static ids through the templates, the id and reference pairing
is now a single JS step —
describe-widget-container.js, called fromsetup-widget-aria.js— and both the static ids and the template-levelaria-labelledby/aria-describedbyare gone from all 15 templates. The helper:collide. The old static ids were already a duplicate-id bug on any page showing two
dialogs.
headline(the default) no longer names a dialog with an empty string.
barby its message, since that layout has no headline element to point at.Reviewer note. Because ids are namespaced under the widget id, a substring match on a
widget id —
[id*="my-widget"]— now also matches the headline and message inside it.The A/B specs did exactly this and are scoped to
.pf-widget[id*=…]as a result. Anycustomer code doing substring or prefix id matching would see the same extra hits. An
earlier revision used an opaque counter to avoid this; the widget id was chosen instead
because pathfora already rejects duplicate widget ids, and it is far easier to debug.
Defect 3 — form states were not exposed after submit
A form state is revealed by CSS alone (
.pf-widget.successhides the headline, messageand form, then re-shows the ones inside
.success-state), which is silent. Three thingswere wrong at once:
still contribute their text through the reference even though the CSS had set them to
display: none— so the dialog kept reporting "Join our list" while the screen showed"Thanks!";
display: none, dropping focus to<body>.Dialog layouts now rename the container after the state's own headline and message and
take focus, which is what gets it read out and keeps a keyboard user from being dumped at
the top of the page. Inline widgets sit in the page's own flow and should not steal focus,
so their state is marked
role="status"and announced politely instead.A live region was deliberately not used on the dialog layouts: pairing one with a
focus move risks the same text being spoken twice, and focusing a dialog that has a name
and a description has better screen-reader support than relying on a
display:none→visible toggle being noticed.
Focus trap
show-widget.jscaptured its set of focusable elements once, at open time. After a stateswap that set still pointed into the hidden form, so Tab called
.focus()ondisplay: noneelements and focus went nowhere — a keyboard user was stranded. It nowrecomputes on every Tab, filtered to elements that are actually rendered
(
getClientRects().length > 0, which is correct for theposition: fixedmodal contentwhere
offsetParentis not). The handler also read the globaleventinstead of its ownev; fixed while rewriting the same expression.Also in this area
transition: opacitydropped from the state classes. Nothing about the state swapchanges opacity, but declaring a transition on the widget root overrode
.slide-transition()— costing slideouts and bars their slide-out animation on theauto-close that fires a few seconds later, undoing part of LYT-1004: restore slideout and bar open/close animations #672.
showDelayfocus call scoped to its own widget. It used a baredocument.querySelector('.pf-widget-ok').focus(), which focuses whichever widget comesfirst in the document and throws outright when the widget was configured with
okShow: false.Verification
test/acceptance/accessibility.spec.jsis new: 22 tests covering all 12 dialogtype/layout combinations, the empty-headline and empty-message cases, two widgets open at
once, the success and error state re-description, focus landing on the container, the
inline live region, and Tab not reaching hidden elements. Each aria reference is resolved
against the whole document, so a dangling or duplicated id fails the test.
Suite is at 300 passing, stable across three consecutive runs (
developbaseline is278). Every new test was confirmed to fail against the unmodified source first.
Rendered in Chrome against the built bundle: every dialog reports
role=dialogwithresolving references; submitting a form slideout, modal, gate and inline widget gives the
expected name, description,
role="status"and focus target in each case; Tab after agate's success state lands on the visible button; and a slideout showing a success state
keeps its full
translate, opacity, visibilitytransition.Two caveats worth knowing:
references resolve and focus moves; they do not prove NVDA or VoiceOver says the right
thing. Screen readers still cannot read Experience content after #673 (form, subscription, and form-state cases) #675 carries the same caveat.
stylesheet. pathfora async-loads
https://c.lytics.io/static/pathfora.min.cssatruntime, and that production stylesheet wins the cascade over a local build — it
initially made the animation fix look like it had not worked. Disable that sheet when
checking CSS locally.
dist/is rebuilt withNODE_ENV=production gulp build, consistent with how #672 and#673 shipped.
Not in scope
Filed separately rather than folded in here:
widgetFormValidateaddsinvalid/bad-validationclasses andreturns — no announcement, noaria-invalid, and thefocus()calls are guarded oni === 0so they focus the first field in the listrather than the first failing one. A spoken message needs new default user-facing copy,
which is a product decision. Worth noting the
errorformState only ever fires forconfirmAction.waitForAsyncResponse, so a server error is the only error a form cancurrently report — the far more common validation error reports nothing.
barlayouts never get state divs.construct-widget-layout.jsbuilds them formodal/slideout/gate/inlineonly, so a bar withformStateshides its contentand shows nothing for 3s.
button-action.js:38,55are no-op statements (callbackTypes.MODAL_CANCEL;with noassignment), so every confirm and cancel callback — form states or not — receives
undefinedas its first argument..pf-widget-container:focus { outline: none; }rule is the one line I would mostlike a second opinion on. Suppressing an outline normally deserves an argument; it is
defensible here only because the container is focused programmatically and never
reachable by tabbing, and Chrome and Firefox would not draw a ring on it anyway.
🤖 Generated with Claude Code