Skip to content

Commit 084a249

Browse files
RobinMalfaitRobinMalfait
authored
Fix incorrect nested Dialogs behaviour (#489)
* add tests to verify the nested Dialog behaviour * set mounted to true once rendered once * cache useWindowEvent listener We only care about the very last version of the listener function. This allows us to only change the event listener if the event name (string) and options (boolean | object) change. * add/delete messages when mounting/unmounting We don't require a dedicated hook anymore, so this is a bit of cleanup! * add comments to the FocusResult enum * splitup functionality and make it a bit more clear using feature flags * add getDialogOverlays helper * simplify the Portal component We don't need to add the current element to the Stack. We only want to take care of that in the Dialog component itself. * drop dom-containers Currently it is only used in a single spot, so I inlined it into that file. * simplify the FocusTrap component, use new API * improve Dialog component * update CHANGELOG
1 parent c13e6b7 commit 084a249

File tree

14 files changed

+602
-252
lines changed

14 files changed

+602
-252
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1515

1616
- Improve SSR for `Dialog` ([#477](https://github.com/tailwindlabs/headlessui/pull/477))
1717
- Delay focus trap initialization ([#477](https://github.com/tailwindlabs/headlessui/pull/477))
18+
- Improve incorrect behaviour for nesting `Dialog` components ([#560](https://github.com/tailwindlabs/headlessui/pull/560))
1819

1920
## [Unreleased - Vue]
2021

‎packages/@headlessui-react/src/components/dialog/dialog.test.tsx‎

Lines changed: 183 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
getByText,
1515
assertActiveElement,
1616
getDialogs,
17+
getDialogOverlays,
1718
} from '../../test-utils/accessibility-assertions'
1819
import { click, press, Keys } from '../../test-utils/interactions'
1920
import { PropsOf } from '../../types'
@@ -637,79 +638,205 @@ describe('Mouse interactions', () => {
637638
})
638639

639640
describe('Nesting', () => {
640-
it('should be possible to open nested Dialog components and close them with `Escape`', async () => {
641-
function Nested({ onClose, level = 1 }: { onClose: (value: boolean) => void; level?: number }) {
642-
let [showChild, setShowChild] = useState(false)
641+
function Nested({ onClose, level = 1 }: { onClose: (value: boolean) => void; level?: number }) {
642+
let [showChild, setShowChild] = useState(false)
643643

644-
return (
645-
<>
646-
<Dialog open={true} onClose={onClose}>
647-
<div>
648-
<p>Level: {level}</p>
649-
<button onClick={() => setShowChild(true)}>Open {level + 1}</button>
650-
</div>
651-
{showChild && <Nested onClose={setShowChild} level={level + 1} />}
652-
</Dialog>
653-
</>
654-
)
655-
}
644+
return (
645+
<>
646+
<Dialog open={true} onClose={onClose}>
647+
<Dialog.Overlay />
656648

657-
function Example() {
658-
let [open, setOpen] = useState(false)
649+
<div>
650+
<p>Level: {level}</p>
651+
<button onClick={() => setShowChild(true)}>Open {level + 1} a</button>
652+
<button onClick={() => setShowChild(true)}>Open {level + 1} b</button>
653+
<button onClick={() => setShowChild(true)}>Open {level + 1} c</button>
654+
</div>
655+
{showChild && <Nested onClose={setShowChild} level={level + 1} />}
656+
</Dialog>
657+
</>
658+
)
659+
}
659660

660-
return (
661-
<>
662-
<button onClick={() => setOpen(true)}>Open 1</button>
663-
{open && <Nested onClose={setOpen} />}
664-
</>
665-
)
666-
}
661+
function Example() {
662+
let [open, setOpen] = useState(false)
667663

668-
render(<Example />)
664+
return (
665+
<>
666+
<button onClick={() => setOpen(true)}>Open 1</button>
667+
{open && <Nested onClose={setOpen} />}
668+
</>
669+
)
670+
}
671+
672+
it.each`
673+
strategy | action
674+
${'with `Escape`'} | ${() => press(Keys.Escape)}
675+
${'with `Outside Click`'} | ${() => click(document.body)}
676+
${'with `Click on Dialog.Overlay`'} | ${() => click(getDialogOverlays().pop()!)}
677+
`(
678+
'should be possible to open nested Dialog components and close them $strategy',
679+
async ({ action }) => {
680+
render(<Example />)
669681

670-
// Verify we have no open dialogs
671-
expect(getDialogs()).toHaveLength(0)
682+
// Verify we have no open dialogs
683+
expect(getDialogs()).toHaveLength(0)
672684

673-
// Open Dialog 1
674-
await click(getByText('Open 1'))
685+
// Open Dialog 1
686+
await click(getByText('Open 1'))
675687

676-
// Verify that we have 1 open dialog
677-
expect(getDialogs()).toHaveLength(1)
688+
// Verify that we have 1 open dialog
689+
expect(getDialogs()).toHaveLength(1)
678690

679-
// Open Dialog 2
680-
await click(getByText('Open 2'))
691+
// Verify that the `Open 2 a` has focus
692+
assertActiveElement(getByText('Open 2 a'))
681693

682-
// Verify that we have 2 open dialogs
683-
expect(getDialogs()).toHaveLength(2)
694+
// Verify that we can tab around
695+
await press(Keys.Tab)
696+
assertActiveElement(getByText('Open 2 b'))
684697

685-
// Press escape to close the top most Dialog
686-
await press(Keys.Escape)
698+
// Verify that we can tab around
699+
await press(Keys.Tab)
700+
assertActiveElement(getByText('Open 2 c'))
687701

688-
// Verify that we have 1 open dialog
689-
expect(getDialogs()).toHaveLength(1)
702+
// Verify that we can tab around
703+
await press(Keys.Tab)
704+
assertActiveElement(getByText('Open 2 a'))
690705

691-
// Open Dialog 2
692-
await click(getByText('Open 2'))
706+
// Open Dialog 2 via the second button
707+
await click(getByText('Open 2 b'))
693708

694-
// Verify that we have 2 open dialogs
695-
expect(getDialogs()).toHaveLength(2)
709+
// Verify that we have 2 open dialogs
710+
expect(getDialogs()).toHaveLength(2)
696711

697-
// Open Dialog 3
698-
await click(getByText('Open 3'))
712+
// Verify that the `Open 3 a` has focus
713+
assertActiveElement(getByText('Open 3 a'))
699714

700-
// Verify that we have 3 open dialogs
701-
expect(getDialogs()).toHaveLength(3)
715+
// Verify that we can tab around
716+
await press(Keys.Tab)
717+
assertActiveElement(getByText('Open 3 b'))
702718

703-
// Press escape to close the top most Dialog
704-
await press(Keys.Escape)
719+
// Verify that we can tab around
720+
await press(Keys.Tab)
721+
assertActiveElement(getByText('Open 3 c'))
705722

706-
// Verify that we have 2 open dialogs
707-
expect(getDialogs()).toHaveLength(2)
723+
// Verify that we can tab around
724+
await press(Keys.Tab)
725+
assertActiveElement(getByText('Open 3 a'))
708726

709-
// Press escape to close the top most Dialog
710-
await press(Keys.Escape)
727+
// Close the top most Dialog
728+
await action()
711729

712-
// Verify that we have 1 open dialog
713-
expect(getDialogs()).toHaveLength(1)
714-
})
730+
// Verify that we have 1 open dialog
731+
expect(getDialogs()).toHaveLength(1)
732+
733+
// Verify that the `Open 2 b` button got focused again
734+
assertActiveElement(getByText('Open 2 b'))
735+
736+
// Verify that we can tab around
737+
await press(Keys.Tab)
738+
assertActiveElement(getByText('Open 2 c'))
739+
740+
// Verify that we can tab around
741+
await press(Keys.Tab)
742+
assertActiveElement(getByText('Open 2 a'))
743+
744+
// Verify that we can tab around
745+
await press(Keys.Tab)
746+
assertActiveElement(getByText('Open 2 b'))
747+
748+
// Open Dialog 2 via button b
749+
await click(getByText('Open 2 b'))
750+
751+
// Verify that the `Open 3 a` has focus
752+
assertActiveElement(getByText('Open 3 a'))
753+
754+
// Verify that we can tab around
755+
await press(Keys.Tab)
756+
assertActiveElement(getByText('Open 3 b'))
757+
758+
// Verify that we can tab around
759+
await press(Keys.Tab)
760+
assertActiveElement(getByText('Open 3 c'))
761+
762+
// Verify that we can tab around
763+
await press(Keys.Tab)
764+
assertActiveElement(getByText('Open 3 a'))
765+
766+
// Verify that we have 2 open dialogs
767+
expect(getDialogs()).toHaveLength(2)
768+
769+
// Open Dialog 3 via button c
770+
await click(getByText('Open 3 c'))
771+
772+
// Verify that the `Open 4 a` has focus
773+
assertActiveElement(getByText('Open 4 a'))
774+
775+
// Verify that we can tab around
776+
await press(Keys.Tab)
777+
assertActiveElement(getByText('Open 4 b'))
778+
779+
// Verify that we can tab around
780+
await press(Keys.Tab)
781+
assertActiveElement(getByText('Open 4 c'))
782+
783+
// Verify that we can tab around
784+
await press(Keys.Tab)
785+
assertActiveElement(getByText('Open 4 a'))
786+
787+
// Verify that we have 3 open dialogs
788+
expect(getDialogs()).toHaveLength(3)
789+
790+
// Close the top most Dialog
791+
await action()
792+
793+
// Verify that the `Open 3 c` button got focused again
794+
assertActiveElement(getByText('Open 3 c'))
795+
796+
// Verify that we can tab around
797+
await press(Keys.Tab)
798+
assertActiveElement(getByText('Open 3 a'))
799+
800+
// Verify that we can tab around
801+
await press(Keys.Tab)
802+
assertActiveElement(getByText('Open 3 b'))
803+
804+
// Verify that we can tab around
805+
await press(Keys.Tab)
806+
assertActiveElement(getByText('Open 3 c'))
807+
808+
// Verify that we have 2 open dialogs
809+
expect(getDialogs()).toHaveLength(2)
810+
811+
// Close the top most Dialog
812+
await action()
813+
814+
// Verify that we have 1 open dialog
815+
expect(getDialogs()).toHaveLength(1)
816+
817+
// Verify that the `Open 2 b` button got focused again
818+
assertActiveElement(getByText('Open 2 b'))
819+
820+
// Verify that we can tab around
821+
await press(Keys.Tab)
822+
assertActiveElement(getByText('Open 2 c'))
823+
824+
// Verify that we can tab around
825+
await press(Keys.Tab)
826+
assertActiveElement(getByText('Open 2 a'))
827+
828+
// Verify that we can tab around
829+
await press(Keys.Tab)
830+
assertActiveElement(getByText('Open 2 b'))
831+
832+
// Close the top most Dialog
833+
await action()
834+
835+
// Verify that we have 0 open dialogs
836+
expect(getDialogs()).toHaveLength(0)
837+
838+
// Verify that the `Open 1` button got focused again
839+
assertActiveElement(getByText('Open 1'))
840+
}
841+
)
715842
})

0 commit comments

Comments
 (0)