Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#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)

dijit.dialog.fix.diff (4.4 KB) - added by Jason Priestley 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by bill

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

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.

comment:2 Changed 9 years ago by Jason Priestley

Sure thing, added a test to Dialog_a11y.html

comment:3 Changed 9 years ago by 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>

comment:4 Changed 9 years ago by Jason Priestley

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

comment:5 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by 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)?

comment:7 Changed 9 years ago by bill

Agreed, _getTabNavigable() seems like the right place.

comment:8 Changed 9 years ago by Jason Priestley

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

Changed 9 years ago by Jason Priestley

Attachment: dijit.dialog.fix.diff added

comment:9 Changed 9 years ago by bill

Milestone: tbd1.6
Owner: set to bill
Status: newassigned

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 9 years ago by bill

Resolution: fixed
Status: assignedclosed

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

comment:11 Changed 8 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.