Repository navigation
fix(spp_change_request_v2): keyboard-accessible search results in the CR create wizard (#580) - #581
Conversation
…le options (#580) The Create Change Request wizard renders its registrant search results server-side as plain table rows with a mouse click handler, and a pager made of <a> tags without href. Neither can take keyboard focus, so a keyboard or screen-reader user could type a search but never pick a result (WCAG 2.1.1, 4.1.2). Rows are now options of a listbox (role, roving tabindex, aria-label with name and type), the pager is made of real buttons whose out-of-range side carries a disabled attribute instead of muted styling, the decorative type icons are hidden from assistive technology, and the range summary / empty-result message carry a status class the widget mirrors into a live region. Nothing changes visually. Tests pin the rendered semantics and the two bridge behaviours the widget relies on (selected-partner onchange, page re-render and clamp), none of which were covered before.
…ion for the search results widget (#580) Enter/Space select the focused row, ArrowUp/ArrowDown/Home/End move between rows with a roving tabindex, and clicks keep working through one delegated listener instead of per-row handlers re-attached after every patch. Every search or page change re-renders the whole results blob, which destroys the focused node, so focus is put back on the pager button that was pressed (or the other one, or the first row) once the form has re-rendered; selecting a row hides the results block and unmounts the widget, so focus is handed to the "Change Registrant" button instead of dropping to <body>. A visually-hidden live region outside the re-rendered blob announces the result range and the empty-result message. The debounced search input now carries the field id so its "Search Registrant" label names it rather than the placeholder, and a focus ring is added for rows since the table-hover shading swallows the default one.
#580) The role locator proves role and accessible name, focus()/toBeFocused() proves the row is focusable, Enter exercises the keydown path instead of click, and the final assertion proves focus is handed to "Change Registrant" once the results disappear. Tests 16 and 17 keep the mouse path so both stay covered.
README.rst and static/description/index.html are left for CI's pinned generator, whose output is applied verbatim in a follow-up commit.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #581 +/- ##
==========================================
+ Coverage 77.04% 77.12% +0.08%
==========================================
Files 727 727
Lines 47165 47157 -8
==========================================
+ Hits 36338 36371 +33
+ Misses 10827 10786 -41
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…e pager arrows for the search results (#580) Review round. A listbox inside an exposed <table> left the header row as a table with no data cells, so the table is now role="presentation" and the <thead> aria-hidden (sighted users still see the headings). Each option carries aria-setsize/aria-posinset so its position is announced within the whole result set rather than within the page, the listbox is named "Registrant search results", and the pager arrows are hidden from assistive technology so the buttons read as "Previous" and "Next". Tests cover the new attributes, a group row (label and icon) and the decorative arrows; the HISTORY fragment describes the table change.
… region and hotkey hardening (#580) Review round. Focus after a page change was restored after a 500 ms timer that started before the onchange request was sent, so on a slow link it focused the old pager button just before the re-render removed it. The focus is now applied in onPatched when the results HTML actually changed, with a guarded fallback for an unchanged answer (page clamped to the last one). The live region moved out of the widget into the wizard form, where it exists before the first results arrive: a region inserted together with its text is not announced. The widget clears it before writing so an unchanged range text is announced again. Keys with Alt/Ctrl/Meta are left to the browser and to Odoo's hotkeys (Ctrl+Enter is the dialog's footer-button hotkey) and handled keys stop propagating. The roving tabindex follows focus that arrives by mouse or from assistive technology (focusin), not only the arrow keys. The focus ring did not paint at all: --bs-primary does not exist in Odoo 19's backend (Bootstrap is compiled without the prefix), which made the whole declaration invalid. It now uses --primary with a literal fallback, the cells carry the ring as a box-shadow because outlines on <tr> are unreliable under border-collapse, and the pager buttons get the same ring. The search box also gets autocomplete="off".
…gation in test 18 (#580) Associating the "Search Registrant" label changed the input's accessible name, so the three locators that used the placeholder would no longer match. Test 18 now also proves that Tab from the search box reaches the list and that ArrowDown/Home move focus, and matches the option name exactly.
#580) The dispatched E2E run hung in test 16 at getByRole("cell", …): a cell inside a tr role="option" is presentational to the accessibility tree, so the locator never resolved. Both mouse-path tests now click the row by its option name, as test 18 does.
…a search request (#580) Restoring focus after a page change, or handing it to "Change Registrant" after a selection, now happens only when focus was lost with the removed results (it sits on body) or still sits inside them. A user who pressed Next and went back to typing in the search box before the response arrived keeps the search box; a user who moved to another field while a slow selection was in flight keeps that field. Verified with 1.5 s emulated latency. The results HTML is compared as text rather than by object identity, so a Markup instance handed out anew for an unchanged value does not count as a re-render. A test pins that quotes and angle brackets in a registrant's name round trip through the aria-label, data-partner-name and the cell unchanged.
…rowse-mode activation, translatable search strings (#580) Second review round, verified on a live stack. A screen reader in browse mode activates a row without moving system focus, and a user may click into the search box while a slow selection is in flight; in both cases the search box disappears with the results once a registrant is set. Whether focus was "moved by the user" is now decided only after the form has re-rendered, against the element that had focus when the action started, so focus is handed to "Change Registrant" whenever it would otherwise land on body, and left alone when the user is in a field that survives. The selected-registrant card no longer carries an alert role: an assertive alert inserted in the same frame as the focus move is cut off by that move. The choice is confirmed instead through the live region ("Selected: NAME, Individual") after focus has settled, and the region now survives a selection. The request-type hint is a status rather than an alert so it does not interrupt on every change of the type field, and the region is cleared when the results go away. The pager buttons are named "Previous/Next page of results" so a bare "Next" is not taken for a wizard step, the card icons are hidden from assistive technology like the row icons, and every string the search and the card render is wrapped in _(). Tests: a CR requestor (role, record rules in force) gets the same results and can select; pager accessible names.
There was a problem hiding this comment.
Thank you for the PR @gonzalesedwin1123
I posted reviews on all changed files. The ARIA semantics, XSS handling, event delegation, and focus management are all correct and well-tested. Three items below — one medium, two low. The medium one (stale requestAnimationFrame) is the only correctness issue; the other two are hardening.
| if (text) { | ||
| requestAnimationFrame(() => { | ||
| region.textContent = text; | ||
| }); |
There was a problem hiding this comment.
[Medium] Stale requestAnimationFrame — race between clear and write
If _announce("1-10 of 23") is called (e.g. from onPatched → _announceStatus) and then _announce("") runs before the first rAF fires (e.g. from onWillUnmount), the clear in the second call happens synchronously but the pending rAF from the first call still executes and writes stale text back into the live region.
The consequence is that the live region shows "1-10 of 23" after the widget has unmounted, when it should be empty.
Suggested fix — track and cancel the pending rAF:
_announce(text) {
const region = this.formEl && this.formEl.querySelector(LIVE_REGION_SELECTOR);
if (!region) {
return;
}
if (this._announceRaf) {
cancelAnimationFrame(this._announceRaf);
}
region.textContent = "";
if (text) {
this._announceRaf = requestAnimationFrame(() => {
region.textContent = text;
this._announceRaf = null;
});
}
}There was a problem hiding this comment.
Confirmed and fixed in dc61c92. _announce now keeps the frame id and cancels a pending frame before clearing, and onWillUnmount clears the mount timer as well.
Reproduced first with a browser probe that holds requestAnimationFrame callbacks instead of running them (Owl keeps its own bound copy of the function, so rendering is unaffected). Before the fix, flushing the held frame after a selection wrote "11-20 of 787" back into the emptied live region (cancelled 0); after the fix the region stays empty (cancelled 1) and the "Selected: …" confirmation still follows. The same check now lives in e2e test 18, around the keyboard selection.
| el.addEventListener("focusin", (ev) => this._onFocusin(ev)); | ||
| this.renderedHtml = this.htmlContent; | ||
| this.formEl = el.closest(".o_form_view"); | ||
| setTimeout(() => this._announceStatus(), MOUNT_ANNOUNCE_DELAY_MS); |
There was a problem hiding this comment.
[Low] Uncanceled setTimeout on mount
If the component unmounts within 150 ms (e.g. the user picks a pre-filled registrant immediately), this timer fires after onWillUnmount. The _announceStatus method guards against a null containerRef.el so it is a no-op, but it is still a dangling timer referencing a destroyed component.
Suggested fix — store the timer ID and clear it in onWillUnmount:
// in onMounted:
this._mountTimer = setTimeout(
() => this._announceStatus(), MOUNT_ANNOUNCE_DELAY_MS
);
// in onWillUnmount:
onWillUnmount(() => {
clearTimeout(this._mountTimer);
this._announce("");
});There was a problem hiding this comment.
Done in dc61c92: the timer id is stored on mount and cleared in onWillUnmount. The probe for the frame race also exercises this path (a selection right after mount): the only frame still held afterwards is the focus helper's, so the timer no longer announces once the widget is gone.
| _("%(start)s-%(end)s of %(total)s", start=start, end=end, total=total), | ||
| _("Previous page of results"), | ||
| page - 1, | ||
| " disabled" if page == 0 else "", |
There was a problem hiding this comment.
[Low] disabled attribute injected as a plain string into Markup.format()
This works today because " disabled" contains no HTML-special characters, so Markup.format()'s auto-escaping passes it through unchanged. But it is fragile: if someone later changes this to a value attribute (e.g. ' disabled="disabled"'), the quotes would be escaped and produce broken markup.
The more idiomatic pattern is to mark the fragment as trusted:
Markup(" disabled") if page == 0 else Markup(""),(Same applies to the " disabled" on line 365.)
Non-blocking — just a hardening suggestion for a follow-up.
There was a problem hiding this comment.
Taken in this PR rather than a follow-up, since it is still open: both fragments are Markup(" disabled") / Markup("") in b9ad69b. The rendered output is unchanged and test_pager_renders_buttons_with_disabled_edges still passes.
…er when the results unmount (#580) _announce clears the live region and writes the new text a frame later, but never cancelled a frame still pending from an earlier call. When the widget unmounted between a range announcement and its frame (a selection right after a page change, or right after mount), the unmount's clear ran first and the frame then wrote the stale range back into a region that should be empty. The frame id is kept and cancelled by the next call, and the 150 ms mount timer is cleared on unmount so it cannot announce after the widget is gone. e2e test 18 now holds requestAnimationFrame callbacks across the keyboard selection (Owl keeps its own bound copy, so rendering is unaffected) and checks that releasing them does not write the range after the results are gone, then that the selection is still confirmed. Review finding by aldnav on #581.
…ted markup (#580) The " disabled" fragment went through Markup.format() as a plain string and only survived because it holds no HTML-special characters. Wrapping it in Markup states the intent and keeps a future attribute with quotes from being escaped into broken markup. Review finding by aldnav on #581.
|
Review round applied (aldnav, 2026-10-06), head b9ad69b:
Verification: spp_change_request_v2 431/0/0 locally; the 18 browser probes from the original round still pass; a new 6-probe race script (held frames) fails on 1a4a831 and passes on dc61c92. Lint clean on the changed files. The e2e change is verified by CI. |
|
@aldnav can you check again before I merge this. |
There was a problem hiding this comment.
All three findings from the first round are addressed. The cancelAnimationFrame fix, the clearTimeout cleanup, and the Markup(" disabled") hardening are all correct. The new e2e regression test for the rAF race is well-designed — OWL's scheduler uses its own bound copy of requestAnimationFrame (captured at module load), so the held-frames helper only intercepts the widget's calls, and the test precisely proves the stale-announcement race is gone.
One non-blocking note: if the e2e test fails between holdAnimationFrames() and releaseAnimationFrames(), window.requestAnimationFrame stays permanently replaced for subsequent serial tests. A defensive afterEach cleanup would make failure diagnostics cleaner:
test.afterEach(async ({page}) => {
await page.evaluate(() => {
const held = (window as any).__heldFrames;
if (held) {
window.requestAnimationFrame = held.request;
window.cancelAnimationFrame = held.cancel;
delete (window as any).__heldFrames;
}
});
});Not blocking — the serial tests are state-dependent and would cascade on failure anyway.
Thanks a lot @gonzalesedwin1123
A failure between holdAnimationFrames and releaseAnimationFrames left the stub installed for the rest of the serial suite, so later tests would fail for the wrong reason. The existing afterEach now puts the real functions back, guarded so a closed or navigating page cannot mask the test's own error. Non-blocking note from aldnav's approval of #581.
|
Thanks @aldnav. Took the non-blocking note as well, in 52612d5: the suite's existing |
aldnav
left a comment
aldnav
left a comment
There was a problem hiding this comment.
Thanks @gonzalesedwin1123 and OpenSPP Team!
2 participants
Summary
Closes #580.
The Create Change Request wizard renders its registrant search results server-side as plain
<tr>s with a mouseonclick, and a Previous/Next pager made of<a>tags withouthref. Nothing in it can take keyboard focus, so a keyboard or screen-reader user can type a search but never pick a result (WCAG 2.1.1 Keyboard, 4.1.2 Name/Role/Value, both Level A).This keeps the inline results table exactly as designed (see the discussion on #580 — the previous
Many2onepicker is not coming back) and makes it conform:role="option", rovingtabindex(one Tab stop for the list),aria-label= "NAME, Individual|Group". Enter/Space select, ArrowUp/ArrowDown/Home/End move between rows.<button type="button">s; the out-of-range side carries adisabledattribute instead of muted styling, so it is skipped in the Tab order and cannot be clicked.t-outblob (destroying the focused node), so focus is put back on the pager button that was pressed once the form has re-rendered. Selecting a row hides the results block and unmounts the widget, so focus is handed to Change Registrant.role="status"node outside the re-rendered blob announces the result range ("1-10 of 23") and "No registrants found."id="props.id", so its "Search Registrant"<label>names it (previously the accessible name fell back to the placeholder). Type icons getaria-hidden="true". A:focus-visiblering is added for rows because the table-hover shading swallows the default one.onclickre-attached after every patch to one delegated listener, which is what lets the keyboard path share the click path.No model, view-structure, ACL or UX change. Mouse users see the same table; the only visible difference is that Previous/Next are now buttons styled as links.
README.rst/static/description/index.htmlare deliberately not regenerated locally; CI's pinned generator prints the expected diff, which will be applied verbatim in a follow-up commit.Test plan
./spp t spp_change_request_v2: 0 failed of 427 (420 baseline + 7 new intests/test_create_wizard_search.py). The 5 rendering tests fail onorigin/19.0and pass here; the bridge/paging tests guard behaviour the widget relies on that had no coverage.getByRole("option", {name: "SANTOS, JOSE MIGUEL, Individual"})→focus()→toBeFocused()→Enter→Change Registrantis focused. Tests 16/17 keep the mouse path. (Runs in the E2E workflow.)Review round (code + UX/a11y reviewers)
<table role="presentation">,<thead aria-hidden="true">— otherwise the header row is a table with no data cells next to the listbox..o_cr_search_live), present before results arrive; a region inserted together with its text is not announced. It is cleared before each write so an unchanged range is announced again.onPatchedwhen the HTML actually changed, with a guarded fallback for an unchanged answer.focusin.aria-setsize/aria-posinset(position within the whole result set), listbox named "Registrant search results", pager arrowsaria-hidden,autocomplete="off"on the search box.--bs-primarydoes not exist in Odoo 19's backend (Bootstrap is compiled without the prefix), so the declaration was invalid. Nowvar(--primary, #714b67), with a<td>box-shadow fallback (outlines on<tr>are unreliable underborder-collapse) and the same ring on the pager buttons. Verified computed:outline: solid 2px rgb(93, 141, 168).Smoke run against a local MIS demo stack (787 hits for "an"): 15/15 keyboard checks pass — label name, live region before first search, Tab → Next → first row, ArrowDown/Home/End, roving tabindex, Next/Previous by keyboard with focus kept on the pager and "11-20 of 787" announced, Enter → "Change Registrant" focused, mouse click unchanged, "No registrants found." announced.
Not done, flagged for the maintainers: there is no JS unit-test runner in this repo (only tours), so the widget's keyboard logic is covered by the Python rendering tests and the Playwright spec rather than hoot tests. Pre-existing, out of scope: untranslated strings in the renderer; focus after "Change Registrant" reopens the dialog; no "Searching…" status while the RPC is in flight.
E2E workflow (dispatched on this branch, since it does not run on PRs)
getByRole("cell", {name: "SANTOS, JOSE MIGUEL"})— a cell inside atr role="option"is presentational to the accessibility tree, so the locator never resolved. Tests 16/17 now click the row by its option name like test 18.Full-flow UI check on a local MIS demo stack (visible Chromium): 25/25 — Edit Individual by keyboard end to end incl. Create opening the detail form, Edit Group by mouse incl. pager, type switching clears the search, exact ID-number search (partial does not match), 1-char and no-hit searches, Create New Group hides the picker; zero console/page errors, zero Odoo error dialogs.
Third check round
aria-label,data-partner-nameand the cell.status+button "Previous"/"Next"+listbox "Registrant search results">option "NAME, Individual"with no table/rowgroup/cell nodes; Tab order search → Next → one row → Cancel and back with Shift+Tab; roving tabindex follows arrow keys and programmatic focus; Space selects without scrolling; Ctrl+Enter on a row does not select; the live region is cleared then refilled so an equal range is re-announced.Fourth round (second reviewer pass, verified empirically on the live stack)
role="alert"(an alert inserted in the same frame as a focus move is cut off); the choice is confirmed through the live region ("Selected: NAME, Individual") after focus settles, and the region now survives a selection. The request-type hint isrole="status"so it no longer interrupts on every type change; the region is cleared when the results go away.aria-hidden; every string the search and the card render wrapped in_().Record.update()resolves after the onchange is applied; html values areMarkupobjects and the same instance is kept for unchanged values, so the text comparison is a safety net, not a requirement.