prevent event from being used to enter a newline into editor #7493

zaggino · 2014-04-11T22:36:48Z

this fixes brackets-userland/brackets-git#385
cc @TomMalbran @peterflynn

peterflynn · 2014-04-14T17:50:05Z

@njx Per earlier conversation, I think the reason this doesn't crop up in our dialogs much is that most of our dialogs fail to re-focus the editor when they're being closed -- so the subsequent keyup doesn't go the editor, it just goes off into space. Whenever we fix our dialogs to behave better I think we'd need this fix for core too :-)

njx · 2014-04-14T23:45:36Z

src/widgets/Dialogs.js

@@ -153,6 +153,8 @@ define(function (require, exports, module) {
            // Enter key in single-line text input always dismisses; in text area, only Ctrl+Enter dismisses
            // Click primary
            $primaryBtn.click();
+            // prevent event from being used to enter a newline into editor
+            e.preventDefault();


This makes sense, but it seems like we should do this in all the other cases where we handle the keypress as well. For example, if you do Quick Edit on a tag, then open your dialog and hit Esc, I bet it falls through to the editor too.

I also wonder if we should do stopPropagation() in these cases.

stopPropagation() didn't help in my case (it was my first guess) but preventDefault() did ... I didn't find any weird stuff going on in my code on first look.

Yeah, I don't think it's required for this case, but it probably makes sense in general since someone else might attach an event handler and we should act here like we've handled it. (I guess there's a question of why we don't just do that in any case in the global keydown hook stuff when a hook returns true.)

What do you think about adding it to the other cases in this method (Esc, etc.)?

(To be clear, I mean - what do you think about preventing default in the other cases? Does the Esc case I mentioned above occur with your extension?)

Well, in my opinion we should preventDefault and stopPropagation always when we use the event to trigger another event with methods such as .click

I think it makes sense to do it in basically all the cases where handling the event causes the dialog to dismiss (which I think is pretty much all the arms of the if statement, except maybe for the TAB case) - it's not so much about triggering another event, but the fact that the keyup goes to the main editor after the dialog is dismissed.

I guess I'm suggesting that you go ahead and fix those other cases as part of this pull request, instead of just fixing the Enter case (since I think other cases like Space and Esc could affect you as well anyway). If you don't have time, let me know and I'll take a stab at it.

I'm quite busy this week before vacation and you probably know more about this than I, so feel free.

njx · 2014-05-14T18:57:14Z

Sorry this missed release 39. I'm tagging it for release 40 so I don't forget about it.

…o handle the keyup

njx · 2014-05-21T23:19:44Z

@zaggino Just pushed an update to this that does the stop/preventDefault in more cases. Would you like to review it? I did some testing with various dialogs in brackets-git as well as the file filters dialog and everything seems to be working okay, but it could probably use more testing.

@peterflynn FYI - do you think this is reasonably safe?

marcelgerber · 2014-05-23T18:56:28Z

@njx Writing spaces into the Extension Manager search box is very laggy with this PR.

njx · 2014-05-23T19:41:12Z

@SAplayer Are you sure that's specific to this branch? Any lag there is probably from the actual search through the large number of extensions taking time, not from the event handling. I don't notice any difference between master and this branch in that regard.

marcelgerber · 2014-05-23T19:57:29Z

Oh, you're right. I'm sorry.

redmunds · 2014-06-03T22:58:31Z

Change Milestone to Release 0.41

njx · 2014-06-03T23:27:03Z

Assigning to @zaggino to review my changes to the original PR.

…an enter key.

zaggino · 2014-06-04T00:27:41Z

@njx I had a problem when another button than primary was focused (Close button) but pressing enter key would still launch click handler for the primary button. See 046aa94

zaggino · 2014-06-04T00:32:58Z

And I'm not 100% sure whether to use $focusedElement or this.find(e.target) there because $focusedElement seems to consider only elements with class dialog-button ...

njx · 2014-06-06T23:49:50Z

@zaggino Interesting - this looks like a bit of a difference between Mac and Windows. In native Mac apps (if "Full Keyboard Access" is on, so you can tab to buttons), Enter always activates the default button even if a different button has focus - you have to hit Space to activate the focused button. On Windows, when a button is focused, both Enter and Space activate it.

(The Windows behavior actually has a slight added nuance. If a non-button control in the body of the dialog is focused, then the original primary button has the "primary button" highlight, and hitting Enter will activate it. But if a button control anywhere in the dialog is focused - regardless of whether it's a dismissal button - then that button control gets both the focus look and the "primary button" look, making it clearer that both Enter and Space will activate it. So the "primary button" highlight actually jumps around as you tab through buttons in dialogs, then returns to the original primary button whenever a non-button is focused.)

I could imagine keeping the original behavior on Mac and changing it for Windows only - though I don't really know how many people are familiar with this nuance on the Mac (perhaps some people might turn it on for accessibility reasons). Or we could just decide not to deal with all these nuances and just keep what you have on both platforms (i.e., if the focused element is a button, Enter always activates it, otherwise activates the original primary button, and we don't worry about moving the "primary button" highlight around).

@larz0 do you have an opinion?

larz0 · 2014-06-07T00:36:56Z

@njx I think we should just keep what @zaggino has on both platforms and see how that goes.

zaggino · 2014-06-11T23:06:33Z

@njx
Didn't find any more issues with this, I think it's good to merge if no-one objects.

njx · 2014-06-12T02:03:02Z

Sounds good - I will try to look at your most recent change tomorrow and get it merged.

njx · 2014-06-12T21:18:06Z

Looks good, merging. Sorry this took so long.

prevent event from being used to enter a newline into editor

prevent event from being used to enter a newline into editor

f4841d6

njx self-assigned this Apr 14, 2014

njx reviewed Apr 14, 2014
View reviewed changes

njx mentioned this pull request Apr 14, 2014

Dialogs don't return focus to editor #7511

Open

njx added the medium priority label Apr 24, 2014

njx added this to the Release #40 milestone May 14, 2014

Stop keydown events that we handle in dialogs, so no one else tries t…

d0f185b

…o handle the keyup

redmunds modified the milestones: Release #41, Release #40 Jun 3, 2014

njx assigned zaggino and unassigned njx Jun 3, 2014

Fix a case when another button than primary is focused and user hits …

046aa94

…an enter key.

njx added a commit that referenced this pull request Jun 12, 2014

Merge pull request #7493 from adobe/zaggino/fix-enter-dialogs

7b12539

prevent event from being used to enter a newline into editor

njx merged commit 7b12539 into master Jun 12, 2014

njx deleted the zaggino/fix-enter-dialogs branch June 12, 2014 21:18

marcelgerber mentioned this pull request Jun 21, 2014

[Dialogs] No arrows usable to select dialog option #5548

Closed

This was referenced Aug 30, 2021

[CLOSED] [Dialogs] No arrows usable to select dialog option brackets-archive/bracketsIssues#5087

Open

[CLOSED] prevent event from being used to enter a newline into editor brackets-archive/bracketsIssues#6749

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent event from being used to enter a newline into editor #7493

prevent event from being used to enter a newline into editor #7493

zaggino commented Apr 11, 2014

peterflynn commented Apr 14, 2014

njx Apr 14, 2014

njx Apr 14, 2014

zaggino Apr 15, 2014

njx Apr 15, 2014

njx Apr 15, 2014

zaggino Apr 16, 2014

njx Apr 24, 2014

zaggino Apr 25, 2014

njx commented May 14, 2014

njx commented May 21, 2014

marcelgerber commented May 23, 2014

njx commented May 23, 2014

marcelgerber commented May 23, 2014

redmunds commented Jun 3, 2014

njx commented Jun 3, 2014

zaggino commented Jun 4, 2014

zaggino commented Jun 4, 2014

njx commented Jun 6, 2014

larz0 commented Jun 7, 2014

zaggino commented Jun 11, 2014

njx commented Jun 12, 2014

njx commented Jun 12, 2014

prevent event from being used to enter a newline into editor #7493

prevent event from being used to enter a newline into editor #7493

Conversation

zaggino commented Apr 11, 2014

peterflynn commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njx commented May 14, 2014

njx commented May 21, 2014

marcelgerber commented May 23, 2014

njx commented May 23, 2014

marcelgerber commented May 23, 2014

redmunds commented Jun 3, 2014

njx commented Jun 3, 2014

zaggino commented Jun 4, 2014

zaggino commented Jun 4, 2014

njx commented Jun 6, 2014

larz0 commented Jun 7, 2014

zaggino commented Jun 11, 2014

njx commented Jun 12, 2014

njx commented Jun 12, 2014