diff --git a/.changelog/next/changed-issue-4170.md b/.changelog/next/changed-issue-4170.md index 110a96a860..d3c9f08ac4 100644 --- a/.changelog/next/changed-issue-4170.md +++ b/.changelog/next/changed-issue-4170.md @@ -1 +1 @@ -- CollapsibleText gained a children-based max-height variant for content CSS line-clamp cannot clamp; the CoS agent card's task description now uses it (measured overflow + aria-expanded/aria-controls) instead of its own character-count heuristic +- CollapsibleText gained a children-based max-height variant for content CSS line-clamp cannot clamp; the CoS agent card's task description now uses it (measured overflow + aria-expanded/aria-controls) instead of its own character-count heuristic, and keyboard focus landing on a link inside a clipped preview now expands it rather than scrolling it out of reach diff --git a/client/src/components/ui/CollapsibleText.jsx b/client/src/components/ui/CollapsibleText.jsx index dff8e4079d..84d54eed62 100644 --- a/client/src/components/ui/CollapsibleText.jsx +++ b/client/src/components/ui/CollapsibleText.jsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect } from 'react'; +import { Children, useState, useRef, useEffect } from 'react'; import { ChevronDown, ChevronUp } from 'lucide-react'; // Tailwind scans source for literal class names, so the clamp variants must @@ -55,7 +55,11 @@ const CLAMP_CLASS = { * never change its box and a resize callback bound to it alone would never fire; * the inner wrapper is uncapped, so its height tracks the real content and a * changed child re-measures without needing `children` (a fresh element object - * every render) in the effect deps. + * every render, which would churn the observer on every parent re-render) in the + * effect deps. Switching *modes* still has to re-run it, though — the rendered + * element swaps between `

` and the capped `

`, so the effect's captured + * element would otherwise stay bound to the detached one and never measure. + * That's what `hasChildren` (a stable boolean) is doing in the deps. * * Two separate guards keep the toggle from vanishing mid-expand (which would * strand the user in the expanded wall of text with no way back): the effect @@ -69,6 +73,11 @@ const CLAMP_CLASS = { * * `id` is required: it wires the toggle's `aria-controls` to the content it expands. */ + +// The 1px slack absorbs sub-pixel line-height rounding, which otherwise reports +// a phantom overflow on content that fits exactly. +const overflows = el => el.scrollHeight > el.clientHeight + 1; + export default function CollapsibleText({ id, text, @@ -84,13 +93,24 @@ export default function CollapsibleText({ const [isOverflowing, setIsOverflowing] = useState(false); const ref = useRef(null); const innerRef = useRef(null); - const hasChildren = children != null && children !== false; + // An empty list or an empty string is *no* children, not "children that + // happen to be blank" — a caller doing ` + // {items.map(…)}` over an empty list must get the text + // fallback, not an empty capped box with its `text` silently dropped. + // Array *length* is the wrong signal: `items.map(i => i.show ? : null)` + // over an all-hidden list yields `[null]` — length 1, renders nothing. + // `Children.toArray` drops exactly those non-rendering placeholders (`null`, + // `undefined`, booleans) and flattens nested arrays; empty/whitespace strings + // survive it, so they're filtered here too. + const hasChildren = Children.toArray(children).some( + child => typeof child !== 'string' || child.trim() !== '' + ); useEffect(() => { if (expanded) return; const el = ref.current; if (!el) return; - const measure = () => setIsOverflowing(el.scrollHeight > el.clientHeight + 1); + const measure = () => setIsOverflowing(overflows(el)); measure(); if (typeof ResizeObserver === 'undefined') return; const observer = new ResizeObserver(measure); @@ -100,6 +120,7 @@ export default function CollapsibleText({ }, [text, hasChildren, expanded]); const clamp = CLAMP_CLASS[lines] || CLAMP_CLASS[2]; + const renderContent = () => { if (hasChildren) { return ( @@ -108,6 +129,18 @@ export default function CollapsibleText({ id={id} className={`break-words ${className} ${expanded ? '' : 'overflow-hidden'}`} style={expanded ? undefined : { maxHeight }} + // `overflow-hidden` is a scroll container with no visible scrollbar, + // and children may hold links, buttons or a horizontally-scrollable + //
. Tabbing to one below the cap would scroll the preview to
+          // reveal it with no way to scroll back — and `aria-expanded="false"`
+          // would be a lie, since the content is fully in the tab order.
+          // Expanding on focus keeps the claim honest and the target on screen.
+          // Gated on real overflow: content that fits is never clipped, so
+          // expanding it would only mint a no-op "Show less" into the tab order.
+          // Measured live rather than read off `isOverflowing`: a descendant with
+          // `autoFocus` takes focus during commit, before the passive measure
+          // effect has run, so the state flag is still false on that first focus.
+          onFocus={() => { if (ref.current && overflows(ref.current)) setExpanded(true); }}
         >
           
{children}
diff --git a/client/src/components/ui/CollapsibleText.test.jsx b/client/src/components/ui/CollapsibleText.test.jsx index 242287b5ab..89597ff742 100644 --- a/client/src/components/ui/CollapsibleText.test.jsx +++ b/client/src/components/ui/CollapsibleText.test.jsx @@ -237,6 +237,106 @@ describe('CollapsibleText children (max-height) variant', () => { expect(observed).toContain(box.firstElementChild); }); + it('measures children that arrive after mount', () => { + // Switching modes swaps the rendered element from

to the capped

, + // so an effect that doesn't re-run stays bound to the now-detached

— + // which has no layout and never gets a resize callback. The content would + // clip at maxHeight with no Show more affordance at all. + const spy = forceOverflow(); + spy.mockReturnValue(0); + const { rerender } = render({null}); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + + spy.mockReturnValue(500); + rerender(

tall rendered markdown

); + expect(screen.getByRole('button', { name: /Show more/ })).toBeInTheDocument(); + }); + + it('treats an empty child list as no children so the text fallback still renders', () => { + // `{items.map(…)}` over an + // empty list must not yield an empty capped box with `text` silently dropped. + render({[]}); + expect(screen.getByText('plain fallback')).toBeInTheDocument(); + expect(document.getElementById('c9')).toHaveClass('line-clamp-2'); + }); + + it('treats a list of non-rendering children as no children', () => { + // `items.map(i => i.show ? : null)` over an all-hidden list yields + // `[null]` — length 1, renders nothing. Counting array length would take the + // children path and drop `text` into an empty capped box. + render( + {[null, false, undefined, ' ']} + ); + expect(screen.getByText('plain fallback')).toBeInTheDocument(); + expect(document.getElementById('c9b')).toHaveClass('line-clamp-2'); + }); + + it('takes the children path when a list renders even one child', () => { + forceOverflow(); + render( + {[null,

real

]}
+ ); + expect(screen.getByText('real')).toBeInTheDocument(); + expect(screen.queryByText('plain fallback')).not.toBeInTheDocument(); + expect(document.getElementById('c9c')).not.toHaveClass('line-clamp-2'); + }); + + it('expands when focus lands inside the capped region', () => { + // The cap is a scroll container with no visible scrollbar. Tabbing to a link + // below it would scroll the preview to reveal the target with no way back, + // while `aria-expanded="false"` claimed content that is fully in the tab + // order was hidden. + forceOverflow(); + render( + + buried link + + ); + + expect(screen.getByRole('button')).toHaveAttribute('aria-expanded', 'false'); + fireEvent.focus(screen.getByRole('link', { name: 'buried link' })); + + expect(screen.getByRole('button', { name: /Show less/ })).toHaveAttribute('aria-expanded', 'true'); + expect(document.getElementById('c10')).not.toHaveClass('overflow-hidden'); + }); + + it('measures overflow live on focus rather than trusting the state flag', () => { + // The focus handler must not read `isOverflowing`, which a passive effect + // populates after commit. A descendant with `autoFocus` takes focus during + // the commit phase — before that effect has ever run — so the flag is still + // a stale `false` and the focused control is stranded inside the clipped, + // scrolled region. Simulated here by letting the element start out fitting + // and become overflowing with no re-measure in between. + const spy = forceOverflow(); + spy.mockReturnValue(0); + render( + + buried link + + ); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + + spy.mockReturnValue(500); + fireEvent.focus(screen.getByRole('link', { name: 'buried link' })); + + expect(screen.getByRole('button', { name: /Show less/ })).toHaveAttribute('aria-expanded', 'true'); + expect(document.getElementById('c12')).not.toHaveClass('overflow-hidden'); + }); + + it('leaves fitting children alone when focus lands inside them', () => { + // Nothing is clipped, so there is nothing to reveal — expanding here would + // only mint a no-op "Show less" button into the tab order. + render( + + visible link + + ); + + fireEvent.focus(screen.getByRole('link', { name: 'visible link' })); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + expect(document.getElementById('c11')).toHaveClass('overflow-hidden'); + }); + it('keeps the toggle when a resize fires against the uncapped container', () => { // Same in-flight-callback race as the line-clamp path: expanding removes the // cap, and a resize notification already in flight would otherwise measure