Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dialog: Only focus the dialog if mousedown is not in the titlebar close button #828

Closed
wants to merge 40 commits into from

Conversation

@kborchers
Copy link
Member

kborchers commented Nov 18, 2012

I would appreciate a review of this before I merge it since it feels dirty but I thought for way too long about a different way to do this and couldn't come up with one. Just want to make sure I didn't miss anything obvious.

jzaefferer added 30 commits Oct 26, 2012
…he dialog if there is nothing yet in the dialog content. Partial fix for:
…fault null, as it should be. No back-compat, as the behaviour doesn't change.
… refactoring _setOption, no need for switch.
…d for the uiDialog closure. Use this.uiDialog and remove the variable.
… instead. Wasn't needed anymore (previous refactorings).
…. Fixes #6830 - Allow Icons to be specified for Dialog buttons.
@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Nov 18, 2012

Makes sense to me. @scottgonzalez what do you think?

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 19, 2012

I suppose you can delegate the mousedown, and cancel the event when it bubbles to .ui-dialog-titlebar-close, otherwise allow focus to happen. Would that be a better alternative? Again, I'm just getting up to speed here, so if my suggestion makes no sense... ignore it. :-)

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 19, 2012

I would also ask, as I always do, if there's a unit test you can write for this.

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Nov 19, 2012

I haven't dug into the details here. Do we know why focusing causes the shift?

@kborchers

This comment has been minimized.

Copy link
Member Author

kborchers commented Nov 19, 2012

It seems like it may be something with the size of the dialog changing and then regaining focus causes position to do a fit.

@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Nov 19, 2012

This addresses http://bugs.jqueryui.com/ticket/8789 - right? The commit message should include that.

@kborchers

This comment has been minimized.

Copy link
Member Author

kborchers commented Nov 20, 2012

The commit message does include that, I just didn't copy it all into the PR message.

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented on ui/jquery.ui.dialog.js in c77ca67 Nov 20, 2012

We shouldn't pass icons and showText here, we need to pull them out of the hash before this call.

This comment has been minimized.

Copy link
Member Author

jzaefferer replied Nov 21, 2012

This comment has been minimized.

Copy link
Member Author

jzaefferer replied Nov 22, 2012

Landed in e88ddf2

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented on ui/jquery.ui.dialog.js in c77ca67 Nov 20, 2012

Why does this work when showText isn't defined? Should button see it as a falsey value and not show the text?

This comment has been minimized.

Copy link
Member Author

jzaefferer replied Nov 21, 2012

When text is undefined because showText was undefined, the default true is used. Seems fine to me.

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented on 9fe3a62 Nov 20, 2012

We should be deprecating the string format as well. Also, we should use $.widget() for extensions/overrides.

This comment has been minimized.

Copy link
Member Author

jzaefferer replied Nov 21, 2012

Done in e88ddf2 - I've removed the fallback to $.ui.dialog.prototype.options.position if options.position is falsey. We don't support invalid options elsewhere (and it simplifies the code quite a bit).

This comment has been minimized.

Copy link
Member

scottgonzalez replied Nov 25, 2012

Thanks, that commit looks good. I updated the ticket titles.

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 22, 2012

@scottgonzalez I'd like to land this as it now fixes two bugs: 8838. Are we ok with the approach here?

@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Nov 22, 2012

We still don't know what is actually causing the two issues. The fiddle involving tabs can probably reduced further as well.

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 24, 2012

@jzaefferer, are you saying not to land this until we figure out the root cause? I'm not opposed to that, I'm just trying to figure out the status of this PR.

@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Nov 24, 2012

Yes, I want to dig further into this.

Also an alternative solution might be to reposition the dialog when the
height option is changed. That seems to make more sense and would avoid
this issue.

@kborchers

This comment has been minimized.

Copy link
Member Author

kborchers commented Nov 25, 2012

Following @jzaefferer's recommendation, this new commit repositions after resize. It appears to fix the issue in a much simpler way. Only problem is I can't seem to replicate the issue as a unit test. Simulating mousedown on the close button doesn't cause it to reposition like it does with actual mouse interactions.

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 25, 2012

@kborchers, use firebug and monitorEvents() to see the events you need to simulate to faithfully reproduce this. That's what I did when I was have trouble with the same on my last commit.

@mikesherov

This comment has been minimized.

Copy link
Member

mikesherov commented Nov 25, 2012

@kborchers, does the new commit also address 8839? That may be simpler to write a test for.

@scottgonzalez

This comment has been minimized.

Copy link
Member

scottgonzalez commented Nov 25, 2012

The new implementation seems better. I'm ok with landing that.

@kborchers

This comment has been minimized.

Copy link
Member Author

kborchers commented Nov 26, 2012

OK, I have updated that commit to include a test so hopefully this can land now.

@jzaefferer jzaefferer closed this Nov 26, 2012
@jzaefferer

This comment has been minimized.

Copy link
Member

jzaefferer commented Nov 26, 2012

Thanks Kris. I saw this after merging your previous commit, so added the test in a68d5ca.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.