Skip to content

♿️(frontend) limit share modal opening announcement for screen readers #2452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ovgodd wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −2
Open

♿️(frontend) limit share modal opening announcement for screen readers #2452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ and this project adheres to
- ♿️(frontend) use heading element for pinned documents section title #2380
- ♿️(frontend) use anchor links for table of contents entries #2390
- ♿️(frontend) improve presenter mode screen reader and keyboard support #2383
♿️(frontend) link export modal name to its heading #2422
- ♿️(frontend) link export modal name to its heading #2422
- ♿️(frontend) limit share modal opening announcement for screen readers #2452

### Fixed

Expand Down
21 changes: 20 additions & 1 deletion src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import {
import { QuickSearchGroupMember } from './DocShareMember';
import { DocShareModalFooter } from './DocShareModalFooter';

const DEBOUNCE_MS = 300;

Comment on lines +43 to +44

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Decouple search debounce timing from accessibility timing.

DEBOUNCE_MS currently drives both query filtering (Line 125) and accessibility re-exposure delay (Line 176). This couples unrelated UX knobs and makes future tuning risky.

Suggested refactor 
-const DEBOUNCE_MS = 300;
+const SEARCH_DEBOUNCE_MS = 300;
+const A11Y_CONTENT_REVEAL_DELAY_MS = 300;
...
-  }, DEBOUNCE_MS);
+  }, SEARCH_DEBOUNCE_MS);
...
-    }, DEBOUNCE_MS);
+    }, A11Y_CONTENT_REVEAL_DELAY_MS);

Also applies to: 125-125, 176-176

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`
around lines 43 - 44, The constant DEBOUNCE_MS is being used for two unrelated
purposes: query filtering and accessibility re-exposure delay. Create two
separate constants instead - one specifically for search query debouncing and
another for the accessibility re-exposure delay timing. This decouples these
independent concerns so each can be tuned separately without affecting the
other. Find where DEBOUNCE_MS is used at the query filtering location (around
line 125) and the accessibility re-exposure location (around line 176), then
replace the appropriate usages with their respective new constants.

const ShareModalStyle = createGlobalStyle`
.--docs--doc-share-modal [cmdk-item] {
cursor: auto;
Expand Down Expand Up @@ -84,6 +86,7 @@ export const DocShareModal = ({ doc, onClose, isRootDoc = true }: Props) => {

const [listHeight, setListHeight] = useState<string>('400px');
const canShare = doc.abilities.accesses_manage && isRootDoc;
const [isContentAccessible, setIsContentAccessible] = useState(canShare);
const canViewAccesses = doc.abilities.accesses_view;
const showMemberSection = inputValue === '' && selectedUsers.length === 0;
const showFooter = selectedUsers.length === 0 && !inputValue;
Expand Down Expand Up @@ -119,7 +122,7 @@ export const DocShareModal = ({ doc, onClose, isRootDoc = true }: Props) => {

const onFilter = useDebouncedCallback((str: string) => {
setUserQuery(str);
}, 300);
}, DEBOUNCE_MS);

const onRemoveUser = (row: User) => {
setSelectedUsers((prevState) => {
Expand Down Expand Up @@ -161,6 +164,20 @@ export const DocShareModal = ({ doc, onClose, isRootDoc = true }: Props) => {
const showInheritedShareContent =
inheritedAccesses.length > 0 && showMemberSection && !isRootDoc;

// When the search input is hidden, keep the modal content out of the
// accessibility tree during the opening announcement, then restore it.
useEffect(() => {
if (canShare) {
return;
}

const id = window.setTimeout(() => {
setIsContentAccessible(true);
}, DEBOUNCE_MS);

return () => window.clearTimeout(id);
}, [canShare]);

Comment on lines +167 to +180

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Ensure isContentAccessible is reset on every canShare transition.

Line 169 only handles the canShare === false branch by scheduling true, but it never forces false when canShare flips from true to false after mount. In that transition, Line 227 can keep content exposed to assistive tech, so the announcement-limiting behavior is skipped.

Suggested fix 
   useEffect(() => {
-    if (canShare) {
-      return;
-    }
+    setIsContentAccessible(canShare);
+    if (canShare) return;
 
     const id = window.setTimeout(() => {
       setIsContentAccessible(true);
     }, DEBOUNCE_MS);
 
     return () => window.clearTimeout(id);
   }, [canShare]);

Also applies to: 227-227

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`
around lines 167 - 180, The useEffect hook in DocShareModal only handles the
case when `canShare` is false by scheduling `isContentAccessible` to true, but
it does not reset `isContentAccessible` to false when `canShare` transitions
from false to true. To fix this, add a branch in the useEffect that immediately
sets `isContentAccessible` to false when `canShare` becomes true, ensuring the
content is removed from the accessibility tree during the opening announcement
for every transition of the `canShare` state.

// Invalidate relevant queries to ensure fresh data on modal open
useEffect(() => {
[
Expand Down Expand Up @@ -197,6 +214,7 @@ export const DocShareModal = ({ doc, onClose, isRootDoc = true }: Props) => {
{t('Share the document')}
</Text>
<ButtonCloseModal
autoFocus={!canShare}
aria-label={t('Close the share modal')}
onClick={onClose}
/>
Expand All @@ -206,6 +224,7 @@ export const DocShareModal = ({ doc, onClose, isRootDoc = true }: Props) => {
>
<ShareModalStyle />
<Box
aria-hidden={isContentAccessible ? undefined : true}
$height="auto"
$maxHeight={canViewAccesses ? modalContentHeight : 'none'}
$overflow="hidden"
Expand Down
Loading