[core] fix(Overlay): handle document click/focus in Shadow DOM #4218
Conversation
|
Thanks for your interest in palantir/blueprint, @lmk123! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
| @@ -425,7 +425,7 @@ export class Overlay extends AbstractPureComponent2<IOverlayProps, IOverlayState | |||
|
|
|||
| private handleDocumentClick = (e: MouseEvent) => { | |||
| const { canOutsideClickClose, isOpen, onClose } = this.props; | |||
| const eventTarget = e.target as HTMLElement; | |||
| const eventTarget = (e.composed ? e.composedPath()[0] : e.target) as HTMLElement; | |||
There was a problem hiding this comment.
is e.composedPath() always guaranteed to be non-empty? can you add some code comments to explain why we are adding this special logic here and on L446?
There was a problem hiding this comment.
is
e.composedPath()always guaranteed to be non-empty?
Yes. It returns an array of EventTarget objects representing the objects on which an event listener will be invoked (from MDN), the first item is always equals to the event.target, or if this event is triggered in an open mode Shadow DOM, the first item is the actually element that trigger the event. There are some examples in MDN.
can you add some code comments to explain why we are adding this special logic here and on L446?
Done. A more detailed explanation can be found in #4220
I posted an issue just now: #4220
When I tried to add unit tests yesterday, I encountered some problems when installing |
|
I ran into a problem when running |
|
Change looks good, can you try to add a unit test in a separate PR please? |
adidahiya
changed the title
I will do this in the next few days. |
Fixes #4220
Checklist
Changes proposed in this pull request:
I use blueprint in Shadow DOM, and the Overlay component is closed even if I click inside it. This PR fix this problem.
I searched the place where
event.targetwas used in the entire code base, it seems that only Overlay component usedevent.targetin the event registered in thedocument.Reviewers should focus on:
Screenshot