Skip to content

Draggable: fix spurious blur in dialogs on mousedown #1730

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

Closed
wants to merge 2 commits into from
Closed

Draggable: fix spurious blur in dialogs on mousedown #1730

wants to merge 2 commits into from

Conversation

AttackTheDarkness
Copy link

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.

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.

testHelper.move( element, 1, 1 );

assert.strictEqual( document.activeElement, focusElement.get( 0 ), "test element is focused after mousing down off the handle" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the focus not move off of the test element after clicking outside of the handle?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because firing a mousedown event in an element won't force a blur (although a manual mouse interaction will).

The blur in question is manually triggered by the draggable element. This test is meant to make sure the draggable only does its manual blur if the mousedown occurs in the handle; otherwise it shouldn't do any special handling, since the event is otherwise ignored by the control.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that the test is asserting the opposite of the expected behavior. When we finally move to WebDriver, this test would be correct, but for now, I'd prefer if we just mock $.ui.safeBlur() to verify the logic.

Would you mind updating the test to do that? If you need any help, just let us know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough... I don't like that part either :-P

Hooking into safeBlur sounds good. I'll get on that.

Should be less brittle this way, and clearer about what it's trying to test.
@AttackTheDarkness
Copy link
Author

OK, good to go.

@AttackTheDarkness AttackTheDarkness deleted the dragblur branch August 3, 2017 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants