Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Popup: Fixes closing the popup unexpectedly by race conditions. #668

Closed
wants to merge 4 commits into from

5 participants

@rxaviers
Collaborator
  • A focusout followed by a focusin sometimes trigger events in reverse order, but should not close the popup nevertheless. Or a click-in-the-popup followed by a focusout should also avoid closing it unexpectedly;
  • _closeTry and _closeVet work together to close the popup, but making sure no other event vets it;
rxaviers added some commits
@rxaviers rxaviers Popup: Added _closeTry and _closeVet methods. Fixes closing the popup…
… unexpectedly by race conditions.

- A focusout followed by a focusin sometimes trigger events in reverse order,
  but should not close the popup nevertheless. Or a click-in-the-popup followed
  by a focusout should also avoid closing it unexpectedly;
- _closeTry and _closeVet work together to close the popup, but making sure no
  other event vets it;
c6cce9c
@rxaviers rxaviers Popup: Binds document mousedown instead of click. The click event is …
…triggered when the mouse button is released, which virtually always exceeds _closeTry delay, leading to erroneous popup close.
96c6b94
@rxaviers rxaviers Popup: Manually binding document instead of using _bind. Because on d…
…estroy, it doesn't unbind other popup instances.
aa135de
@rxaviers rxaviers Popup: Lowered the closing delay from 150ms to 50ms. Makes response f…
…aster since the _closeTry and _closeVet approach is working smooth.
a3cd4fd
@jzaefferer
Owner

_closeVet and _closeTry are weird named that don't match any convention used in other components. Should try to come up with something else.

@rxaviers
Collaborator

I agree they are not common names. Can you guys help me with suggestions please? For now, I can help by summarizing the implementation dynamics below:

Currently, the popup closes unexpectedly due to race conditions. The easiest example to observe this behavior (before this patch) is to click in the popup and hold the button down. The popup will close, because the popup loses focus and is not followed by a "vetting" event like mouseup. Another example happened to me when I had nested popups: the reason the popup was closing unexpectedly was because the focusin event (of the inner popup) was occasionally being triggered before the focusout (of the parent popup).

The order of the events doesn't matter. But, the time-distance between them. So, to solve the above scenario, basically what I did was:
a) Closing events (like focusout, or clicking outside the popup) still "tries" to close the popup unless it's "vetted" (like it was implemented before);
b) A "vetting" event (like focusin, or clicking in the popup) still cancels any previous closing attempt, and also creates a temporary shield against short future closing events;

@rxaviers
Collaborator

_closeTry and _closeAbort?

@mikesherov
Collaborator

@rxaviers, do you have unit tests and a test case for this pull request? I'd like to know whether to close this or if you're going to continue to pursue this PR. Thanks man!

@rxaviers
Collaborator

Hi @mikesherov, I think this is still worth. Although, it's a little old and it needs a review. I will do it and provide the unit test soon.

@jzaefferer
Owner

@rxaviers still want to update this? Would have to starting using _on/_off instead of bind and unbind, along with the tests.

@rxaviers
Collaborator
@scottgonzalez

Closing due to inactivity. If you get around to the other updates, we can re-open or start a new PR. The fate of popup isn't clear to me right now.

@rxaviers
Collaborator

Makes sense... Sorry for the inactivity in here. I will (hopefully) update this and reopen.
PS: the quickest test is: open demos/popup/animation.html, click Log In to open the popup. Then, click on the 'Username' text and keep your mouse clicked. Note, It will close the popup unexpectedly. My fix prevents this race condition and any other.

@jzaefferer
Owner

I tend to killing popup. We don't use it in Menubar and that's probably not going to change, its very unlikely that we ever want to adopt it in dialog, we don't get feature request for menu to support and in the end we can just write some more custom popup code for the new datepicker.

I hope @sgharms gets around to look at menubar and popup together. Maybe he has some input.

@sgharms

@jzaefferer I'm finally all caught up on my PR's so now I can look at others' :).

I can confirm @rxaviers 's reported error on origin/menubar branch, but on the other hand I see no reason why we would want to use popup in menubar. I agree, it's a neat feature, but it doesn't make sense in the context of a menubar (as @jzaefferer mentioned; no users are asking for it).

If you were to kill it here, it wouldn't affect menubar, that I can see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 26, 2012
  1. @rxaviers

    Popup: Added _closeTry and _closeVet methods. Fixes closing the popup…

    rxaviers authored
    … unexpectedly by race conditions.
    
    - A focusout followed by a focusin sometimes trigger events in reverse order,
      but should not close the popup nevertheless. Or a click-in-the-popup followed
      by a focusout should also avoid closing it unexpectedly;
    - _closeTry and _closeVet work together to close the popup, but making sure no
      other event vets it;
  2. @rxaviers

    Popup: Binds document mousedown instead of click. The click event is …

    rxaviers authored
    …triggered when the mouse button is released, which virtually always exceeds _closeTry delay, leading to erroneous popup close.
  3. @rxaviers

    Popup: Manually binding document instead of using _bind. Because on d…

    rxaviers authored
    …estroy, it doesn't unbind other popup instances.
  4. @rxaviers

    Popup: Lowered the closing delay from 150ms to 50ms. Makes response f…

    rxaviers authored
    …aster since the _closeTry and _closeVet approach is working smooth.
This page is out of date. Refresh to see the latest.
Showing with 38 additions and 16 deletions.
  1. +38 −16 ui/jquery.ui.popup.js
View
54 ui/jquery.ui.popup.js
@@ -89,7 +89,7 @@ $.widget( "ui.popup", {
case $.ui.keyCode.UP:
// prevent scrolling
event.preventDefault();
- clearTimeout( this.closeTimer );
+ this._closeVet();
this._delay(function() {
this.open( event );
this.focusPopup( event );
@@ -114,7 +114,7 @@ $.widget( "ui.popup", {
return;
}
this.open( event );
- clearTimeout( this.closeTimer );
+ this._closeVet();
this._delay( function() {
if ( !noFocus ) {
this.focusPopup();
@@ -164,17 +164,10 @@ $.widget( "ui.popup", {
this._bind({
focusout: function( event ) {
- // use a timer to allow click to clear it and letting that
- // handle the closing instead of opening again
- this.closeTimer = this._delay( function() {
- this.close( event );
- }, 150);
+ this._closeTry( event );
},
focusin: function( event ) {
- clearTimeout( this.closeTimer );
- },
- mouseup: function( event ) {
- clearTimeout( this.closeTimer );
+ this._closeVet();
}
});
@@ -187,15 +180,43 @@ $.widget( "ui.popup", {
}
});
- this._bind( this.document, {
- click: function( event ) {
- if ( this.isOpen && !$( event.target ).closest( this.element.add( this.options.trigger ) ).length ) {
- this.close( event );
+ this.outsideHandler = $.proxy ( function ( event ) {
+ if ( this.isOpen ) {
+ if ( $( event.target ).closest( this.element.add( this.options.trigger ) ).length ) {
+ this._closeVet();
+ }
+ else {
+ this._closeTry( event );
}
}
- });
+ }, this );
+ $( this.document ).mousedown( this.outsideHandler );
},
+ // Unless vetted, close. Use a timer to allow veto in a timed window.
+ // Note that a focusout followed by a focusin sometimes trigger events in reverse order, but should not close the popup nevertheless. Or a click-in-the-popup followed by a focusout should also avoid closing it unexpectedly.
+ _closeTry: function( event ) {
+ if ( this.closeVeto || this.closeTimer ) {
+ return;
+ }
+ this.closeTimer = this._delay( function() {
+ this.close( event );
+ this.closeTimer = null;
+ }, 50 );
+ },
+
+ // Vets any close attempt in a timed window.
+ _closeVet: function() {
+ this.closeVeto = true;
+ this._delay( function() {
+ this.closeVeto = false;
+ }, 50 );
+ if ( this.closeTimer ) {
+ clearTimeout( this.closeTimer );
+ this.closeTimer = null;
+ }
+ },
+
_destroy: function() {
this.element
.show()
@@ -203,6 +224,7 @@ $.widget( "ui.popup", {
.removeAttr( "aria-hidden" )
.removeAttr( "aria-expanded" )
.unbind( "keypress.ui-popup");
+ $( this.document ).unbind( "mousedown", this.outsideHandler );
this.options.trigger
.removeAttr( "aria-haspopup" )
Something went wrong with that request. Please try again.