Opened 8 years ago

Last modified 7 years ago

#13839 review_failed Bug

Incorrect Tab navigation behavior with radio buttons inside the dialog

Reported by: Irina Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.7.1
Component: General Version: 4.3
Keywords: IBM Cc: Satya Minnekanti, giogio, Christophe Guillou

Description

Steps to reproduce

  1. Open Creating Captioned Images sample http://sdk.ckeditor.com/samples/captionedimage.html
  2. Double click on the image to get 'Image Properties' dialog
  3. Use Tab key to navigate inside the radio button group
  4. Keep pressing Tab key

Expected result

Tab key will enter the radio group.

When Tab or Shift+Tab into a radio group, focus goes to the selected radio button. If none is selected, focus goes to the first radio button if Tab was pressed, or the last radio bottom if Shift+Tab was pressed.

When focus is on any radio button, Tab or Shift+Tab will exit the radio group.

Up Arrow and Left Arrow moves focus to the previous radio button in the group, and selects that button. If focus is on the first item, then focus wraps to last item.

Down Arrow and Right Arrow moves focus to the next radio button in the group, and selects that button. If focus is on the last item, then focus wraps to first item.

Control+Arrow moves through the options without updating content or selecting the button.

Space selects the radio button with focus and de-selects other radio buttons in the group.

http://www.w3.org/TR/wai-aria-practices/#radiobutton

Actual result

When focus is on any radio button, Tab will not exit the radio group

Other details (browser, OS, CKEditor version, installed plugins)

Change History (20)

comment:1 Changed 8 years ago by Jakub Ś

Version: 4.5.44.3

Curretly the Tab is not exiting the group once radio button is selected.

comment:2 Changed 8 years ago by Marek Lewandowski

Status: newconfirmed

comment:3 Changed 8 years ago by Anna Tomanek

Summary: Incorrect tab navigation behavior with radio buttons inside the dialogIncorrect Tab navigation behavior with radio buttons inside the dialog

comment:4 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8

comment:5 Changed 8 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:6 Changed 8 years ago by Tomasz Jakut

Status: assignedreview

Ok, I've fixed the incorrect Tab behavior. Now Tab exits radio group if the focus is on any radio button. I've also added manual test for that.

Pushed branch:t/13839.

comment:7 Changed 8 years ago by Marek Lewandowski

@k.krzton I'll ask you for an early review on this one. Still I'll have a look on it, but it will be nice to have a feedback earlier, thanks!

comment:8 Changed 8 years ago by kkrzton

Looks good to me. I have only two small readability concerns, I would consider:

  1. if ( ( currentElem = focusList[ currentIndex ] ).role == 'radiogroup' ) { ... (Line 476) is the only places where value is assigned to currentElem. While it is inside the if it may suggest it will be used only inside it but it is also used later. Maybe it could be moved outside, like
currentElem = focusList[ currentIndex ];
if ( currentElem.role == 'radiogroup' ) { ...
  1. Rename variable startInsideRadioGroup to startsInsideRadioGroup. It indicates if focus is currently inside radiogroup so if focus starts inside radio group (correct me if I am wrong with this grammar stuff).

@m.lewandowski probably you would like to take a look before any changes are applied so I am not changing the task status.

comment:9 Changed 8 years ago by Marek Lewandowski

Thanks @k.krzton, I'll get this ticket updated soon.

comment:10 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9
Status: reviewreview_failed

+1 to both @k.krzton notes. In addition to that:

This issue needs to be moved to 4.5.9, there are couple of issues:

  1. Other CKE tests are failing (e.g. http://tests.ckeditor.dev:1030/tests/plugins/div/div - confirmed with chrome).
  2. Fix is applied in wrong place, instead of adding a fix in core/generic dialog method - the fix should be applied inside radio type.

This actually results with a more important change, we should fix focusList so that it does not contain x radio elements, but only radiogroup (radio wrapper) instead - and this object should know all the logic on how to determine focus position.

comment:11 Changed 8 years ago by Tomasz Jakut

Status: review_failedreview

Ok, I've moved whole fix into radio type definition. The code is de facto duplicated version of code in `plugins/dialog/plugin.js` that adds focus to the input element.

The issue with radiobutton group seems to be caused by the fact that dialog assumes there is always only one input element in the dialog's UI element, therefore not all event listeners were added and focus event was not properly propagated. The second part of the issue was caused by the fix for #10866 (which fixed the effect of already mentioned issue without fixing the cause of it) and adding focus to all radiobuttons separately instead of adding focus to the whole group.

Pushed changes to branch:t/13839.

Last edited 8 years ago by Tomasz Jakut (previous) (diff)

comment:12 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:13 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:14 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:15 Changed 7 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

comment:16 Changed 7 years ago by kkrzton

Status: reviewreview_failed

The solution looks quite solid, however:

  1. The issue description says something about Control + Arrow shortcut, but it does not seem to work. The linked specs (​http://www.w3.org/TR/wai-aria-practices/#radiobutton) does not say anything about this particular shortcut so I am not sure if it should be implemented?
  1. When there is no radio element selected by default, the error is thrown while focusing radiogroup (via Tab or Tab+Shift). We should also cover this case and add some manual test for it.
Uncaught TypeError: Cannot read property 'isVisible' of undefined
    at radio.isVisible (plugin.js:2914)
    at radio.isFocusable (plugin.js:2924)
    at changeFocus (plugin.js:468)
    at CKEDITOR.dialog.keydownHandler (plugin.js:494)
    at CKEDITOR.dom.element.listenerFirer (event.js:144)
    at CKEDITOR.dom.element.CKEDITOR.event.fire (event.js:290)
    at HTMLDivElement.<anonymous> (domobject.js:46)

comment:17 Changed 7 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2CKEditor 4.7.0

Moving to major release.

comment:18 Changed 7 years ago by Tomasz Jakut

Status: review_failedreview

I've changed definition of CKEDITOR.ui.dialog.radio.prototype.getInputElement to return first radiobutton in case of none selected.

I've also added a bunch of unit tests. Rebased to latest major and pushed branch:t/13839.

comment:19 Changed 7 years ago by Tade0

Status: reviewreview_failed

One complaint that I have is that the last test in http://tests.ckeditor.dev:1030/tests/plugins/dialogui/elements doesn't fail on major, even though it probably should.

comment:20 Changed 7 years ago by Marek Lewandowski

Milestone: CKEditor 4.7.0CKEditor 4.7.1
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