WebUI: Allow closing modals with Escape #22920
Conversation
|
I think i've tested every dialog and the only one in which it doesn't apply is Pressing escape when 2+ dialogs are active closes everything. |
Latest commit should take care of this. A bunch of modals are iframed it seems and do not inherit the Same approach was done before for (You can also see a bunch of other deeply hidden modals that would not close, changed them too)
Is this an issue ? I see that it happens, and currently it should depend on focus & modal type. If the focus is on the main window, it will close all modals. If the focus is on a modal in an iframe, it would close only this modal. Checking the code i see an easy fix for 2 modals, but when we have 2+ i doubt that we can guarantee the same order of closing. Just tell me if its a blocker, and if it is im gonna put some more time into it. |
As a user this would bother me. For example in settings > RSS > Edit auto downloading rules, after closing that dialog i'd expect to go back to settings and in general i'd expect only the focused dialog to close. |
| Object.values(MochaUI.Windows.instances).forEach((modal) => { | ||
| modal.close(); |
There was a problem hiding this comment.
Updated it with a check/sort based on the instance's timestamp so that we always close the latest one.
Tested with the described scenario from above and a few more, seems to work.
I also took a look at the PR you linked, and see that till now the idea was to add the event handler on each dialog's src/webui/www/private/views/*.html separately. Personally I dont think this is a good approach, but i can be persuaded into following it.
It would help me understand the code base better if someone has an idea how it was decided which dialog should load by xhr and which by iframe thats declared in src/webui/www/private/scripts/mocha-init.js with some modals calling new MochaUI.Window with loadMethod: "xhr", and some with loadMethod: "iframe",
I see that there are files from both load methods that declare their own event handler for escape, and I understand that for the iframe ones, but what is the reasoning for not using xhr only ?
Using xhr only, we could have a centralized event handler (in client.js as i attempt to do) without the need to re-declare the handler for each dialog.
There was a problem hiding this comment.
Updated it with a check/sort based on the instance's
timestampso that we always close the latest one. Tested with the described scenario from above and a few more, seems to work.
This is a clever solution, however the most recently opened window isn't necessarily the one on top. A user can click on a different window to bring it to the foreground.
It does seem that the focused window is assigned the class isFocused. Could you use that?
I also took a look at the PR you linked, and see that till now the idea was to add the event handler on each dialog's
src/webui/www/private/views/*.htmlseparately. Personally I dont think this is a good approach, but i can be persuaded into following it.
You're likely right that there's a better way. I followed this approach since it was what was already in the codebase (which of course isn't a great reason). I do like your approach of having a single handler for this (the code in client.js).
It would help me understand the code base better if someone has an idea how it was decided which dialog should load by
xhrand which byiframethats declared insrc/webui/www/private/scripts/mocha-init.jswith some modals callingnew MochaUI.WindowwithloadMethod: "xhr",and some withloadMethod: "iframe",I see that there are files from both load methods that declare their own event handler for escape, and I understand that for the
iframeones, but what is the reasoning for not usingxhronly ?Using
xhronly, we could have a centralized event handler (inclient.jsas i attempt to do) without the need to re-declare the handler for each dialog.
I can look into this further, but off the top of my head I can't remember an exact reason. @Chocobo1 might have more context.
There was a problem hiding this comment.
Didnt think that through it seems.
Latest commit uses the isFocused class as you suggested. Only thing that I notice is that if the user opens a bunch of modals and then (accidentally or deliberately) focuses the main window, escape would do nothing. But thats logical, because there is no focused modal if the user focuses the main window, so i take it thats intended behavior.
There was a problem hiding this comment.
- When closing "Enable automatic torrent management" with Esc, it also closes the last focused dialog.
- Closing any dialog with Esc while "Rename files..." is also open, will close that too.
|
@userwiths |
userwiths
force-pushed
the
fix/allow-closing-modals-with-escape
branch
from
5f527c2 to
8dd1662
Compare
|
Rebased and fixed the conflicts, should be fine now. |
userwiths
force-pushed
the
fix/allow-closing-modals-with-escape
branch
from
8dd1662 to
d09070d
Compare
Closes #13891
Tested on the following WebUI Modals:
Pressing Escape without an active modal/dialog should not do anything, in my tests it did not, but i might have missed some strange cases. If any such is reported, im gonna can care of it.