Opened 9 years ago

Closed 9 years ago

#12146 closed enhancement (invalid)

[Patch][CLA] Improve refocus behavior for keyboard navigation

Reported by: Jason Priestley Owned by:
Priority: high Milestone: tbd
Component: Dijit Version: 1.5
Keywords: Cc: Becky Gibson
Blocked By: Blocking:

Description

The current refocus behavior on dialogs and menus is not very helpful for keyboard navigation. The menu refocus assumes that the previously focused element should be refocused, but in the case of keyboard navigation that will usually be another menu item. A better choice is the last element focused before anything under the menu was focused.

The patch I'm submitting unifies the different refocus implementations under the focus manager. The focus manager keeps track of any widgets with the property "refocus" which receive focus, and pushes the last focused element on to the stack. When these elements close they need to call "dijit.popFocus" to remove themselves from the stack, and refocus the previous element on the stack. Work done on behalf of TeamPatent.

Attachments (2)

dijit.focus.diff (7.3 KB) - added by Jason Priestley 9 years ago.
dijit.focus.2.diff (9.6 KB) - added by Jason Priestley 9 years ago.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by Jason Priestley

Attachment: dijit.focus.diff added

comment:1 Changed 9 years ago by Jason Priestley

Whoops, component should be dijit.

comment:2 Changed 9 years ago by bill

Component: GeneralDijit
Owner: anonymous deleted

Hi again, thanks for the patch!

Menu_a11y.html already has a number of tests about focus during keyboard navigation. Are those tests insufficient, or do they need to be changed? Either way that test file needs to be updated to test your changes. Same for Dialog_a11y.html and !TooltipDialog_a11y.html

Also, you've added a bunch of functions with no comments. They should all have summaries and /*String*/ etc. to note the type of each parameter. Also, if any of the methods are private they should start with an underscore.

The "refocus" attribute itself also needs to be commented. Actually I don't understand the meaning of this parameter, and why only certain elements have it. For example, shouldn't the button that launched a Dialog also get refocused? Or the FilteringSelect that launched a drop down? My other concern is backwards compatibility and whether focus for existing third party widgets that spawn drop downs will still work.

Oh, don't forget to run the full regression suite.

comment:3 Changed 9 years ago by Jason Priestley

Thanks for your comments. I'm working on a more polished patch now.

The "refocus" parameter causes the previously focused node to be saved when focus moves on to the "refocus" widget. The widget can restore focus after it closes by calling "popFocus". So the button that launches the dialog will be refocused, if it was the last node to receive focus prior to the dialog.

The widgets which I gave refocus to were based on typical use patterns: if you move from node A to a menu, and finish using the menu, you probably want to return to node A. But if you move from node A to a button, then when you leave that button there is no reason to expect that you'd like to return to node A.

comment:4 Changed 9 years ago by bill

I still don't see what's broken, I'll be looking forward to your test cases as a way to demonstrate the issue(s). I'd rather not require a refocus attribute since it seems like it can be determined automatic based on whenever dijit.popup.open() is called.

comment:5 Changed 9 years ago by Jason Priestley

I've added test cases. The test cases for dijit.Dialog are not changed, because the behavior is already good. When a dialog is closed, focus reverts to the dom node that was focused prior to opening it.

Really this patch just adds that kind of behavior to Menu (where it was only half-implemented) and TooltipDialog?, and unifies implementation.

I think refocus attribute needs to stay in for backward compatibility? Anyone using Menu or Dialog could have set refocus:false, in which case it should stay disabled. I don't think that Dialog uses dijit.popup.open, either.

Changed 9 years ago by Jason Priestley

Attachment: dijit.focus.2.diff added

comment:6 Changed 9 years ago by bill

Cc: Becky Gibson added

Ah you are right about Dialog, it doesn't even use dijit.popup.

I tried your Menu test file. The test is basically:

  1. focus on "random link"
  2. tab to MenuBar (focus lands on "File")
  3. open File menu, select "New"

After #3 (without your patch) focus goes nowhere, which is obviously bad, like you said. Your patch is making focus go back to "random link", rather than to "File" like I expected. Becky, is that OK? I can see that behavior being convenient if the user moved focus to the MenuBar by some shortcut key like Alt-F, in which case they would want focus to return to where they were before that.

For TooltipDialog, the test you added makes sense to me. The only thing is that the test passes against the current code, without your patch. Is there some bug is TooltipDialog that you are fixing with this patch?

About the refocus parameter, I agree we need to keep the parameter for backwards compatibility. There's still something strange though, just adding that parameter to true for things like TooltipDialog, what if there's a DropDownButton with a ColorPalette child? I don't see any reason for TooltipDialog to be treated as a special case.

comment:7 Changed 9 years ago by Jason Priestley

You're right about TooltipDialog passing without the patch - I guess I didn't actually check that. It seems that the DropDownButton is managing focus in this case. If the TooltipDialog were opened programmatically, then the new code would cause refocus, while the old code would lose focus.

I guess that currently, the DropDownButton takes care of refocus for many widgets? Maybe this behavior could be superseded by adding pushFocus/popFocus to dijit.popup.open/close? I haven't done much work yet with DropDownButtons, so I don't know all the ins and outs.

comment:8 Changed 9 years ago by bill

Right, the intended design is that focus is managed from _HasDropDown, or whatever code calls dijit.popup.open() and dijit.popup.close(), rather than the popup widgets such as TooltipDialog or ColorPalette. Popup widgets aren't even supposed to know that they are popup widgets; the idea is that any widget can be used as a popup.

Maybe this behavior could be superseded by adding pushFocus/popFocus to dijit.popup.open/close?

I guess I don't see much point to having a pushFocus/popFocus API, because we are just talking about one line of code in _HasDropDown.closeDropDown() to refocus the button.

comment:9 Changed 9 years ago by bill

Resolution: invalid
Status: newclosed

I'm going to close this for now, but please reopen if you have an example of when this code is needed.

Note: See TracTickets for help on using tickets.