[Patch][CLA] Dialog: can lose focus if the first element is a radio button

[Jason Priestley]

The code which keeps focus inside the dialog does not deal with the case where a radio button is the first tabstop. In this case, the tab focus will fall on the selected radio button, which may not be the same as the first input. Work done on behalf of TeamPatent.

[bill]

Hi, thanks for the patch, can you add an automated test for this? Probably to Dialog_a11y.html, with corresponding changes in test_Dialog.html.

I don't follow exactly what you are saying, but it sounds like we need to test when the first and the last sections of a Dialog contain a set of radio buttons, and the middle one is selected rather than the first or last one.

[Jason Priestley]

Sure thing, added a test to Dialog_a11y.html

[bill]

Thanks. It looks like there's a bug in your patch though for the plain input (non radio button case), something like this?

<div data-dojo-type=dijit.Dialog>
     <input name=foo>
     <input name=foo>
</div>

[Jason Priestley]

Hmm, good point. It should be okay if we simply add a check that the type is "radio" into the conditional.

[bill]

Yah that looks better, assuming that the radio buttons are all grouped together, rather than having some random element splitting them up. That seems like a fair assumption / requirement.

I think there's a tiny bug in your code that it will fail for strings with different case (uppercase / lowercase), either:

<input type="radio">
<input type="RADIO">

or:

<input name="foo">
<input name="FOO">

I doubt either of those cases would ever happen but might as well correct the code to handle it anyway.

I think there's also another issue if the first elements in the Dialog or a TooltipDialog are radio buttons. The current focus code for TooltipDialog is:

focus: function(){
	// summary:
	//		Focus on first field
	this._getFocusItems(this.containerNode);
	dijit.focus(this._firstFocusItem);
},

Dialog has similar code for onLoad.

What will happen when the Dialog starts with some radio buttons and the selected radio button isn't the first radio button? I think it focuses on the first radio button rather than the selected one. Focusing the selected one would be better. It's sort of a separate issue but I mention it because maybe you can kill two birds with one stone by fixing _getFocusItems() to handle this case, to denote the selected radio button as the first focusable element?

If none of the radio buttons are selected I guess it should focus on the first (albeit unselected) one.

[Jason Priestley]

I think you're right that the fix should go in _getFocusItems() rather than in the key event handler. Is there any reason not to push it up even further to _getTabNavigable (in manager.js)?

[bill]

Agreed, _getTabNavigable() seems like the right place.

[Jason Priestley]

Latest version changes _getTabNavigable, and is entirely agnostic about attribute capitalization.

[bill]

Thanks, this looks good, I'll just run the full regression suite and assuming it's OK I'll check it in!

[bill]

(In [23560]) Patch for Dialog etc. to accurately handle initial focus and looping tab navigation when the first or last elements in the Dialog are radio buttons. Patch from jhpriestley (TeamPatent, CLA), thanks! Fixes #12145 !strict.