Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Dialog Redesign #787

Merged
merged 6 commits into from Oct 24, 2012

Conversation

Projects
None yet
5 participants
Owner

jzaefferer commented Oct 22, 2012

For now just for code review.

Just improved the moveToTop method to get rid of the event handling issues, see last commit for details.

/cc @neagle @artzstudio

@scottgonzalez scottgonzalez commented on an outdated diff Oct 22, 2012

ui/jquery.ui.dialog.js
@@ -69,8 +70,19 @@ $.widget("ui.dialog", {
show: null,
stack: true,
title: "",
- width: 300,
- zIndex: 1000
+ width: 300
+ },
+
+ _appendToTarget: function() {
+ var uiFront;
+
+ if ( this.options.appendTo ) {
+ return this.document.find( this.options.appendTo );
+ } else {
+ uiFront = this.element.closest(".ui-front");

@scottgonzalez scottgonzalez commented on an outdated diff Oct 22, 2012

ui/jquery.ui.dialog.js
@@ -93,8 +105,7 @@ $.widget("ui.dialog", {
.addClass( uiDialogClasses + options.dialogClass )
.css({
display: "none",
- outline: 0, // TODO: move to stylesheet
- zIndex: options.zIndex
+ outline: 0 // TODO: move to stylesheet
@scottgonzalez

scottgonzalez Oct 22, 2012

Owner

Let's just delete this.

@scottgonzalez scottgonzalez commented on an outdated diff Oct 22, 2012

ui/jquery.ui.dialog.js
@@ -93,8 +105,7 @@ $.widget("ui.dialog", {
.addClass( uiDialogClasses + options.dialogClass )
.css({
display: "none",
@scottgonzalez

scottgonzalez Oct 22, 2012

Owner

Let's change this to either .hide() or add display: none; to .ui-dialog.

@scottgonzalez scottgonzalez commented on an outdated diff Oct 22, 2012

ui/jquery.ui.dialog.js
@@ -330,7 +297,6 @@ $.widget("ui.dialog", {
this._position( options.position );
uiDialog.show( options.show );
this.overlay = options.modal ? new $.ui.dialog.overlay( this ) : null;
- this.moveToTop( true );
@scottgonzalez

scottgonzalez Oct 22, 2012

Owner

This needs to be restored. Restoring this will change the order of events though.

@scottgonzalez scottgonzalez commented on an outdated diff Oct 22, 2012

ui/jquery.ui.dialog.js
@@ -692,32 +652,12 @@ $.extend( $.ui.dialog.overlay, {
oldInstances: [],
maxZ: 0,
events: $.map(
- "focus,mousedown,mouseup,keydown,keypress,click".split( "," ),
+ "focus mousedown keydown keypress".split( " " ),
function( event ) {
return event + ".dialog-overlay";
}
).join( " " ),
create: function( dialog ) {
@scottgonzalez

scottgonzalez Oct 22, 2012

Owner

We can't just wholesale remove the overlay event functionality.

Owner

jzaefferer commented Oct 22, 2012

After some more cleanup we squashed all the changes so far into a single commit.

Owner

jzaefferer commented Oct 22, 2012

http://bugs.jqueryui.com/ticket/5968 - works fine in master: https://gist.github.com/1107b76f5c0b2b71250a
Need to track down where this got fixed.

Owner

jzaefferer commented Oct 22, 2012

based on z-index handling, which is now gone:
http://bugs.jqueryui.com/ticket/5466 - fixed
http://bugs.jqueryui.com/ticket/6267 - fixed
http://bugs.jqueryui.com/ticket/7051 - fixed (slider handle has z-index of 2, that didn't go well with the old z-index stuff)
http://bugs.jqueryui.com/ticket/7107 - fixed
http://bugs.jqueryui.com/ticket/7120 - fixed
http://bugs.jqueryui.com/ticket/5649 - fixed, probably (nothing to test with)

Owner

jzaefferer commented Oct 22, 2012

http://bugs.jqueryui.com/ticket/7960 - still a problem, due to high z-index on resziable handle

Owner

jzaefferer commented Oct 22, 2012

http://bugs.jqueryui.com/ticket/4995 - pretty sure this is fixed by the new overlay code
http://bugs.jqueryui.com/ticket/4892 - looks fixed, sure really sure
http://bugs.jqueryui.com/ticket/4421 - looks fixed (focus is retained), though I'm unable to get anything other then the default animated show (width/height/opacity), only setting the duration has an effect; very broken when using { show: "clip" }, the dialog ends up under the overlay

Owner

jzaefferer commented Oct 23, 2012

#313 - close
#324 - close
#510 - close
#546 - close
#749 - close
#779 - close

neagle and others added some commits Oct 15, 2012

@neagle @jzaefferer neagle Dialog: Awesome new stacking and overlay implementation. Fixes the fo…
…llowing tickets:

Fixes #3534 - Dialog: Modal dialog disables all input elements on page.
Fixes #4671 - Dialog: Modal Dialog disables vertical scroll bar in Chrome & Safari.
Fixes #4995 - Dialog: Modal Dialog's overlay dissapears in IE when content is tall.
Fixes #5388 - Dialog: Don't change z-index when already at the top.
Fixes #5466 - Dialog: "modal" Dialog Incorrectly Cancels Input Events.
Fixes #5762 - Dialog: Get rid of z-index workaround, document it instead.
Fixes #6267 - Dialog: checkboxes that inherit a z-index < jqueryui.dialog z-index don't work.
Fixes #7051 - Dialog: modal prevents tab key from moving focus off slider handle.
Fixes #7107 - Dialog: Modal dialog event loss with high zindex child elements (FF 3.6).
Fixes #7120 - Dialog: Modal operation interrupts drag drop marker functionality on gmaps.
Fixes #8172 - Dialog: Change event cancelled when opening modal dialog from another modal dialog.
Fixes #8583 - Dialog: Mouse event wrongly stopped.
Fixes #8722 - Dialog: Remove stack option.
Fixes #8729 - Dialog: Remove zIndex option.
3829a37
@jzaefferer jzaefferer Dialog: Prevent dialog form losing focus (or move it back in IE <= 8). 2a2a2c0
@jzaefferer jzaefferer Dialog: Save the active element that opened the dialog and restore fo…
…cus to that. Fixes #8730 - Dialog: Restore focus to opener.
14691ae
@DavidSouther @jzaefferer DavidSouther Dialog: Prevent tabbing off any dialog. Fixes #3768 - Dialog: contain…
… focus within dialog.
3a09a4a
@jzaefferer jzaefferer Dialog: Keep focus inside dialog, even when dialog itself has focus. 513b6da
@jzaefferer jzaefferer Dialog: Use _show and _hide consistently. Fixes #4892 - Dialog: zInde…
…x error with animated modal dialog.
d07074d

@jzaefferer jzaefferer merged commit d07074d into master Oct 24, 2012

Owner

jzaefferer commented Oct 24, 2012

Landed in master. Thanks @neagle for kicking this off last week! You just closed 14 tickets as fixed!

Contributor

neagle commented Oct 25, 2012

Great work, guys!

Owner

jzaefferer commented on 3829a37 Nov 26, 2012

@neagle Nate, could you sign our CLA? http://jquery.github.com/cla.html Thanks.

Contributor

neagle replied Nov 26, 2012

Done!

Owner

jzaefferer commented on 3a09a4a Nov 26, 2012

@DavidSouther David, could you sign our CLA? http://jquery.github.com/cla.html Thanks.

Contributor

DavidSouther replied Nov 26, 2012

My pleasure, and done.

Contributor

usmonster commented on ui/jquery.ui.dialog.js in 14691ae Nov 5, 2014

This call to .focus() will trigger unwanted scrolling in some cases. Maybe wrap this in a save/restore of the scrollTop/scrollLeft of the opener's scroll parent (if there is one)? Issue filed as http://bugs.jqueryui.com/ticket/10686. Please add your thoughts!

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