Skip to content

Commit

Permalink
Dialog: Inline code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@jzaefferer
jzaefferer committed Nov 26, 2012
1 parent 0a25b2c commit 2062a18
Showing 1 changed file with 53 additions and 5 deletions.
58 changes: 53 additions & 5 deletions ui/jquery.ui.dialog.js
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -68,30 +68,48 @@ $.widget("ui.dialog", {
resizable: true, resizable: true,
show: null, show: null,
title: "", title: "",
width: 300 width: 300,

// callbacks
beforeClose: null,
close: null,
drag: null,
dragStart: null,
dragStop: null,
focus: null,
open: null,
resize: null,
resizeStart: null,
resizeStop: null
}, },


_create: function() { _create: function() {
this.originalTitle = this.element.attr( "title" ); this.originalTitle = this.element.attr( "title" );
// #5742 - .attr() might return a DOMElement // #5742 - .attr() might return a DOMElement
// TODO WTF?
if ( typeof this.originalTitle !== "string" ) { if ( typeof this.originalTitle !== "string" ) {
this.originalTitle = ""; this.originalTitle = "";
} }
this.oldPosition = { this.oldPosition = {
parent: this.element.parent(), parent: this.element.parent(),
index: this.element.parent().children().index( this.element ) index: this.element.parent().children().index( this.element )
}; };
// TODO don't overwrite options
this.options.title = this.options.title || this.originalTitle; this.options.title = this.options.title || this.originalTitle;
var that = this, var that = this,
options = this.options, options = this.options,


// TODO make this the default for the title option?
title = options.title || " ", title = options.title || " ",
// TODO should use this.uiDialog instead
uiDialog, uiDialog,
// TODO should use this.uiDialogTitlebar instead
uiDialogTitlebar, uiDialogTitlebar,
uiDialogTitlebarClose, uiDialogTitlebarClose,
uiDialogTitle, uiDialogTitle,
uiDialogButtonPane; uiDialogButtonPane;


// TODO extract into _createWrapper
uiDialog = ( this.uiDialog = $( "<div>" ) ) uiDialog = ( this.uiDialog = $( "<div>" ) )
.addClass( uiDialogClasses + options.dialogClass ) .addClass( uiDialogClasses + options.dialogClass )
.hide() .hide()
Expand All @@ -115,9 +133,10 @@ $.widget("ui.dialog", {
.addClass( "ui-dialog-content ui-widget-content" ) .addClass( "ui-dialog-content ui-widget-content" )
.appendTo( uiDialog ); .appendTo( uiDialog );


// TODO extract this and the next three into a _createTitlebar method
uiDialogTitlebar = ( this.uiDialogTitlebar = $( "<div>" ) ) uiDialogTitlebar = ( this.uiDialogTitlebar = $( "<div>" ) )
.addClass( "ui-dialog-titlebar ui-widget-header " + .addClass( "ui-dialog-titlebar ui-widget-header ui-corner-all ui-helper-clearfix" )
"ui-corner-all ui-helper-clearfix" ) // TODO use _on, call _focusTabbable or _keepFocus
.bind( "mousedown", function() { .bind( "mousedown", function() {
// Dialog isn't getting focus when dragging (#8063) // Dialog isn't getting focus when dragging (#8063)
uiDialog.focus(); uiDialog.focus();
Expand All @@ -144,18 +163,21 @@ $.widget("ui.dialog", {
.html( title ) .html( title )
.prependTo( uiDialogTitlebar ); .prependTo( uiDialogTitlebar );


// TODO extract this one and the next into a _createButtonPane method
uiDialogButtonPane = ( this.uiDialogButtonPane = $( "<div>" ) ) uiDialogButtonPane = ( this.uiDialogButtonPane = $( "<div>" ) )
.addClass( "ui-dialog-buttonpane ui-widget-content ui-helper-clearfix" ); .addClass( "ui-dialog-buttonpane ui-widget-content ui-helper-clearfix" );


( this.uiButtonSet = $( "<div>" ) ) ( this.uiButtonSet = $( "<div>" ) )
.addClass( "ui-dialog-buttonset" ) .addClass( "ui-dialog-buttonset" )
.appendTo( uiDialogButtonPane ); .appendTo( uiDialogButtonPane );


// TODO move into _createWrapper
uiDialog.attr({ uiDialog.attr({
role: "dialog", role: "dialog",
"aria-labelledby": uiDialogTitle.attr( "id" ) "aria-labelledby": uiDialogTitle.attr( "id" )
}); });


// TODO move into _createWrapper
// We assume that any existing aria-describedby attribute means // We assume that any existing aria-describedby attribute means
// that the dialog content is marked up properly // that the dialog content is marked up properly
// otherwise we brute force the content as the description // otherwise we brute force the content as the description
Expand All @@ -165,7 +187,11 @@ $.widget("ui.dialog", {
}); });
} }


// TODO use andSelf()
// TODO get rid of this?! why do we need to disable selection anyway?
uiDialogTitlebar.find( "*" ).add( uiDialogTitlebar ).disableSelection(); uiDialogTitlebar.find( "*" ).add( uiDialogTitlebar ).disableSelection();

// TODO use button? or at least a button element, so that SPACE works?
this._hoverable( uiDialogTitlebarClose ); this._hoverable( uiDialogTitlebarClose );
this._focusable( uiDialogTitlebarClose ); this._focusable( uiDialogTitlebarClose );


Expand All @@ -176,10 +202,14 @@ $.widget("ui.dialog", {
this._makeResizable(); this._makeResizable();
} }


// TODO merge with _createButtonPane?
this._createButtons( options.buttons ); this._createButtons( options.buttons );

this._isOpen = false; this._isOpen = false;


// prevent tabbing out of dialogs // prevent tabbing out of dialogs
// TODO move into _createWrapper
// TODO fix formatting
this._on( uiDialog, { keydown: function( event ) { this._on( uiDialog, { keydown: function( event ) {
if ( event.keyCode !== $.ui.keyCode.TAB ) { if ( event.keyCode !== $.ui.keyCode.TAB ) {
return; return;
Expand Down Expand Up @@ -217,13 +247,15 @@ $.widget("ui.dialog", {
.removeUniqueId() .removeUniqueId()
.removeClass( "ui-dialog-content ui-widget-content" ) .removeClass( "ui-dialog-content ui-widget-content" )
.hide() .hide()
// TODO restore old position directly, instead of appending to body first
.appendTo( "body" ); .appendTo( "body" );
this.uiDialog.remove(); this.uiDialog.remove();


if ( this.originalTitle ) { if ( this.originalTitle ) {
this.element.attr( "title", this.originalTitle ); this.element.attr( "title", this.originalTitle );
} }


// TODO do this before removing the wrapper
next = oldPosition.parent.children().eq( oldPosition.index ); next = oldPosition.parent.children().eq( oldPosition.index );
// Don't try to place the dialog next to itself (#8613) // Don't try to place the dialog next to itself (#8613)
if ( next.length && next[ 0 ] !== this.element[ 0 ] ) { if ( next.length && next[ 0 ] !== this.element[ 0 ] ) {
Expand All @@ -244,6 +276,7 @@ $.widget("ui.dialog", {
return; return;
} }


// TODO fix yoda-if
if ( false === this._trigger( "beforeClose", event ) ) { if ( false === this._trigger( "beforeClose", event ) ) {
return; return;
} }
Expand All @@ -261,6 +294,7 @@ $.widget("ui.dialog", {
$( this.document[ 0 ].activeElement ).blur(); $( this.document[ 0 ].activeElement ).blur();
} }


// TODO shouldn't _hide restore `this` to the instance? would also help tooltip
this._hide( this.uiDialog, this.options.hide, function() { this._hide( this.uiDialog, this.options.hide, function() {
that._trigger( "close", event ); that._trigger( "close", event );
}); });
Expand All @@ -279,11 +313,14 @@ $.widget("ui.dialog", {


open: function() { open: function() {
if ( this._isOpen ) { if ( this._isOpen ) {
// TODO don't pass silent flag? should probably trigger focus when moving to top again
this.moveToTop( null, true ); this.moveToTop( null, true );
// TODO run this only when dialog wasn't focused?
this._focusTabbable(); this._focusTabbable();
return; return;
} }


// TODO remove useless tmp vars
var options = this.options, var options = this.options,
uiDialog = this.uiDialog; uiDialog = this.uiDialog;


Expand All @@ -304,6 +341,7 @@ $.widget("ui.dialog", {
return this; return this;
}, },


// TODO check if dialog already has focus, merge with _keepFocus
_focusTabbable: function() { _focusTabbable: function() {
// set focus to the first tabbable element in the content area or the first button // set focus to the first tabbable element in the content area or the first button
// if there are no tabbable elements, set focus on the dialog itself // if there are no tabbable elements, set focus on the dialog itself
Expand Down Expand Up @@ -342,6 +380,7 @@ $.widget("ui.dialog", {
this.uiDialogButtonPane.remove(); this.uiDialogButtonPane.remove();
this.uiButtonSet.empty(); this.uiButtonSet.empty();


// TODO use jQuery.isEmptyObject()
if ( typeof buttons === "object" && buttons !== null ) { if ( typeof buttons === "object" && buttons !== null ) {
$.each( buttons, function() { $.each( buttons, function() {
return !(hasButtons = true); return !(hasButtons = true);
Expand All @@ -363,6 +402,7 @@ $.widget("ui.dialog", {
button = $( "<button></button>", props ) button = $( "<button></button>", props )
.appendTo( that.uiButtonSet ); .appendTo( that.uiButtonSet );
if ( $.fn.button ) { if ( $.fn.button ) {
// TODO allow passing through button options
button.button(); button.button();
} }
}); });
Expand Down Expand Up @@ -408,6 +448,7 @@ $.widget("ui.dialog", {
}); });
}, },


// TODO why are handles passed by _setOption??
_makeResizable: function( handles ) { _makeResizable: function( handles ) {
handles = (handles === undefined ? this.options.resizable : handles); handles = (handles === undefined ? this.options.resizable : handles);
var that = this, var that = this,
Expand Down Expand Up @@ -472,6 +513,7 @@ $.widget("ui.dialog", {
isVisible; isVisible;


if ( position ) { if ( position ) {
// TODO we don't support 1.3.2 anymore, clean this mess up
// deep extending converts arrays to objects in jQuery <= 1.3.2 :-( // deep extending converts arrays to objects in jQuery <= 1.3.2 :-(
// if (typeof position == 'string' || $.isArray(position)) { // if (typeof position == 'string' || $.isArray(position)) {
// myAt = $.isArray(position) ? position : position.split(' '); // myAt = $.isArray(position) ? position : position.split(' ');
Expand Down Expand Up @@ -551,9 +593,11 @@ $.widget("ui.dialog", {
case "dialogClass": case "dialogClass":
uiDialog uiDialog
.removeClass( this.options.dialogClass ) .removeClass( this.options.dialogClass )
// TODO why adding uiDialogClasses again? we didn't remove those
.addClass( uiDialogClasses + value ); .addClass( uiDialogClasses + value );
break; break;
case "disabled": case "disabled":
// TODO use toggleClass( "ui-dialog-disabled", value )
if ( value ) { if ( value ) {
uiDialog.addClass( "ui-dialog-disabled" ); uiDialog.addClass( "ui-dialog-disabled" );
} else { } else {
Expand Down Expand Up @@ -591,7 +635,8 @@ $.widget("ui.dialog", {
} }
break; break;
case "title": case "title":
// convert whatever was passed in o a string, for html() to not throw up // convert whatever was passed in to a string, for html() to not throw up
// TODO deduplicate this (see _create)
$( ".ui-dialog-title", this.uiDialogTitlebar ) $( ".ui-dialog-title", this.uiDialogTitlebar )
.html( "" + ( value || "&#160;" ) ); .html( "" + ( value || "&#160;" ) );
break; break;
Expand Down Expand Up @@ -643,8 +688,8 @@ $.widget("ui.dialog", {
}); });


$.extend($.ui.dialog, { $.extend($.ui.dialog, {
// TODO remove these
uuid: 0, uuid: 0,

getTitleId: function($el) { getTitleId: function($el) {
var id = $el.attr( "id" ); var id = $el.attr( "id" );
if ( !id ) { if ( !id ) {
Expand All @@ -654,17 +699,20 @@ $.extend($.ui.dialog, {
return "ui-dialog-title-" + id; return "ui-dialog-title-" + id;
}, },


// TODO move to dialog instance method
overlay: function( dialog ) { overlay: function( dialog ) {
this.$el = $.ui.dialog.overlay.create( dialog ); this.$el = $.ui.dialog.overlay.create( dialog );
} }
}); });


// TODO get rid of instance list, at least the oldInstance stuff, and inline as dialog methods
$.extend( $.ui.dialog.overlay, { $.extend( $.ui.dialog.overlay, {
instances: [], instances: [],
// reuse old instances due to IE memory leak with alpha transparency (see #5185) // reuse old instances due to IE memory leak with alpha transparency (see #5185)
oldInstances: [], oldInstances: [],
create: function( dialog ) { create: function( dialog ) {
if ( this.instances.length === 0 ) { if ( this.instances.length === 0 ) {
// TODO get rid of the timeout, which should remove the need for the #4065 workaround as well
// prevent use of anchors and inputs // prevent use of anchors and inputs
// we use a setTimeout in case the overlay is created from an // we use a setTimeout in case the overlay is created from an
// event that we're going to be cancelling (see #2804) // event that we're going to be cancelling (see #2804)
Expand Down

0 comments on commit 2062a18

Please sign in to comment.