[Overlay] Enforce focus after backdrop mouse down #1330
Conversation
Enforce focus after backdrop mouse downPreview: documentation |
| safeInvoke(onClose, e); | ||
| } | ||
| if (enforceFocus) { | ||
| // make sure document.activeElement is updated before bringing the focus back |
There was a problem hiding this comment.
not sure I understand this comment -- what's the "before" part about? isn't it updating document.activeElement by bringing the focus back?
There was a problem hiding this comment.
Kind of the same problem than in Select and MultiSelect: https://github.com/palantir/blueprint/pull/1275/files#diff-61619f2144645e953195b1eadae9ceadR199
In both this case and the one from the URL above, document.activeElement isn't the expected value after clicking away. From what I understand, rAF allows to wait for the next frame at which point activeElement is the correct value.
There was a problem hiding this comment.
rAF is decidedly not "the next frame," but the last possible moment of the same frame: before the next repaint. it lets us execute some logic after DOM updates but before repainting (to avoid flicker).
| } | ||
| if (enforceFocus) { | ||
| // make sure document.activeElement is updated before bringing the focus back | ||
| requestAnimationFrame(() => this.bringFocusInsideOverlay()); |
There was a problem hiding this comment.
why do we need requestAnimationFrame?
There was a problem hiding this comment.
There was a problem hiding this comment.
that link demonstrates setTimeout, which potentially makes more sense here (as noted in the other comment thread, rAF is not the next frame).
There was a problem hiding this comment.
We're already using rAF everywhere else we need the updated document.activeElement after triggering an event. Happy to change across the board to maintain consistency, I'll let you and @giladgray decide which is best and I'll open a separate PR if necessary
There was a problem hiding this comment.
we've enjoyed success using rAF for focus things, more than with setTimeout because it executes later in the same frame. i support this.
| if (canOutsideClickClose) { | ||
| safeInvoke(onClose, e); | ||
| } | ||
| if (enforceFocus) { |
There was a problem hiding this comment.
would be super great to add a unit test for this case. we have others for enforceFocus, should be a matter of copy-and-edit.
There was a problem hiding this comment.
Failing so far. What I have:
it("returns focus to overlay after clicking the backdrop if enforceFocus=true", () => {
wrapper = mount(
<Overlay enforceFocus={true} canOutsideClickClose={false} inline={true} isOpen={true}>
{createOverlayContents()}
</Overlay>,
);
wrapper.find(BACKDROP_SELECTOR).simulate("mousedown");
assert.equal(document.querySelector(BACKDROP_SELECTOR), document.activeElement);
});I can't figure out what the last line should be. As of now, it gives me this:
expected null to equal <body class="">
which I don't understand because the BACKDROP_SELECTOR element exists, also why is document.activeElement equals to body after mousing down on the backdrop?
Add test for enforceFocusPreview: documentation | table |
|
Made that test work 👍 Ready for review again. |
Fixes #1324
Checklist