#12145 closed defect (fixed)
[Patch][CLA] Dialog: can lose focus if the first element is a radio button
Reported by: | Jason Priestley | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | 1.6 |
Component: | Dijit - Form | Version: | 1.5 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
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.
Attachments (1)
Change History (12)
comment:1 Changed 10 years ago by
Summary: | [Patch][CLA] dijit.Dialog can lose focus if the first element is a radio button → [Patch][CLA] Dialog: can lose focus if the first element is a radio button |
---|
comment:3 Changed 10 years ago by
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>
comment:4 Changed 10 years ago by
Hmm, good point. It should be okay if we simply add a check that the type is "radio" into the conditional.
comment:5 Changed 10 years ago by
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.
comment:6 Changed 10 years ago by
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)?
comment:8 Changed 10 years ago by
Latest version changes _getTabNavigable, and is entirely agnostic about attribute capitalization.
Changed 10 years ago by
Attachment: | dijit.dialog.fix.diff added |
---|
comment:9 Changed 10 years ago by
Milestone: | tbd → 1.6 |
---|---|
Owner: | set to bill |
Status: | new → assigned |
Thanks, this looks good, I'll just run the full regression suite and assuming it's OK I'll check it in!
comment:10 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:11 Changed 10 years ago by
Component: | Dijit → Dijit - Form |
---|
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.