From 19931e292abd1715156d6c4d16db0371257206c9 Mon Sep 17 00:00:00 2001 From: Ryan Oriecuia Date: Tue, 16 Aug 2016 16:52:15 -0700 Subject: [PATCH 1/2] Draggable: fix spurious blur in dialogs on mousedown I was running into a problem with a popup menu control in a dialog; clicks weren't working (but keyboard was working fine). It turned out that the menu was getting destroyed before the click event could fire. Tracked down the issue to the way draggable blurs focused controls; it was doing the blur before it ran through the logic to figure out if the drag was actually on the handle. I've moved the blur below these checks, so it'll only blur things if it actually needs to handle the drag. Otherwise, it asserts no opinion on what should and shouldn't be focused, which seems like the way things ought to be. Also, added a unit test to check for the expected behavior. --- tests/unit/draggable/core.js | 21 +++++++++++++++++++++ ui/widgets/draggable.js | 11 +++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/tests/unit/draggable/core.js b/tests/unit/draggable/core.js index 2ec102598da..bbc5a7dd439 100644 --- a/tests/unit/draggable/core.js +++ b/tests/unit/draggable/core.js @@ -338,6 +338,27 @@ QUnit.test( "blur behavior - descendant of handle", function( assert ) { } ); } ); +QUnit.test( "blur behavior - off handle", function( assert ) { + var ready = assert.async(); + assert.expect( 3 ); + + var element = $( "#draggable2" ).draggable( { handle: "span" } ), + focusElement = $( "
" ).appendTo( element ); + + testHelper.onFocus( focusElement, function() { + assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused before mousing down on a draggable" ); + + testHelper.move( focusElement, 1, 1 ); + + assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused after mousing down on itself" ); + + testHelper.move( element, 1, 1 ); + + assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused after mousing down off the handle" ); + ready(); + } ); +} ); + QUnit.test( "ui-draggable-handle assigned to appropriate element", function( assert ) { assert.expect( 5 ); diff --git a/ui/widgets/draggable.js b/ui/widgets/draggable.js index 6b862d0d0ad..9e46c81bb00 100644 --- a/ui/widgets/draggable.js +++ b/ui/widgets/draggable.js @@ -103,8 +103,6 @@ $.widget( "ui.draggable", $.ui.mouse, { _mouseCapture: function( event ) { var o = this.options; - this._blurActiveElement( event ); - // Among others, prevent a drag on a resizable-handle if ( this.helper || o.disabled || $( event.target ).closest( ".ui-resizable-handle" ).length > 0 ) { @@ -117,6 +115,8 @@ $.widget( "ui.draggable", $.ui.mouse, { return false; } + this._blurActiveElement( event ); + this._blockFrames( o.iframeFix === true ? "iframe" : o.iframeFix ); return true; @@ -147,11 +147,10 @@ $.widget( "ui.draggable", $.ui.mouse, { var activeElement = $.ui.safeActiveElement( this.document[ 0 ] ), target = $( event.target ); - // Only blur if the event occurred on an element that is: - // 1) within the draggable handle - // 2) but not within the currently focused element + // Don't blur if the event occurred on an element that is within + // the currently focused element // See #10527, #12472 - if ( this._getHandle( event ) && target.closest( activeElement ).length ) { + if ( target.closest( activeElement ).length ) { return; } From b086e911d268fddea99fef8a15428bd5ffd495cd Mon Sep 17 00:00:00 2001 From: Ryan Oriecuia Date: Thu, 8 Sep 2016 10:05:15 -0700 Subject: [PATCH 2/2] Draggable: Modified draggable unit test to mock out safeBlur Should be less brittle this way, and clearer about what it's trying to test. --- tests/unit/draggable/core.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tests/unit/draggable/core.js b/tests/unit/draggable/core.js index bbc5a7dd439..f41c8a53005 100644 --- a/tests/unit/draggable/core.js +++ b/tests/unit/draggable/core.js @@ -345,16 +345,25 @@ QUnit.test( "blur behavior - off handle", function( assert ) { var element = $( "#draggable2" ).draggable( { handle: "span" } ), focusElement = $( "
" ).appendTo( element ); + // mock $.ui.safeBlur with a spy + var _safeBlur = $.ui.safeBlur; + var blurCalledCount = 0; + $.ui.safeBlur = function() { + blurCalledCount++; + }; + testHelper.onFocus( focusElement, function() { assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused before mousing down on a draggable" ); - testHelper.move( focusElement, 1, 1 ); + testHelper.move( element, 1, 1 ); + assert.strictEqual( blurCalledCount, 0, "draggable doesn't blur when mousing down off handle" ); - assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused after mousing down on itself" ); + testHelper.move( element.find( "span" ), 1, 1 ); + assert.strictEqual( blurCalledCount, 1, "draggable blurs when mousing down on handle" ); - testHelper.move( element, 1, 1 ); + // Restore safeBlur + $.ui.safeBlur = _safeBlur; - assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused after mousing down off the handle" ); ready(); } ); } );