Opened 12 years ago

Closed 12 years ago

#8985 closed Bug (fixed)

IE - Enter key not being properly processed by dialogs

Reported by: Lynne Kues Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.6.4
Component: UI : Dialogs Version: 3.0
Keywords: IE9 IBM Cc: monahant@…

Description

Enter key is not being consumed by dialog even though the dialog has focus. Instead the keystroke is propagating up and is being processed by our application while the dialog is opened. See attachment for an explanation. config.dialog_startupFocusTab = true;

I looked at dialog.js to see if I could figure out what was going on. If I modify focusKeydownHandler to consume the enter key in all instances, things work as expected.

Attachments (6)

table.png (104.3 KB) - added by Lynne Kues 12 years ago.
replacebycode.html (3.7 KB) - added by Jakub Ś 12 years ago.
8985.patch (3.9 KB) - added by Garry Yao 12 years ago.
8985_2.patch (6.5 KB) - added by Frederico Caldeira Knabben 12 years ago.
8985_3.patch (6.9 KB) - added by Frederico Caldeira Knabben 12 years ago.
8985_4.patch (6.9 KB) - added by Frederico Caldeira Knabben 12 years ago.

Download all attachments as: .zip

Change History (26)

Changed 12 years ago by Lynne Kues

Attachment: table.png added

comment:1 Changed 12 years ago by Jakub Ś

Status: newpending

I have just checked this issue in latest CKEditor 3.6.3.

First enter puts focus in Rows field but second inserts the table. There is no saving or other undesired action. Were you able to reproduce this problem in plain CKEditor or only inside your application?

comment:2 Changed 12 years ago by Teresa Monahan

Cc: monahant@… added

comment:3 Changed 12 years ago by Lynne Kues

I can only reproduce this inside our application and I do think it is related to running CKE inside dojo and/or our framework. If the CKE dialog has focus, why is the Enter key event being propagated, which it is and which is then being picked up by our application. As I indicated, if I change focusKeyDownHandler to consume the event (i.e., evt.stop(); evt.data.preventDefault();), our application doesn't process it, but that's not a fix and it's unclear as to why the event doesn't trigger OK like it does in FF. Any suggestions on how to further debug this? I am at a bit of a loss and unclear as to why our application would effect this dialog. We are not extending the table dialog.

Last edited 12 years ago by Lynne Kues (previous) (diff)

comment:4 Changed 12 years ago by Frederico Caldeira Knabben

Do you think you're able to produce a reduced test case for this issue, so we can confirm the problem at our side? I think this is the only way to be able to give you some more details about it. Thanks!

comment:5 Changed 12 years ago by Lynne Kues

The issue is that on IE, the first button it finds is being set as the default button. You'll notice in the screenshot that the Save button has a dark outline around it. Then when the table dialog is opened and the Enter key pressed, because the event is being propagated, the Save button is being invoked.

Changed 12 years ago by Jakub Ś

Attachment: replacebycode.html added

comment:6 Changed 12 years ago by Jakub Ś

Status: pendingconfirmed
Version: 3.4.23.0

OK, I was able to reproduce this problem in IEs from CKEditor 3.0.

It seems that config.dialog_startupFocusTab = true; is not required and all this TC needs is submit button above CKEditor.

To reproduce:

  1. Put attached file in _samples folder and load it in a browser
  2. Open table dialog and press Enter once or twice - focus will go to submit button

@lynne_kues you've said that you have managed to overcome this problem. Could you perhaps provide a patch write where (path and line to file) and what have you changed.

Last edited 12 years ago by Jakub Ś (previous) (diff)

comment:7 Changed 12 years ago by Lynne Kues

In 3.6.2 code base, dialog/plugin.js, method focusKeyDownHandler, line 449, I added the following code

} else if (keystroke == 13) {

evt.data.preventDefault();

}

This resolves the issue; however, the fix is not quite correct. As a side effect of this change, if the Ok button has focus and the enter key is pressed, the click action is not fired.

comment:8 Changed 12 years ago by Garry Yao

Status: confirmedpending

I'm unable to reproduce it on IE with the attached sample.

comment:9 Changed 12 years ago by Jakub Ś

@garry.yao I have modified the description, could you please test the attached sample once more?

You have to press Enter once or twice. With one of those "Enters" focus will go to submit button.

Last edited 12 years ago by Jakub Ś (previous) (diff)

comment:10 Changed 12 years ago by Jakub Ś

Keywords: IE9 added
Status: pendingconfirmed

Ok, I have checked this once more and problem is only reproducible in IE9.

Changed 12 years ago by Garry Yao

Attachment: 8985.patch added

comment:11 Changed 12 years ago by Garry Yao

Milestone: CKEditor 3.6.4
Owner: set to Garry Yao
Status: confirmedreview

comment:12 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Unfortunately the idea of making it work for text fields only is wrong. The ENTER action is not limited to that type of field only. It should work in the same way when over a combo or a checkbox or whichever UI element that doesn't handle ENTER.

In poor words, whenever ENTER reaches the dialog holder element, the OK button must be activated and the event must stop propagating. It doesn't matter what kind of element is focused.

Changed 12 years ago by Frederico Caldeira Knabben

Attachment: 8985_2.patch added

comment:13 Changed 12 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedreview

The new patch is based on the previous one, but making the event catching through the entire dialog, regardless the focused element.

It helps also fixing a regression of #4269 with FF, which was still happening when focused on the dialog tabs. It looks like something changed on the way FF handles key events.

comment:14 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Changed 12 years ago by Frederico Caldeira Knabben

Attachment: 8985_3.patch added

comment:15 Changed 12 years ago by Frederico Caldeira Knabben

Status: assignedreview

This new patch is very similar to the previous, but is smarter on figuring out if the ENTER key should be accepted by the focused element.

comment:16 Changed 12 years ago by Garry Yao

Status: reviewreview_failed
  1. Open link dialog;
  2. Type something into the URL field;
  3. Click on "Protocol" select field to open drop-down;
  4. Press enter key
  • Actual: Dialog get closed;
  • Expected: Dropdown get closed and focus return to the select element.

Thea above applies also to the ESC key.

Changed 12 years ago by Frederico Caldeira Knabben

Attachment: 8985_4.patch added

comment:17 Changed 12 years ago by Frederico Caldeira Knabben

Status: review_failedreview

So it looks like we have no alternative other than not enabling the ok button when ENTER on select elements.

Btw, I've noticed that ENTER has the "click" action on closed select boxes as well. This can be checked on the link dialog. Just select Link Type with the keyboard, without opening the select box, and hit ENTER. You'll see that ENTER changes the dialog accordingly. This helps justifying the proposed patch.

comment:18 Changed 12 years ago by Garry Yao

Status: reviewreview_failed

Thea above applies also to the ESC key.

This comment is not taken into the new patch.

comment:19 Changed 12 years ago by Garry Yao

Status: review_failedreview_passed

Ok, one justification is that it may looks strange if ESC doesn't work consistently across all fields... and cosnidering that we have that same behavior on v3, let's keep it the current way.

comment:20 Changed 12 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [7516].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy