Skip to content

Commit 7fad8b7

Browse files
authored
fix(dotcom): dismiss sidebar menus from outside clicks (tldraw#9330)
In order to keep sidebar menus from being stranded when dismissed from outside the sidebar, this PR clears open menu state when the mobile sidebar overlay closes and lets desktop canvas clicks dismiss the workspace switcher. The switcher dismissal overlay covers the viewport, so that dismissing click is swallowed before it reaches sidebar items or the canvas, while focus changes from workspace-switch canvas remounts are still ignored. ### Change type - [x] `bugfix` ### Test plan 1. On desktop, open the workspace switcher, click the canvas, and verify the switcher closes without the click deselecting the current shape. 2. In a mobile viewport, open the sidebar, open the workspace switcher, then click the sidebar overlay and verify the switcher closes. 3. Reopen the mobile sidebar, open a sidebar file menu, then click the sidebar overlay and verify the file menu closes. 4. `yarn lint-current` 5. `yarn workspace dotcom lint` 6. Attempted `NODE_OPTIONS='--no-strip-types' yarn workspace dotcom playwright test e2e/tests/smoke/workspaces.spec.ts --project=chromium -g "closing the mobile sidebar closes open sidebar menus"`, but the shared `workerStorageState` auth fixture timed out waiting for the login redirect before the test body ran. - [x] End to end tests ### Release notes - Fix sidebar menus staying open after closing the mobile sidebar or clicking the canvas. ### Code changes | Section | LOC change | | ------- | ---------- | | Apps | +8 / -7 | | Tests | +68 / -0 |
1 parent cfef7b1 commit 7fad8b7

4 files changed

Lines changed: 76 additions & 7 deletions

File tree

apps/dotcom/client/e2e/tests/smoke/workspaces.spec.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,74 @@ test.describe('workspaces', () => {
163163
await expect(page.getByTestId('tla-workspace-switcher-home')).toBeHidden()
164164
})
165165

166+
test('clicking the canvas closes the workspace switcher without reaching the canvas', async ({
167+
page,
168+
sidebar,
169+
}) => {
170+
const shapeId = await page.evaluate(() => {
171+
const editor = (window as any).editor
172+
editor.createShapes([
173+
{
174+
type: 'geo',
175+
x: 100,
176+
y: 100,
177+
props: { geo: 'rectangle', w: 100, h: 100 },
178+
},
179+
])
180+
const shape = editor.getCurrentPageShapes()[0]
181+
editor.select(shape.id)
182+
return shape.id
183+
})
184+
185+
await sidebar.openWorkspaceSwitcher()
186+
const homeItem = page.getByTestId('tla-workspace-switcher-home')
187+
await expect(homeItem).toBeVisible()
188+
189+
await page.mouse.click(600, 300)
190+
await expect(homeItem).toBeHidden()
191+
expect(
192+
await page.evaluate(() => {
193+
const editor = (window as any).editor
194+
return editor.getSelectedShapeIds()
195+
})
196+
).toEqual([shapeId])
197+
})
198+
199+
test('closing the mobile sidebar closes open sidebar menus', async ({ page, sidebar }) => {
200+
const fileName = getRandomName()
201+
await sidebar.createNewDocument(fileName)
202+
203+
await page.setViewportSize({ width: 390, height: 844 })
204+
205+
const mobileToggle = page.getByTestId('tla-sidebar-toggle-mobile')
206+
const mobileOverlay = page.getByTestId('tla-sidebar-overlay-mobile')
207+
const closeMobileSidebar = async () => {
208+
await mobileOverlay.click({ position: { x: 380, y: 422 } })
209+
await expect(mobileOverlay).toBeHidden()
210+
}
211+
212+
await expect(mobileToggle).toBeVisible()
213+
await mobileToggle.click()
214+
await expect(mobileOverlay).toBeVisible()
215+
216+
await sidebar.openWorkspaceSwitcher()
217+
await expect(page.getByTestId('tla-workspace-switcher-home')).toBeVisible()
218+
219+
await closeMobileSidebar()
220+
await expect(page.getByTestId('tla-workspace-switcher-home')).toBeHidden()
221+
222+
await mobileToggle.click()
223+
await expect(mobileOverlay).toBeVisible()
224+
225+
const fileLink = sidebar.getFileByName(fileName)
226+
await fileLink.hover()
227+
await fileLink.getByRole('button').click()
228+
await expect(page.getByRole('menuitem', { name: 'Rename' })).toBeVisible()
229+
230+
await closeMobileSidebar()
231+
await expect(page.getByRole('menuitem', { name: 'Rename' })).toBeHidden()
232+
})
233+
166234
test('create file button creates in the active workspace', async ({ sidebar }) => {
167235
const workspaceName = getRandomName()
168236
const file1 = getRandomName()

apps/dotcom/client/src/tla/components/TlaSidebar/TlaSidebar.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { memo, useCallback, useEffect } from 'react'
2+
import { tlmenus } from 'tldraw'
23
import { useActiveWorkspaceId } from '../../hooks/useActiveWorkspaceId'
34
import { useHasFlag } from '../../hooks/useHasFlag'
45
import { useTldrFileDrop } from '../../hooks/useTldrFileDrop'
@@ -43,6 +44,7 @@ export const TlaSidebar = memo(function TlaSidebar() {
4344
}, [trackEvent])
4445

4546
const handleOverlayClick = useCallback(() => {
47+
tlmenus.clearOpenMenus()
4648
updateLocalSessionState(() => ({ isSidebarOpenMobile: false }))
4749
}, [])
4850

@@ -56,6 +58,7 @@ export const TlaSidebar = memo(function TlaSidebar() {
5658
<button
5759
className={styles.sidebarOverlayMobile}
5860
data-visiblemobile={isSidebarOpenMobile}
61+
data-testid="tla-sidebar-overlay-mobile"
5962
onClick={handleOverlayClick}
6063
/>
6164
<div

apps/dotcom/client/src/tla/components/TlaSidebar/components/TlaSidebarWorkspaceSwitcher.tsx

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ export function TlaSidebarWorkspaceSwitcher() {
9090
/>
9191
)}
9292
<div className={styles.sidebarWorkspaceSwitcherRoot}>
93+
{/* Modal doesn't really work for us here because other elements have explicit pointer-events on. Thus the switcher overlay.*/}
9394
<_DropdownMenu.Root open={isOpen} onOpenChange={onOpenChange} modal>
9495
<_DropdownMenu.Trigger asChild>
9596
<button
@@ -116,12 +117,9 @@ export function TlaSidebarWorkspaceSwitcher() {
116117
sideOffset={4}
117118
alignOffset={-4}
118119
collisionPadding={8}
119-
// Switching workspaces mounts a new canvas that steals focus as it
120-
// loads. Without this the focus shift would dismiss the switcher mid-
121-
// switch. The open state is driven externally (useGlobalMenuIsOpen), so
122-
// the menu still closes via the trigger, the overlay, or Escape.
123-
onPointerDownOutside={(e) => e.preventDefault()}
124-
onInteractOutside={(e) => e.preventDefault()}
120+
// Switching workspaces mounts a new canvas that steals focus as it loads.
121+
// Ignore that focus shift while still allowing real outside pointer clicks
122+
// (for example, clicking the canvas) to dismiss the switcher.
125123
onFocusOutside={(e) => e.preventDefault()}
126124
>
127125
<WorkspaceSwitcherItem

apps/dotcom/client/src/tla/components/TlaSidebar/sidebar.module.css

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,7 @@
548548
}
549549

550550
.sidebarWorkspaceSwitcherOverlay {
551-
position: absolute;
551+
position: fixed;
552552
inset: 0;
553553
z-index: calc(var(--tl-layer-menus) - 1);
554554
pointer-events: all;

0 commit comments

Comments
 (0)