Incorrect Tab navigation behavior with radio buttons inside the dialog

[Irina]

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)

[Jakub Ś]

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

[Tomasz Jakut]

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.

[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!

[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' ) { ...

2. 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.

[Marek Lewandowski]

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

[Marek Lewandowski]

+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.

[Tomasz Jakut]

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.

[Marek Lewandowski]

Moving tickets to the next milestone.

[kkrzton]

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?

2. 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)

[Marek Lewandowski]

Moving to major release.

[Tomasz Jakut]

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.

[Tade0]

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.