Draggable: enabled draggable from within iframe. Fixed #5727 - draggable: cannot drag element inside iframe

[bgrins]

Fixes http://bugs.jqueryui.com/ticket/5727. This allows the ability to call $(element).draggable() when element is inside of an iframe.

Previously, all the mouse handlers were bound to parent document, this PR binds them to the owner document of the element being dragged.

Includes a visual test and minimal unit tests. I have been able to test in Chrome and Firefox.

This cleans up this PR: #909 based on feedback from @mikesherov

[mikesherov]

Hi @bgrins, you don't need to open new pull requests for feedback, you can just push up new commits. I have a few more comments to add, please just add and push up commits on the existing branch and the PR will reflect those changes automatically.

[mikesherov]

this.document[ 0 ] <--- notice the spacing.

[bgrins]

@mikesherov Sorry, I was unable to get a clean single commit with squashing after I had merged in from upstream. I was hoping that the discussion could happen on this branch, which is already flattened into a single commit. This way, I could just amend the existing commit to remain within the contribution guidelines. If it is important that the discussion happens over on the other PR I could give it another shot, but it had turned into a big time sink for me.

[mikesherov]

return this._mouseUp( event );

[mikesherov]

So I've asked you to clean up existing bad styles in the lines you've modified. Thanks for doing that! The last thing we need is a unit test supporting this change. Can you whip one up?

[mikesherov]

Append the iframe programmatically in the test please.

[bgrins]

@mikesherov I have pushed up a test, but it is not yet working. I think it could be because the $( handle ).simulate( "drag") is not working with the iframe inside of TestHelpers.draggable.testDrag. Also, for some reason the iframe's innerHTML seems to get reset after the qunit test, but I'm not sure that it is related.

I will take a look at it later.

[mikesherov]

Actually, I'll clean up the rest of your styles when I pull in these changes. But just be aware of the various spacing violations in case you'd like to contribute further! Thanks again!

Have you signed our CLA yet: http://contribute.jquery.org/CLA/ ?

[mikesherov]

Yeah, the HTML gets reset after every test to maintain test isolation, so that's not it. It might be due to simulate's weirdness. Please see if there's anything you need to change there to get it to work.

[bgrins]

@mikesherov I went ahead and updated the existing styles and will do so on the tests also. As far as I can tell, it is due to simulate, but I can't say for sure until I get some more time to dig into it. Failing message:

Expected: { "left": 58, "top": 58 } Result: { "left": 8, "top": 8 } 

[bgrins]

I looked into this a little bit more, and for some reason the _mouseDown and _mouseUp events inside of jquery.ui.mouse do get fired, but the _mouseMove (and _mouseMoveDelegate) do not get fired from the simulated call when the element is in an iframe. Still trying to figure out exactly why.

[bgrins]

This was the line that I needed to change to get the test to work.

[bgrins]

Ah, finally got the test working! Had to make a change to simulateDrag to use the owner document. Please review and let me know if changes are required.

[mikesherov]

Awesome! @bgrins, can you rebase this pull request?

[bgrins]

Ugh, now it seems to have split it up into multiple commits. I followed these instructions to rebase: https://www.openshift.com/wiki/github-workflow-for-submitting-pull-requests. Which resolved the conflicts and now the feature is working fine, but now the history seems to be messed up. Sorry, but do you have any documentation explaining what I should do instead?

[mikesherov]

@bgrins, assuming you have jquery/jquery-ui set to upstream and bgrins/jquery-ui as origin, do:

• git checkout master
• git pull upstream master
• git checkout b5727
• git rebase master
• If there are conflicts... make your edits, then do git add <filename> and then git rebase --continue.
• git push origin +b5727

[bgrins]

@mikesherov I have done this process, and afterwards I get this message:

> git rebase master Current branch b5727 is up to date. > git status # On branch b5727 # Your branch is ahead of 'upstream/master' by 7 commits. 

If I took a patch between the current branch and master, is there a way for me to wipe out all of my history to make it match with master, apply the patch, and push it all as one commit? Sorry for the hassle, I really don't know how it got so screwed up. It seems to have history from years ago when I first built the patch (in my master branch) along with some of the merges I did along the way when building it this time.

[mikesherov]

@bgrins, no worries. I'll rebase it for you when I land this... thanks!

[bgrins]

@mikesherov great! Is there anything else that I can do for this?

[mikesherov]

@bgrins Thanks! Landed in 24756a9