debabsah commented on PR #41329: URL: https://github.com/apache/superset/pull/41329#issuecomment-4792940183
Thanks for the thorough review, and for pushing me to handle the deferred focus interaction here. I have addressed every item plus that bug, which turned out to be larger than I first described. - H1 (autofocus). Done, with one note for transparency. I first added the autoFocus prop exactly as you suggested, but antd's Modal moves focus to the dialog container on open, so the input never kept it (confirmed with a test that showed focus on the modal container rather than the field). I used the ref + select() pattern you mentioned instead, which reliably focuses the input and selects the prefilled name so it can be overtyped, matching the old prompt(). - M1 (Enter on empty input). Fixed using your suggestion: onPressEnter no-ops when the trimmed name is empty, so the modal stays open and mirrors the disabled Save button rather than dismissing like Escape. - M2 (test coverage). Added tests for prefill with the current tab name, focus on open, Save disabled on empty or whitespace input, Enter on empty not dispatching or dismissing, and dismissal via both Cancel and the close (X) button. The dismiss tests assert store.getActions() stays empty, as you asked. 16 tests total, all green; prettier and oxlint clean. - M3 (the deferred focus interaction). Fixed here, and it was a real bug: while typing in the rename field, Delete threw focus out to the tab header and Space would not type. Root cause: the modal input renders in a portal, but React bubbles its key events through the component tree to the SQL Lab tab, which is an antd editable-card Tabs node. rc-tabs' keyboard handler treats Delete/Backspace as remove-tab, arrows/Home/End as navigate, and Enter/Space as activate, calling preventDefault on several of them (which is why Space was swallowed). The fix stops key events from leaking out of the input, leaving Escape and Tab to bubble so the modal can still close and trap focus. A unit test asserts the leak is closed. - N1 (double trim). Extracted the trimmed title once and reused it for the Save disabled state and the handler. - N2 (focus return on close). Done via the openerRef prop you pointed to. One note: the MenuDotsDropdown trigger is a non-focusable div and the menu item unmounts with the menu, so the focus-restore target is the tab header that contains the trigger (made focusable with tabIndex -1) rather than the trigger itself. openerRef covers the Cancel/X/Escape paths; the Save path closes through the show prop rather than onHide, so I return focus there explicitly too. If you would rather focus land on the trigger itself, that needs the shared MenuDotsDropdown made focusable, which I am glad to do as a separate PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
