Commit 2d7033f
Polish CAML extract embed: keyboard nav, payload bounds, key fixes (#1234)
* PR #1177 follow-up: bound CAML extract embed payload, accessibility, polish
Addresses the review items in #1227:
- GET_EXTRACT_GRID_EMBED now passes `first: EXTRACT_GRID_EMBED_MAX_CELLS`
to fullDatacellList. ExtractType.full_datacell_list accepts an optional
`first` arg and slices the queryset after permission filtering, so
pathological extracts no longer transmit thousands of cells just to
trigger the row guard. New constants EXTRACT_GRID_EMBED_MAX_COLS and
EXTRACT_GRID_EMBED_MAX_CELLS make the cap explicit. Server-side
pagination is still tracked by #1204.
- resolveComponentMarker now receives the marker string as its React key
from both call sites (useCamlComponentRenderer and CamlDirectiveRenderer),
silencing the "missing key prop" warning when an article contains
multiple [component:...] blocks. Added regression tests in
camlComponents.test.ts.
- formatCellValue truncates JSON cell values on code-point boundaries
via Array.from(json) instead of String.substring, preventing
surrogate-pair corruption (U+FFFD) when cells contain emoji or
other non-BMP characters.
- The "Insert Extract Grid" picker in CamlArticleEditor now supports
full keyboard navigation: Arrow / Home / End move the focused option,
Enter selects, and Escape closes and returns focus to the trigger
button. Active option is reflected via aria-activedescendant and a
$active highlight on ExtractPickerItem.
- buildSourceLink: added an explanatory comment clarifying that
Annotation.page is 1-based and only used for the chip label — the
document viewer navigates by annotationId alone.
- Verified GET_EXTRACTS already selects fullDocumentList and that
CamlDirectiveRenderer still wires resolveImageSrc through to
MarkdownMessageRenderer (verification items, no code change required).
Closes #1227
* Address review: accessibility fixes, keyboard nav tests, datacell first-arg tests
- Move aria-activedescendant from listbox to trigger button per ARIA spec
- Replace outline:none with focus-visible ring on ExtractPickerItem
- Add ArrowUp wrap-around comment explaining WAI-ARIA intent
- Add console.warn for column-bound datacell cap in ExtractGridEmbed
- Add backend tests for resolve_full_datacell_list first argument
- Add keyboard navigation component tests for extract picker
- Add ComponentEmbedErrorFallback component test with docScreenshot
- Add GET_EXTRACTS mocks to CamlArticleEditorTestWrapper
* Address review: move warn to useEffect, optimize formatCellValue, cap datacell first
- Move console.warn from render body into useEffect to avoid side effects
during React renders (fires on every render and doubles in StrictMode)
- Add fast-path in formatCellValue to skip Array.from allocation when the
JSON string is already short enough in UTF-16
- Add MAX_DATACELL_FIRST constant (10,000) and enforce server-side cap in
resolve_full_datacell_list to prevent unbounded payloads from API clients
- Add test for server-side cap path
* Fix server-cap test to actually verify ceiling binds, document mock counts
- test_first_capped_at_server_maximum now patches MAX_DATACELL_FIRST to 2
(below the 3 fixture cells) so the assertion proves the cap actually
limits the result count, not just that the query succeeds.
- Add explanatory comment on CamlArticleEditorTestWrapper mock array
explaining why 4 article mocks and 3 extract mocks are needed.
* Fix datacell list tests: add M2M relation, superuser, correct mock target
The 6 failing ExtractFullDatacellListFirstArgTestCase tests had three
root causes:
1. Documents were never added to extract.documents M2M, so the resolver
iterated an empty set and returned no results (5 tests).
2. The test user lacked permissions to read documents through the
permission-filtering code path.
3. test_first_capped_at_server_maximum patched the wrong module --
MAX_DATACELL_FIRST is imported locally inside the resolver function,
so the patch target must be the source module
(opencontractserver.constants.annotations), not the consuming module.
Also moves the too-many-rows console.warn from a render-time side
effect into the existing useEffect in ExtractGridEmbed, addressing
review feedback about React Strict Mode double-invocation.
* Address review: harden ARIA, dev warnings, and test patch target
- Set aria-controls on trigger button only when listbox is rendered,
fixing ARIA spec compliance (references must point to existing DOM)
- Add event.stopPropagation() on Enter key handler to prevent event
leaking to parent Modal
- Guard useEffect dev warnings with useRef to fire only once per mount,
avoiding repeated console noise on re-renders / Apollo cache updates
- Hoist MAX_DATACELL_FIRST import to module level in extract_types.py
and patch at the resolver lookup site in the test, making the mock
resilient to future import reorganization
- Document omission of Enter-key insertion test (requires additional
GET_EXTRACT_GRID_EMBED mocks not yet in the test wrapper)
* Apply unconditional server-side cap on fullDatacellList resolver
Previously first<=0 bypassed MAX_DATACELL_FIRST entirely, allowing
any API client to retrieve unbounded payloads by sending first:0.
The resolver now always applies the ceiling: positive values are
honoured up to MAX_DATACELL_FIRST, and zero/negative/omitted values
fall back to the default cap.
Adds regression test (test_first_zero_still_capped_by_server_maximum)
and updates existing test names/docstrings to reflect the new semantics.
* Address review: fix file handle leaks, reset warning refs per extract
- Fix resource leak in test setUp: use context manager for PDF file reads
instead of bare open() that leaks file descriptors
- Reset useRef warning guards when extractId changes so each extract gets
its own dev warning instead of silencing warnings for subsequent extracts
- Add clarifying comments for $active/hover visual parity and useEffect
dependency rationale
* Address review: fix ContentFile reuse bug, document breaking default cap
Fix ContentFile reuse across loop iterations in
ExtractFullDatacellListFirstArgTestCase.setUp -- ContentFile wraps a
BytesIO whose pointer advances after Django's storage backend reads it,
so subsequent loop iterations produced empty PDF files. Now creates a
fresh ContentFile per iteration from pre-read bytes.
Add a Changed section to the CHANGELOG documenting that fullDatacellList
is now capped at MAX_DATACELL_FIRST (10,000) by default. Previously
unbounded, this is a breaking change for any caller that relied on
receiving all cells without passing an explicit `first` argument.
* Move aria-activedescendant to listbox element and fix dep-array comment
aria-activedescendant is only valid on composite roles (listbox, combobox,
grid, etc.) per WAI-ARIA 1.2, not on role="button". Move it from the
trigger button to the ExtractPickerDropdown (role="listbox") where it is
spec-compliant. The aria-selected attribute on each option and the visual
$active highlight continue to provide accessible selection feedback.
Also fix the contradictory useEffect dependency comment in
ExtractGridEmbed that said "[extract] is the real driver" while including
rows and columns in the array — the deps satisfy the exhaustive-deps lint
rule.
* Address review feedback: fix aria placement, improve test coverage
- Move aria-activedescendant from listbox to trigger button per WAI-ARIA spec
- Return focus to trigger after Enter key selection for consistent keyboard UX
- Export formatCellValue and add unit tests for all branches including emoji
- Fix coverage collection for ExtractGridEmbed and ComponentEmbedErrorFallback
CT tests (were importing directly instead of via coverage wrapper)
* Address review feedback: ARIA fix, string truncation, f-strings, cross-ref
- Move onKeyDown handler from wrapper div to trigger button in
CamlArticleEditor extract picker, so keyboard events and
aria-activedescendant share the same DOM focus context (WAI-ARIA
virtual-focus listbox pattern)
- Add code-point-safe truncation for raw string values in
formatCellValue (previously only objects were truncated), with
two new unit tests covering long strings and emoji
- Add cross-reference comment in frontend constants linking
EXTRACT_GRID_EMBED_MAX_CELLS to the backend MAX_DATACELL_FIRST cap
- Convert %-style string formatting to f-strings in
test_extract_queries.py to match project conventions
* Address review: extract truncation helper, fix picker close, add truncation TODO
- Extract duplicated Array.from truncation logic into reusable
truncateAtCodePoint() helper, used by both object and string branches
in formatCellValue()
- Add clarifying comment that Enter key handler already closes picker
via handleInsertComponent (which calls setShowExtractPicker(false))
- Add TODO(#1204) comment for silent column omission when datacell cap
is the binding constraint
- Add unit tests for truncateAtCodePoint()
* Address review: move utils to formatters, tabIndex fix, field description
- Move truncateAtCodePoint and formatCellValue from ExtractGridEmbed.tsx
to frontend/src/utils/formatters.ts per CLAUDE.md utility conventions.
Update imports in component and test files.
- Add tabIndex={-1} to ExtractPickerItem buttons so they stay out of the
tab order in the WAI-ARIA virtual-focus listbox pattern (WCAG 2.4.3).
- Improve fullDatacellList `first` field description to document zero/
negative/absent semantics and note this is not Relay cursor pagination.
- Add Apollo cache-key isolation comment near fetchPolicy for future
contributors (#1204).
* Fix ARIA: use role="combobox" on extract picker trigger for spec-compliant aria-activedescendant
The ARIA spec only allows aria-activedescendant on composite widget roles
(combobox, grid, listbox, etc.), not on role="button". Screen readers
silently ignore the attribute on buttons, breaking keyboard navigation
announcements. Switch the trigger to role="combobox" which is the
standard WAI-ARIA pattern for a button that opens a listbox with
virtual-focus management. Update component tests accordingly.
* Fix codecov config: rename file to files param, ignore test/config paths
* Improve patch coverage for extract grid and article editor
- Add istanbul ignore comments for dev-only warning code in
ExtractGridEmbed.tsx (NODE_ENV guard makes it unreachable in
production/test builds)
- Add two new component tests for CamlArticleEditor keyboard nav:
Enter key guard (no-op when no item focused) and mouse hover
highlighting (onMouseEnter updates active index)
- Add unit tests for extract grid constants validating the
MAX_CELLS formula and backend cap constraint
* Fix formatting in merge-resolved test file
* Fix aria-selected on hover in extract picker dropdown
Reserve aria-selected for the actually selected option, not the
keyboard/mouse-highlighted one. The visual highlight is already
handled by the $active styled-component prop.
* Fix CT tests and CHANGELOG accuracy for CAML extract picker
- Keyboard-nav CT tests now assert aria-activedescendant on the
combobox trigger rather than aria-selected on each option. This
matches the intentional ARIA design (aria-selected reserved for
actually-selected options; active option is surfaced via
aria-activedescendant + the $active styled-component highlight) so
all 10 CamlArticleEditor CT tests pass again.
- Correct CHANGELOG entries to match the actual constants:
MAX_FULL_DATACELL_LIST_LIMIT (500) in
opencontractserver/constants/extracts.py, mirrored on the frontend
as EXTRACT_GRID_EMBED_CELL_LIMIT (500). The previous entries cited
non-existent names / values (MAX_DATACELL_FIRST, 10_000,
EXTRACT_GRID_EMBED_MAX_CELLS, etc.).
* Address review: add ArrowDown+Enter happy-path test for extract picker
---------
Signed-off-by: JSIV <5049984+JSv4@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>1 parent 6c1d1fa commit 2d7033f
15 files changed
Lines changed: 781 additions & 65 deletions
File tree
- docs/assets/images/screenshots/auto
- frontend
- src
- assets/configurations
- components
- corpuses
- caml
- extracts
- __tests__
- hooks
- utils
- __tests__
- tests
- opencontractserver/tests
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
372 | 372 | | |
373 | 373 | | |
374 | 374 | | |
375 | | - | |
376 | 375 | | |
377 | 376 | | |
378 | 377 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
240 | 240 | | |
241 | 241 | | |
242 | 242 | | |
243 | | - | |
| 243 | + | |
244 | 244 | | |
245 | 245 | | |
246 | 246 | | |
247 | 247 | | |
248 | | - | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
249 | 251 | | |
250 | 252 | | |
251 | 253 | | |
252 | 254 | | |
253 | 255 | | |
254 | 256 | | |
255 | 257 | | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
256 | 262 | | |
257 | 263 | | |
258 | 264 | | |
| |||
364 | 370 | | |
365 | 371 | | |
366 | 372 | | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
367 | 376 | | |
368 | 377 | | |
| 378 | + | |
369 | 379 | | |
370 | 380 | | |
371 | 381 | | |
| |||
534 | 544 | | |
535 | 545 | | |
536 | 546 | | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
537 | 555 | | |
538 | 556 | | |
539 | 557 | | |
| |||
565 | 583 | | |
566 | 584 | | |
567 | 585 | | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
| 593 | + | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
| 602 | + | |
| 603 | + | |
| 604 | + | |
| 605 | + | |
| 606 | + | |
| 607 | + | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
| 611 | + | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
| 621 | + | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
| 636 | + | |
| 637 | + | |
| 638 | + | |
| 639 | + | |
| 640 | + | |
| 641 | + | |
| 642 | + | |
| 643 | + | |
| 644 | + | |
| 645 | + | |
| 646 | + | |
| 647 | + | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
568 | 660 | | |
569 | 661 | | |
570 | 662 | | |
| |||
596 | 688 | | |
597 | 689 | | |
598 | 690 | | |
| 691 | + | |
599 | 692 | | |
| 693 | + | |
| 694 | + | |
| 695 | + | |
| 696 | + | |
| 697 | + | |
600 | 698 | | |
601 | 699 | | |
| 700 | + | |
| 701 | + | |
| 702 | + | |
| 703 | + | |
| 704 | + | |
| 705 | + | |
| 706 | + | |
| 707 | + | |
| 708 | + | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
602 | 713 | | |
603 | 714 | | |
604 | 715 | | |
| |||
610 | 721 | | |
611 | 722 | | |
612 | 723 | | |
613 | | - | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
614 | 729 | | |
615 | 730 | | |
616 | 731 | | |
| |||
620 | 735 | | |
621 | 736 | | |
622 | 737 | | |
623 | | - | |
| 738 | + | |
624 | 739 | | |
| 740 | + | |
625 | 741 | | |
626 | 742 | | |
| 743 | + | |
627 | 744 | | |
| 745 | + | |
| 746 | + | |
| 747 | + | |
628 | 748 | | |
629 | 749 | | |
630 | 750 | | |
| |||
Lines changed: 3 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
148 | 148 | | |
149 | 149 | | |
150 | 150 | | |
| 151 | + | |
| 152 | + | |
151 | 153 | | |
152 | | - | |
| 154 | + | |
153 | 155 | | |
154 | 156 | | |
155 | 157 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
256 | 256 | | |
257 | 257 | | |
258 | 258 | | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
259 | 262 | | |
260 | 263 | | |
261 | 264 | | |
| |||
Lines changed: 91 additions & 19 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
2 | 6 | | |
3 | | - | |
4 | | - | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
5 | 34 | | |
6 | | - | |
| 35 | + | |
7 | 36 | | |
8 | 37 | | |
9 | 38 | | |
| |||
20 | 49 | | |
21 | 50 | | |
22 | 51 | | |
23 | | - | |
| 52 | + | |
24 | 53 | | |
25 | 54 | | |
26 | 55 | | |
27 | 56 | | |
28 | | - | |
| 57 | + | |
29 | 58 | | |
30 | 59 | | |
31 | 60 | | |
32 | | - | |
33 | | - | |
34 | | - | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
35 | 64 | | |
36 | 65 | | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
45 | 90 | | |
46 | | - | |
47 | | - | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
48 | 94 | | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
49 | 121 | | |
50 | 122 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
33 | 38 | | |
34 | 39 | | |
35 | 40 | | |
| |||
0 commit comments