Popup hh #344

Merged
merged 4 commits into from Jun 10, 2011

2 participants

@hanshillen

(reopened) Default keyboard mechanism for popup content.

@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
this.element.menu("focus", event, this.element.children( "li" ).first() );
+ this.element.focus();
@jzaefferer
jQuery Foundation member

why two focus() calls for menu?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
var that = this;
// use a timer to allow click to clear it and letting that
// handle the closing instead of opening again
that.closeTimer = setTimeout( function() {
that.close( event );
}, 100);
- }
+ },
+ focusin: function( event ) {
+ var that = this;
+ clearTimeout( that.closeTimer );
@jzaefferer
jQuery Foundation member

don't really need 'that' here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
@@ -80,19 +80,22 @@ $.widget( "ui.popup", {
});
this._bind(this.element, {
@jzaefferer
jQuery Foundation member

No need to pass this.element to _bind, that's the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
}
-
+ // TODO add other special use cases that differ from the default dialog style keyboard mechanism
+ else {
+ //default use case, popup could be anything (e.g. a form)
+ this.element
+ .bind( "keydown.ui-popup", function( event ) {
@jzaefferer
jQuery Foundation member

Should move this to _create - no need to bind every time when popup is opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
}
-
+ // TODO add other special use cases that differ from the default dialog style keyboard mechanism
+ else {
+ //default use case, popup could be anything (e.g. a form)
+ this.element
+ .bind( "keydown.ui-popup", function( event ) {
+ if ( event.keyCode !== $.ui.keyCode.TAB ) {
+ return;
+ }
+ var tabbables = $( ":tabbable", this ),
+ first = tabbables.filter( ":first" ),
+ last = tabbables.filter( ":last" );
@jzaefferer
jQuery Foundation member

Can use .first() and .last() here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
+ }
+ var tabbables = $( ":tabbable", this ),
+ first = tabbables.filter( ":first" ),
+ last = tabbables.filter( ":last" );
+ if ( event.target === last[0] && !event.shiftKey ) {
+ first.focus( 1 );
+ return false;
+ } else if ( event.target === first[0] && event.shiftKey ) {
+ last.focus( 1 );
+ return false;
+ }
+ });
+
+ // set focus to the first tabbable element in the popup container
+ // if there are no tabbable elements, set focus on the popup itself
+ var tabbables = this.element.find( ":tabbable" );
@jzaefferer
jQuery Foundation member

Maybe add this? .andSelf( ":tabbable" )
Should match the element if its already tabbable and avoid overriding the tabIndex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
+ first.focus( 1 );
+ return false;
+ } else if ( event.target === first[0] && event.shiftKey ) {
+ last.focus( 1 );
+ return false;
+ }
+ });
+
+ // set focus to the first tabbable element in the popup container
+ // if there are no tabbable elements, set focus on the popup itself
+ var tabbables = this.element.find( ":tabbable" );
+ if (!tabbables.length) {
+ this.element.attr("tabindex", "0");
+ tabbables.add(this.element);
+ }
+ tabbables.eq( 0 ).focus(1);
@jzaefferer
jQuery Foundation member

.first() == .eq( 0 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 26, 2011
ui/jquery.ui.popup.js
@@ -160,7 +192,8 @@ $.widget( "ui.popup", {
this.element
.hide()
.attr( "aria-hidden", true )
- .attr( "aria-expanded", false );
+ .attr( "aria-expanded", false )
+ .unbind( "keypress.ui-popup");
@jzaefferer
jQuery Foundation member

see above, or is there a reason to rebind this every time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
jQuery Foundation member

Works fine in Chrome now, but doesn't wrap in IE6 and 7.

@hanshillen

Jörn, I've handled your comments in the previous commit. The IE6/7 issue was caused by http://bugs.jqueryui.com/ticket/7438 and is related to the :focusable selector matching unfocusable form nodes. Since the issue is not related to popup itself, I worked around it for now by removing the form element from the popup demo's html.

@jzaefferer jzaefferer commented on an outdated diff May 31, 2011
ui/jquery.ui.popup.js
@@ -78,52 +78,76 @@ $.widget( "ui.popup", {
}, 1);
}
});
-
- this._bind(this.element, {
- // TODO use focusout so that element itself doesn't need to be focussable
- blur: function( event ) {
+
+ if ( !this.element.is( ":ui-menu" ) ) {
+ //default use case, wrap tab order in popup
+ this.element.bind( "keydown.ui-popup", function( event ) {
@jzaefferer
jQuery Foundation member

Should also use _bind here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff May 31, 2011
ui/jquery.ui.popup.js
+ //default use case, wrap tab order in popup
+ this.element.bind( "keydown.ui-popup", function( event ) {
+ if ( event.keyCode !== $.ui.keyCode.TAB ) {
+ return;
+ }
+
+ var tabbables = $( ":tabbable", this ),
+ first = tabbables.first(),
+ last = tabbables.last();
+
+ if ( event.target === last[ 0 ] && !event.shiftKey ) {
+ first.focus( 1 );
+ return false;
+ } else if ( event.target === first[ 0 ] && event.shiftKey ) {
+ last.focus( 1 );
+ return false;
@jzaefferer
jQuery Foundation member

We generally try to avoid using "return false" in event handlers now, to allow propagation. Should replace with event.preventDefault()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer
jQuery Foundation member

Good update. I'll ping Scott about the :focusable issue.

@hanshillen hanshillen commented on the diff Jun 10, 2011
ui/jquery.ui.popup.js
this.generatedRole = true;
}
-
+
this.options.trigger
.attr( "aria-haspopup", true )
.attr( "aria-owns", this.element.attr( "id" ) );

Using aria-owns here is causing a bug in JAWS:

JAWS will consider the dialog to be part of the trigger element, and because of that it will not announce the dialog itself before announcing the focused element inside of it when the popup expands. Only when moving focus away from both the trigger element and the popup and back into the popup JAWS will announce the dialog properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer merged commit 7ccb0e5 into jquery:master Jun 10, 2011
@jzaefferer
jQuery Foundation member

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment