Fix for #6904 - Pressing [Enter] in Extension Manager Search box closes Extension Manager

[ingorichter]

This will prevent that the Extension Manager Dialog will be closed when the Enter key is hit inside the search field.

[peterflynn]

Doesn't this cause the button to change appearance? This styling would stop getting applied: https://github.com/adobe/brackets/blob/master/src/styles/brackets_patterns_override.less#L1126

[ingorichter]

Yes, you are right. What is the reason of having this button anyway? ESC closed the dialog and I don't think it's too bad indicating that this button won't do anything as long as the input field has the focus. I think, it's even worse once you tab out of the input field and the button get's enabled again. What does this mean in that context? The visual clue would be, now you can close the dialog? But this was possible, before using the mouse. Perhaps we should remove the button completely?

[njx]

Hmmm, this is a bit of an odd case. Normally Enter is supposed to act as the primary button, but in this case we explicitly don't want it to. I think what @ingorichter is arguing makes sense, but I can't think of another app that has a similar behavior in a modal dialog.

/cc @larz0 any thoughts? I think it's probably fine the way Ingo has it.

[larz0]

I think Enter should only work on controls that are on focus. The Primary button should generally be on focus by default. For this case the enter shouldn't close the window because the search field is on focus.

[peterflynn]

On Windows at least, Enter clicks the primary button even if something else has focus. For a simple text-input dialog (think prompt() in JS, for example), this is actually desirable: you can type a value and then dismiss the dialog by pressing Enter without having to move your hand to the mouse. This is just an odd exception to that rule.

Btw, if we had more sane keyboard handling for dialogs (e.g. a global bubble handler) then we could easily fix this simply by calling stopPropagation() from the text field. It's only the weird way Dialog captures key events that makes this tricky to fix...

[ingorichter]

Okay, what is the conclusion? We want a different fix for this issue?

[TomMalbran]

I guess that the easiest solution would be to have a special class for the search inputs and then change https://github.com/adobe/brackets/blob/master/src/widgets/Dialogs.js#L152 so that it takes into account the search inputs.

[peterflynn]

@TomMalbran I like that idea! Nice and simple.

[TomMalbran]

@peterflynn What do you think about using an input type search and removing the browser's default styles?

[ingorichter]

I think it's weird to handle a very specific case in the general purpose dialog class. Do we have more search input fields where we want this behavior?

[TomMalbran]

Not yet, but we might need it later for the Preferences UI, and maybe for other similar UI we make or that extensions authors make that would use a search input. To me is not a very specific case, but a common one, and the dialog is already handling several cases.

[ingorichter]

I can add this to the Dialog class, but I'm not convinced that this is the right solution nor do these other special case handlings belong there. This might be the easiest way to fix it. I think this should somehow be configurable by the creator of the dialog. Otherwise somebody doesn’t like a specific behavior that we provide with the general purpose Dialog implementation.

[TomMalbran]

In that case we could use a more generic class with a name that would imply that enter won't be executed after being pressed.

Another possibility could be to pass a keydown handler function that is executed before _keydownHook and _keydownHook is only executed if the given handler returns a falsy result. This function could be a new parameter when creating the dialog, or since it will only used a few times, we can add a new method to the Dialog class that is returned when creating the dialogs, that will allow to add 1 or more keydown handlers.

[njx]

Unassigning myself. I haven't fully digested the comments above, but it looks like this PR is not ready to go as-is since there's some desire to follow an alternate solution. @ingorichter Do you want to consider closing this and coming up with a different fix along the lines that @TomMalbran is proposing? Or @TomMalbran, maybe you'd like to implement a new fix?

[busykai]

FWIW, we also have File -> Project Settings and View -> Themes. In the Project Settings case, closing on enter seems natural -- it only has one field. In case of Themes (which wasn't there at the time of the discussion above), it does not feel right to close the dialog when you focus on, for example, Font Size and hit enter. So it sounds like a special "marker" class which would make Dialog to ignore Enter would be a nice solution (if I understand it right, it is what @TomMalbran suggests). Extensions with complex configuration could benefit from it, e.g. Brackets git.

Another question is what's the right behavior should be: should it just stay on the input field and do nothing, should it switch the focus to the default button, should it act as tab? Should there be different classes for different behaviors?..

[rroshan1]

It would be good if we could reach some consensus on the fix for this issue. This issue is experienced by many and it has been open for quite long now.

[ficristo]

Superceded by #12839