Opened 10 years ago

Closed 10 years ago

#3575 closed Bug (fixed)

Insert special character not keyboard accessible

Reported by: Damian Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: IBM Confirmed Review+ Cc:

Description

This is the same issue as for the Smiley dialog. Ticket #3492 .

Attachments (6)

3575.patch (7.9 KB) - added by Tobiasz Cudnik 10 years ago.
Tested on FF3, IE6, IE7, IE8.
3575_2.patch (8.9 KB) - added by Tobiasz Cudnik 10 years ago.
Tested on FF3, IE7, Chrome2.
3575_3.patch (8.9 KB) - added by Tobiasz Cudnik 10 years ago.
3575_4.patch (9.3 KB) - added by Tobiasz Cudnik 10 years ago.
3575_5.patch (9.3 KB) - added by Tobiasz Cudnik 10 years ago.
3575_6.patch (9.7 KB) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 10 years ago by Tobiasz Cudnik

Keywords: Confirmed added

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575.patch added

Tested on FF3, IE6, IE7, IE8.

comment:3 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added

comment:4 Changed 10 years ago by Artur Formella

Keywords: Review- added; Review? removed

Review- because

  • characters are not keyboard accessible in Google Chrome
  • in IE7 and Safari arrows doesn't work at open (press tab to access)
  • changes in smiley.js aren't related

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575_2.patch added

Tested on FF3, IE7, Chrome2.

comment:5 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Second patch fixes:

  • webkit compatibility for both Characters and Smiles dialogs
  • IE7 needed delayed focus

comment:6 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

The logic should be ok. But can you change the links' style to display: block? It doesn't look good to have thin characters (e.g. the ! sign) highlighted by their outline.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575_3.patch added

comment:7 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Still not ok - the background color of the individual cells is gone in IE. Also the width and height of the cells are incorrect.

comment:9 Changed 10 years ago by Martin Kou

The problem should be caused by the modified attributes at the td cell generation code in L232 - L237. Also note that the href attribute at the td tag shouldn't be needed.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575_4.patch added

comment:10 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:11 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Ok... almost there. But still one more issue.

The cell highlight style is incorrect at times. For example, if you keep on pressing tab and wait for the focus to go from the last cell to the Cancel button, and then go back to the first cell - you'll see that now we have two cells that are highlighted, the last and the first. This is wrong.

There should be a blur event which will cancel the highlight from keyboard. And also, I can see you're sharing the onMouse* functions for both mouse events and keyboard events. It would be better to rename them to say what the functions are doing in this case.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575_5.patch added

comment:12 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

Ive refactored callback names to more describing and added missing events, which caused highlighting issues mentioned by Martin.

comment:13 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Hmm.. it's still not fixed. It is still possible to have more than 1 cell highlighted like this:

  1. Open special char dialog.
  2. Press tab a few times.
  3. Click on an empty area - the blurred cell is still highlighted.
  4. Press tab a few more times.
  5. Now you have two cells highlighted.

It seems to be me that it would be much simpler to hook the blur handler via onblur on the <a> tags, instead of adding it inside keyboard handlers.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3575_6.patch added

comment:14 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

This patch adds highlighted cell synchronization.

Using onBlur/Focus wont be as much simplification, because of mouse which still can highlight cells and it uses own events for that.

comment:15 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:16 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3664].

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy