Skip to content

Commit

Permalink
Fix menu positioning when opened via keyboard. Fixes #17769.
Browse files Browse the repository at this point in the history 
Changes include:
  - make keydown handler process context-menu key in addition to shift-F10
  - (for keyboard) open menu around focused node rather than node matching
    this.targetNodeIds / this.selector
  - different approach to avoid repeat contextmenu key event;
    1ms delay in _scheduleOpen() was too short; there's about 100ms between
    the keydown and contextmenu events on IE8
  • Loading branch information
@wkeese
wkeese committed Mar 3, 2014
1 parent d641c3f commit a160390
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 63 deletions.
57 changes: 38 additions & 19 deletions Menu.js
View file
Expand Up @@ -55,7 +55,7 @@ define([

// selector: String?
// CSS expression to apply this Menu to descendants of targetNodeIds, rather than to
// the nodes specified by targetNodeIds themselves. Useful for applying a Menu to
// the nodes specified by targetNodeIds themselves. Useful for applying a Menu to
// a range of rows in a table, tree, etc.
//
// The application must require() an appropriate level of dojo/query to handle the selector.
Expand Down Expand Up @@ -164,20 +164,34 @@ define([
self = this;
return [
on(cn, delegatedEvent(this.leftClickToOpen ? "click" : "contextmenu"), function(evt){
// Schedule context menu to be opened unless it's already been scheduled from onkeydown handler
evt.stopPropagation();
evt.preventDefault();

if((new Date()).getTime() < this._lastKeyDown + 500){
// Ignore contextmenu/click events that were already processed in keydown handler below.
// But still call preventDefault() (above) so system context menu doesn't appear.
return;
}

// Schedule context menu to be opened.
// Note that this won't work will if the click was generated by the keyboard, while
// focused on a <button> etc. In that case evt.pageX and evt.pageY are either (0,0) or
// wherever the mouse cursor is is.
self._scheduleOpen(this, iframe, {x: evt.pageX, y: evt.pageY});
// wherever the mouse cursor is. See keydown handler below.
self._scheduleOpen(this, iframe, {x: evt.pageX, y: evt.pageY}, evt.target);
}),
on(cn, delegatedEvent("keydown"), function(evt){
if(evt.shiftKey && evt.keyCode == keys.F10){
if(evt.keyCode == 93 || // context menu key
(evt.shiftKey && evt.keyCode == keys.F10) || // shift-F10
(this.leftClickToOpen && evt.keyCode == keys.SPACE) // space key
){
evt.stopPropagation();
evt.preventDefault();
self._scheduleOpen(this, iframe); // no coords - open near target node

// Open the menu around evt.target. Note that "this" and evt.target
// are likely different, especially for global context menu, where "this" is <body>.
self._scheduleOpen(this, iframe, null, evt.target); // no coords - open near evt.target

this._lastKeyDown = (new Date()).getTime();
}
})
];
Expand Down Expand Up @@ -243,23 +257,26 @@ define([
}
},

_scheduleOpen: function(/*DomNode?*/ target, /*DomNode?*/ iframe, /*Object?*/ coords){
_scheduleOpen: function(delegatedTarget, iframe, coords, target){
// summary:
// Set timer to display myself. Using a timer rather than displaying immediately solves
// two problems:
//
// 1. IE: without the delay, focus work in "open" causes the system
// context menu to appear in spite of stopEvent.
//
// 2. Avoid double-shows on linux, where shift-F10 generates an oncontextmenu event
// even after a evt.preventDefault(). (Shift-F10 on windows doesn't generate the
// oncontextmenu event.)
// IE problem: without the delay, focus work in "open" causes the system
// context menu to appear in spite of evt.preventDefault().
// delegatedTarget: Element
// The node specified in targetNodeIds or matching selector that the menu is being opened for.
// iframe: HTMLIframeElement?
// Set if target is inside the specified iframe.
// coords: Object
// x/y position to center the menu around. Undefined if menu was opened via keyboard.
// target: Element
// The actual clicked node, either delegatedTarget or a descendant.

if(!this._openTimer){
this._openTimer = this.defer(function(){
delete this._openTimer;
this._openMyself({
target: target,
delegatedTarget: delegatedTarget,
iframe: iframe,
coords: coords
});
Expand All @@ -273,10 +290,12 @@ define([
// args:
// This is an Object containing:
//
// - target: The node that is being clicked
// - target: The node that is being clicked.
// - delegatedTarget: The node from this.targetNodeIds or matching this.selector,
// either the same as target or an ancestor of target.
// - iframe: If an `<iframe>` is being clicked, iframe points to that iframe
// - coords: Put menu at specified x/y position in viewport, or if iframe is
// specified, then relative to iframe.
// - coords: Mouse cursor x/y coordinates. Null when opened via keyboard.
// Put menu at specified position in iframe (if iframe specified) or otherwise in viewport.
//
// _openMyself() formerly took the event object, and since various code references
// evt.target (after connecting to _openMyself()), using an Object for parameters
Expand All @@ -288,7 +307,7 @@ define([
byKeyboard = !coords;

// To be used by MenuItem event handlers to tell which node the menu was opened on
this.currentTarget = target;
this.currentTarget = args.delegatedTarget;

// Get coordinates to open menu, either at specified (mouse) position or (if triggered via keyboard)
// then near the node the menu is assigned to.
Expand Down
91 changes: 47 additions & 44 deletions tests/robot/Menu_a11y.html
View file
Expand Up @@ -14,10 +14,10 @@
<script type="text/javascript">
require([
"doh/runner", "dojo/robotx",
"dojo/dom", "dojo/dom-class", "dojo/dom-geometry",
"dojo/_base/array", "dojo/dom", "dojo/dom-class", "dojo/dom-geometry",
"dojo/keys", "dojo/query", "dojo/sniff",
"dijit/tests/helpers", "dojo/domReady!"
], function(doh, robot, dom, domClass, domGeom, keys, query, has, helpers){
], function(doh, robot, array, dom, domClass, domGeom, keys, query, has, helpers){

robot.initRobot('../test_Menu.html');

Expand Down Expand Up @@ -691,49 +691,52 @@
// Run test about opening context menu via keyboard, except on safari/mac where that isn't
// possible (#9927)
if(!has("mac") || !has("webkit")){
doh.register("Context menu keyboard tests", [{
name: "open global context menu (keyboard)",
timeout: 5000,
runTest: function(){
var d = new doh.Deferred();

// Put focus on the link; this is just a random place on the screen to have focus
robot.sequence(function(){
dom.byId("link").focus();
}, 500, 500);

// open via keyboard
if(has("mac")){
robot.keyPress(keys.SPACE, 500, {
ctrl: true
});
}else{
robot.keyPress(keys.F10, 500, {
shift: true
});
var tests = has("mac") ?
[
{label: "ctrl-space", key: keys.SPACE, options: {ctrl: true}}
] :
[
{label: "context menu key", key: 525, options: {}}, // 93 in JS but 525 in Java
{label: "shift-F10", key: keys.F10, options: {shift: true}}
];
doh.register("Context menu keyboard tests", array.map(tests, function(test){
return {
name: "open global context menu via " + test.label,
timeout: 5000,
runTest: function(){
var d = new doh.Deferred();

// Focus random element to make sure menu is opened near focus point.
robot.sequence(function(){
dom.byId("input").focus();
}, 500);

// open via keyboard
robot.keyPress(test.key, 500, test.options);

robot.sequence(d.getTestCallback(function(){
var menu = registry.byId("windowContextMenu");
doh.t(helpers.isVisible(menu), "menu is now shown");

var menuCoords = domGeom.position(menu.domNode),
aroundCoords = domGeom.position("input");

// Check that menu's top left corner is near focus point,
// rather than upper-left hand corner of browser window.
doh.t(menuCoords.x < aroundCoords.x + 100, "x < 100", "actual x: " + menuCoords.x);
doh.t(menuCoords.y < aroundCoords.y + 50, "y < 50", "actual y: " + menuCoords.y);
doh.t(menuCoords.x >= aroundCoords.x, "x >= 0", "actual x: " + menuCoords.x);
doh.t(menuCoords.y >= aroundCoords.y, "y >= 0", "actual y: " + menuCoords.y);
doh.t(domClass.contains("windowContextMenuFirstChoice", "dijitMenuItemSelected"),
"first choice selected");

robot.keyPress(keys.ESCAPE, 0, {}, true); // close context menu
}), 1000);

return d;
}


robot.sequence(d.getTestCallback(function(){
var menu = registry.byId("windowContextMenu");
doh.t(helpers.isVisible(menu), "menu is now shown");

var menuCoords = domGeom.position(menu.domNode),
linkCoords = domGeom.position("link");

doh.t(menuCoords.x < 100, "x < 100", "actual x: " + menuCoords.x);
doh.t(menuCoords.y < 50, "y < 50", "actual y: " + menuCoords.y);
doh.t(menuCoords.x >= 0, "x >= 0", "actual x: " + menuCoords.x);
doh.t(menuCoords.y >= 0, "y >= 0", "actual y: " + menuCoords.y);
doh.t(domClass.contains("windowContextMenuFirstChoice", "dijitMenuItemSelected"),
"first choice selected");

robot.keyPress(keys.ESCAPE, 0, {}, true); // close context menu
}), 1000);

return d;
}
}]);
};
}));
}

doh.register("Cancellation (ESCAPE) tests", [
Expand Down
2 changes: 2 additions & 0 deletions tests/test_Menu.html
View file
Expand Up @@ -330,6 +330,8 @@ <h3 style="margin-bottom: 2em;">Navigation menu:</h3>

<h1 class="testTitle">Dijit Menu System Test</h1>

<input id="input" value="random input">

<p>This page contains:</p>
<ul>
<li>"Navigation bar" Menu widget on left, a.k.a vertical MenuBar
Expand Down

0 comments on commit a160390

Please sign in to comment.