mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(ui): don't start playback when closing the disc cover lightbox (#5901)
The Lightbox renders through a React portal (react-modal), and React propagates synthetic events up the React tree rather than the DOM tree, so clicking its close button or backdrop bubbled into the ancestor TableRow and triggered playSubset. The existing timestamp guard could not catch this: react-image-lightbox defers onCloseRequest by animationDuration via setTimeout, so lightboxClosedAt was only stamped 200ms after the click had already propagated. The guard is kept because it still covers the separate mobile ghost-click path, where the synthesized click lands on the row after the overlay unmounts. Stopping propagation in onCloseRequest — the approach used by ContextMenus, whose MUI onClose fires synchronously — does not work for the same reason. Instead, wrap the Lightbox in an element that stops propagation at the React tree boundary the portal bubbles through, which also covers the lightbox controls we don't own (zoom buttons, caption, image drag).
This commit is contained in:
parent
dce48ff650
commit
0f4c9b8212
@ -78,7 +78,7 @@ const useStyles = makeStyles({
|
||||
},
|
||||
})
|
||||
|
||||
const DiscSubtitleRow = forwardRef(
|
||||
export const DiscSubtitleRow = forwardRef(
|
||||
({ record, onClick, colSpan, contextAlwaysVisible }, ref) => {
|
||||
const translate = useTranslate()
|
||||
const isDesktop = useMediaQuery((theme) => theme.breakpoints.up('md'))
|
||||
@ -153,13 +153,17 @@ const DiscSubtitleRow = forwardRef(
|
||||
{subtitle}
|
||||
</Typography>
|
||||
{isLightboxOpen && !imageError && (
|
||||
<Lightbox
|
||||
imagePadding={50}
|
||||
animationDuration={200}
|
||||
imageTitle={record.album + ' - ' + subtitle}
|
||||
mainSrc={fullImageUrl}
|
||||
onCloseRequest={handleCloseLightbox}
|
||||
/>
|
||||
// Lightbox portals out of the row, but React still bubbles its
|
||||
// events up this tree, where the row's onClick would play the disc.
|
||||
<span onClick={(e) => e.stopPropagation()}>
|
||||
<Lightbox
|
||||
imagePadding={50}
|
||||
animationDuration={200}
|
||||
imageTitle={record.album + ' - ' + subtitle}
|
||||
mainSrc={fullImageUrl}
|
||||
onCloseRequest={handleCloseLightbox}
|
||||
/>
|
||||
</span>
|
||||
)}
|
||||
</TableCell>
|
||||
<TableCell>
|
||||
|
||||
74
ui/src/common/SongDatagrid.test.jsx
Normal file
74
ui/src/common/SongDatagrid.test.jsx
Normal file
@ -0,0 +1,74 @@
|
||||
import React from 'react'
|
||||
import { render, fireEvent, screen } from '@testing-library/react'
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest'
|
||||
import { createTheme, ThemeProvider } from '@material-ui/core/styles'
|
||||
import { DiscSubtitleRow } from './SongDatagrid'
|
||||
|
||||
vi.mock('../subsonic', () => ({
|
||||
default: { getDiscCoverArtUrl: () => 'http://localhost/cover.jpg' },
|
||||
}))
|
||||
|
||||
vi.mock('react-redux', () => ({ useDispatch: () => vi.fn() }))
|
||||
|
||||
vi.mock('../common', () => ({ AlbumContextMenu: () => null }))
|
||||
|
||||
vi.mock('react-dnd', () => ({ useDrag: () => [{}, vi.fn()] }))
|
||||
|
||||
const record = {
|
||||
id: 'song-1',
|
||||
albumId: 'album-1',
|
||||
album: 'The Album',
|
||||
discNumber: 2,
|
||||
discSubtitle: 'Bonus Disc',
|
||||
updatedAt: '2024-01-01',
|
||||
}
|
||||
|
||||
const renderRow = (onClick) =>
|
||||
render(
|
||||
<ThemeProvider theme={createTheme()}>
|
||||
<table>
|
||||
<tbody>
|
||||
<DiscSubtitleRow record={record} onClick={onClick} colSpan={3} />
|
||||
</tbody>
|
||||
</table>
|
||||
</ThemeProvider>,
|
||||
)
|
||||
|
||||
const openLightbox = () => {
|
||||
fireEvent.click(document.querySelector('img'))
|
||||
expect(document.querySelector('.ril__closeButton')).toBeTruthy()
|
||||
}
|
||||
|
||||
describe('DiscSubtitleRow', () => {
|
||||
beforeEach(() => vi.clearAllMocks())
|
||||
|
||||
it('plays the disc when the row is clicked', () => {
|
||||
const onClick = vi.fn()
|
||||
renderRow(onClick)
|
||||
fireEvent.click(screen.getByText('Bonus Disc'))
|
||||
expect(onClick).toHaveBeenCalledWith(2)
|
||||
})
|
||||
|
||||
it('does not play the disc when opening the lightbox', () => {
|
||||
const onClick = vi.fn()
|
||||
renderRow(onClick)
|
||||
openLightbox()
|
||||
expect(onClick).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not play the disc when closing the lightbox', () => {
|
||||
const onClick = vi.fn()
|
||||
renderRow(onClick)
|
||||
openLightbox()
|
||||
fireEvent.click(document.querySelector('.ril__closeButton'))
|
||||
expect(onClick).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not play the disc when clicking the lightbox backdrop', () => {
|
||||
const onClick = vi.fn()
|
||||
renderRow(onClick)
|
||||
openLightbox()
|
||||
fireEvent.click(document.querySelector('.ril__inner'))
|
||||
expect(onClick).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
Loading…
x
Reference in New Issue
Block a user