[core] refactor(Popover): improve popover captureDismiss mechanism #4328
Conversation
ejose19
changed the title
|
@adidahiya Thanks for the review, I run tests locally and completely forgot about react 15. Is there an script for running tests with that configuration? |
| // an OVERRIDE inside a DISMISS does not dismiss, and a DISMISS inside an OVERRIDE will dismiss. | ||
| const dismissElement = eventTarget.closest(`.${Classes.POPOVER_DISMISS}, .${Classes.POPOVER_DISMISS_OVERRIDE}`); | ||
| const shouldDismiss = dismissElement != null && dismissElement.classList.contains(Classes.POPOVER_DISMISS); | ||
| const isDisabled = eventTarget.closest(`:disabled, .${Classes.DISABLED}`) != null; | ||
| if (shouldDismiss && !isDisabled && !e.isDefaultPrevented()) { | ||
| if (shouldDismiss && !isDisabled && (!isEventPopoverCapturing || isEventFromSelf)) { |
There was a problem hiding this comment.
is isDisabled still relevant here? Not sure if the check was to avoid preventDefault on an event that should not have been touched, or if we genuinely didn't want to close the popover if its target is disabled
There was a problem hiding this comment.
I'm pretty sure it's redundant, since a disabled element can't emit a click event. However I don't know if any blueprint component only uses disabled class but doesn't respect that rule (I checked anchor button and they do respect not triggering onClick) if there's no element which applies disabled class and allow clicking I can remove that too.
|
@ejose19 yes, it is possible to run those tests by setting an environment variable in your shell: |
Fixes #2820
Checklist
Changes proposed in this pull request:
captureDismisspreventDefault/defaultPreventedand instead use a more self contained approach to determine whether the popover should close in the presence ofcaptureDismiss