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

Draggable: fix spurious blur in dialogs on mousedown #1730

Closed
wants to merge 2 commits into from

Conversation

@AttackTheDarkness
Copy link
Contributor

commented Aug 17, 2016

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.

Ryan Oriecuia
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.

testHelper.move( element, 1, 1 );

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

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Aug 31, 2016

Member

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

This comment has been minimized.

Copy link
@AttackTheDarkness

AttackTheDarkness Sep 1, 2016

Author Contributor

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.

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Sep 8, 2016

Member

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.

This comment has been minimized.

Copy link
@AttackTheDarkness

AttackTheDarkness Sep 8, 2016

Author Contributor

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

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

Ryan Oriecuia
Draggable: Modified draggable unit test to mock out safeBlur
Should be less brittle this way, and clearer about what it's trying to test.
@AttackTheDarkness

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2016

OK, good to go.

@AttackTheDarkness AttackTheDarkness deleted the AttackTheDarkness:dragblur branch Aug 3, 2017

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