Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Selectmenu: Delay trigger until after the list closes #7197

Closed
wants to merge 3 commits into from

3 participants

@gabrielschulhof
Collaborator

Fixes gh-7076

@coveralls

Coverage Status

Coverage remained the same when pulling 4116f85 on 7076-delay-value-change-trigger into 00e80c9 on master.

@gabrielschulhof gabrielschulhof added this to the 1.4.3 milestone
tests/integration/select/select_core.js
((15 lines not shown))
+ });
+ return origTrigger.apply( this, arguments );
+ };
+
+ openEvent.event += ns + "1";
+ closeEvent.event += ns + "2";
+
+ $.testHelper.detailedEventCascade([
+ function() {
+ $( "#" + select.attr( "id" ) + "-button" ).click();
+ },
+ {
+ openevent: openEvent
+ },
+ function() {
+ $.event.trigger = replacementTrigger;
@arschmitz Owner

this should be done as part of setup and teardown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Coverage Status

Coverage increased (+0.12%) when pulling e758e34 on 7076-delay-value-change-trigger into 7b818d8 on master.

js/widgets/forms/select.custom.js
@@ -243,14 +247,16 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
.toggleClass( "ui-checkbox-off", !option.selected );
}
- // trigger change if value changed
- if ( self.isMultiple || oldIndex !== newIndex ) {
- self.select.trigger( "change" );
+ // if it's not a multiple select, trigger change after it has finished closing
@arschmitz Owner

Capitalize first word of comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
js/widgets/forms/select.custom.js
((10 lines not shown))
}
+ // trigger change if it's a multiple select
@arschmitz Owner

caps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
js/widgets/forms/select.custom.js
@@ -243,14 +247,16 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
.toggleClass( "ui-checkbox-off", !option.selected );
}
- // trigger change if value changed
- if ( self.isMultiple || oldIndex !== newIndex ) {
- self.select.trigger( "change" );
+ // if it's not a multiple select, trigger change after it has finished closing
+ if ( !self.isMultiple && oldIndex !== newIndex ) {
+ self._triggerChange = true;
@arschmitz Owner

should switch to use _on to remove need for self

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Coverage Status

Coverage increased (+0.35%) when pulling f2f89c3 on 7076-delay-value-change-trigger into 7b818d8 on master.

@coveralls

Coverage Status

Coverage increased (+0.11%) when pulling 9c2ea16 on 7076-delay-value-change-trigger into 44f503d on master.

@arschmitz
Owner

:+1:

@gabrielschulhof gabrielschulhof referenced this pull request from a commit
@gabrielschulhof gabrielschulhof Selectmenu: Delay trigger until after the list closes
(cherry picked from commit dde4873)

Closes gh-7197
Fixes gh-7076
ea0533f
@gabrielschulhof gabrielschulhof deleted the 7076-delay-value-change-trigger branch
@agcolom agcolom referenced this pull request from a commit in agcolom/jquery-mobile
@gabrielschulhof gabrielschulhof Selectmenu: Delay trigger until after the list closes
Closes gh-7197
Fixes gh-7076
436a2f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
101 js/widgets/forms/select.custom.js
@@ -110,6 +110,10 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
},
_handleMenuPageHide: function() {
+
+ // After the dialog's done, we may want to trigger change if the value has actually changed
+ this._delayedTrigger();
+
// TODO centralize page removal binding / handling in the page plugin.
// Suggestion from @jblas to do refcounting
//
@@ -132,18 +136,55 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
}
},
+ _handleListItemClick: function( event ) {
+ var listItem = $( event.target ).closest( "li" ),
+
+ // Index of option tag to be selected
+ oldIndex = this.select[ 0 ].selectedIndex,
+ newIndex = $.mobile.getAttribute( listItem, "option-index" ),
+ option = this._selectOptions().eq( newIndex )[ 0 ];
+
+ // Toggle selected status on the tag for multi selects
+ option.selected = this.isMultiple ? !option.selected : true;
+
+ // Toggle checkbox class for multiple selects
+ if ( this.isMultiple ) {
+ listItem.find( "a" )
+ .toggleClass( "ui-checkbox-on", option.selected )
+ .toggleClass( "ui-checkbox-off", !option.selected );
+ }
+
+ // If it's not a multiple select, trigger change after it has finished closing
+ if ( !this.isMultiple && oldIndex !== newIndex ) {
+ this._triggerChange = true;
+ }
+
+ // Trigger change if it's a multiple select
+ // Hide custom select for single selects only - otherwise focus clicked item
+ // We need to grab the clicked item the hard way, because the list may have been rebuilt
+ if ( this.isMultiple ) {
+ this.select.trigger( "change" );
+ this.list.find( "li:not(.ui-li-divider)" ).eq( newIndex )
+ .find( "a" ).first().focus();
+ }
+ else {
+ this.close();
+ }
+
+ event.preventDefault();
+ },
+
build: function() {
var selectId, popupId, dialogId, label, thisPage, isMultiple, menuId,
themeAttr, overlayTheme, overlayThemeAttr, dividerThemeAttr,
menuPage, listbox, list, header, headerTitle, menuPageContent,
- menuPageClose, headerClose, self,
+ menuPageClose, headerClose,
o = this.options;
if ( o.nativeMenu ) {
return this._super();
}
- self = this;
selectId = this.selectId;
popupId = selectId + "-listbox";
dialogId = selectId + "-dialog";
@@ -221,52 +262,18 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
// Events for list items
this.list.attr( "role", "listbox" );
this._on( this.list, {
- focusin : "_handleListFocus",
- focusout : "_handleListFocus",
- keydown: "_handleListKeydown"
+ "focusin": "_handleListFocus",
+ "focusout": "_handleListFocus",
+ "keydown": "_handleListKeydown",
+ "click li:not(.ui-disabled,.ui-state-disabled,.ui-li-divider)": "_handleListItemClick"
});
- this.list
- .delegate( "li:not(.ui-disabled,.ui-state-disabled,.ui-li-divider)", "click", function( event ) {
-
- // index of option tag to be selected
- var oldIndex = self.select[ 0 ].selectedIndex,
- newIndex = $.mobile.getAttribute( this, "option-index" ),
- option = self._selectOptions().eq( newIndex )[ 0 ];
-
- // toggle selected status on the tag for multi selects
- option.selected = self.isMultiple ? !option.selected : true;
-
- // toggle checkbox class for multiple selects
- if ( self.isMultiple ) {
- $( this ).find( "a" )
- .toggleClass( "ui-checkbox-on", option.selected )
- .toggleClass( "ui-checkbox-off", !option.selected );
- }
-
- // trigger change if value changed
- if ( self.isMultiple || oldIndex !== newIndex ) {
- self.select.trigger( "change" );
- }
-
- // hide custom select for single selects only - otherwise focus clicked item
- // We need to grab the clicked item the hard way, because the list may have been rebuilt
- if ( self.isMultiple ) {
- self.list.find( "li:not(.ui-li-divider)" ).eq( newIndex )
- .find( "a" ).first().focus();
- }
- else {
- self.close();
- }
-
- event.preventDefault();
- });
// button refocus ensures proper height calculation
// by removing the inline style and ensuring page inclusion
this._on( this.menuPage, { pagehide: "_handleMenuPageHide" } );
// Events on the popup
- this._on( this.listbox, { popupafterclose: "close" } );
+ this._on( this.listbox, { popupafterclose: "_popupClosed" } );
// Close button on small overlays
if ( this.isMultiple ) {
@@ -276,6 +283,18 @@ $.widget( "mobile.selectmenu", $.mobile.selectmenu, {
return this;
},
+ _popupClosed: function() {
+ this.close();
+ this._delayedTrigger();
+ },
+
+ _delayedTrigger: function() {
+ if ( this._triggerChange ) {
+ this.element.trigger( "change" );
+ }
+ this._triggerChange = false;
+ },
+
_isRebuildRequired: function() {
var list = this.list.find( "li" ),
options = this._selectOptions().not( ".ui-screen-hidden" );
View
58 tests/integration/select/index.html
@@ -36,6 +36,64 @@
<div id="qunit"></div>
<div id="default" data-nstest-role="page" data-nstest-theme="c">
+ <select name="small-select-change-after-close" id="small-select-change-after-close" data-nstest-native-menu="false">
+ <option value="1">One</option>
+ <option value="2">Two</option>
+ <option value="3">Three</option>
+ </select>
+ <select name="large-select-change-after-close" id="large-select-change-after-close" data-nstest-native-menu="false">
+ <option value="AL">Alabama</option>
+ <option value="AK">Alaska</option>
+ <option value="AZ">Arizona</option>
+ <option value="AR">Arkansas</option>
+ <option value="CA">California</option>
+ <option value="CO">Colorado</option>
+ <option value="CT">Connecticut</option>
+ <option value="DE">Delaware</option>
+ <option value="FL">Florida</option>
+ <option value="GA">Georgia</option>
+ <option value="HI">Hawaii</option>
+ <option value="ID">Idaho</option>
+ <option value="IL">Illinois</option>
+ <option value="IN">Indiana</option>
+ <option value="IA">Iowa</option>
+ <option value="KS">Kansas</option>
+ <option value="KY">Kentucky</option>
+ <option value="LA">Louisiana</option>
+ <option value="ME">Maine</option>
+ <option value="MD">Maryland</option>
+ <option value="MA">Massachusetts</option>
+ <option value="MI">Michigan</option>
+ <option value="MN">Minnesota</option>
+ <option value="MS">Mississippi</option>
+ <option value="MO">Missouri</option>
+ <option value="MT">Montana</option>
+ <option value="NE">Nebraska</option>
+ <option value="NV">Nevada</option>
+ <option value="NH">New Hampshire</option>
+ <option value="NJ">New Jersey</option>
+ <option value="NM">New Mexico</option>
+ <option value="NY">New York</option>
+ <option value="NC">North Carolina</option>
+ <option value="ND">North Dakota</option>
+ <option value="OH">Ohio</option>
+ <option value="OK">Oklahoma</option>
+ <option value="OR">Oregon</option>
+ <option value="PA">Pennsylvania</option>
+ <option value="RI">Rhode Island</option>
+ <option value="SC">South Carolina</option>
+ <option value="SD">South Dakota</option>
+ <option value="TN">Tennessee</option>
+ <option value="TX">Texas</option>
+ <option value="UT">Utah</option>
+ <option value="VT">Vermont</option>
+ <option value="VA">Virginia</option>
+ <option value="WA">Washington</option>
+ <option value="WV">West Virginia</option>
+ <option value="WI">Wisconsin</option>
+ <option value="WY">Wyoming</option>
+ </select>
+
<div data-nstest-role="fieldcontain" id="select-choice-few-container">
<select name="select-choice-few" id="select-choice-few.dotTest" data-nstest-native-menu="false">
<option value="standard">Standard: 7 day</option>
View
96 tests/integration/select/select_core.js
@@ -536,4 +536,100 @@
]);
});
+ // Utensils for logging calls to $.event.trigger()
+ var callLog, origTrigger,
+ replacementTrigger = function( event, data, element, onlyHandlers ) {
+ callLog.push({
+ event: event,
+ data: data,
+ element: element,
+ onlyHandlers: onlyHandlers
+ });
+ return origTrigger.apply( this, arguments );
+ };
+
+ module( "Custom select change comes after closing list", {
+ setup: function() {
+ callLog = [];
+ origTrigger = $.event.trigger;
+ $.event.trigger = replacementTrigger;
+ },
+ teardown: function() {
+ $.event.trigger = origTrigger;
+ }
+ });
+
+ function testChangeAfterClose( select, ns, openEvent, closeEvent, tail ) {
+ var closeComesBeforeChange = false,
+ closeEventName = closeEvent.event;
+
+ openEvent.event += ns + "1";
+ closeEvent.event += ns + "2";
+
+ $.testHelper.detailedEventCascade([
+ function() {
+ $( "#" + select.attr( "id" ) + "-button" ).click();
+ },
+ {
+ openevent: openEvent
+ },
+ function() {
+ $( "#" + select.attr( "id" ) + "-menu" ).find( "a" ).eq( 2 ).click();
+ },
+ {
+ closeevent: closeEvent,
+ change: { src: select, event: "change" + ns + "2" }
+ },
+ function() {
+ $.each( callLog, function( index, value ) {
+ var name = ( typeof callLog[ index ].event === "string" ?
+ callLog[ index ].event :
+ callLog[ index ].event.type ),
+ target = callLog[ index ].element;
+
+ if ( name === "change" && target === select[ 0 ] ) {
+ return false;
+ }
+
+ if ( name === closeEventName &&
+ target === ( typeof closeEvent.src === "function" ?
+ closeEvent.src()[ 0 ] :
+ closeEvent.src[ 0 ] ) ) {
+
+ closeComesBeforeChange = true;
+ return false;
+ }
+ });
+
+ deepEqual( closeComesBeforeChange, true,
+ "close event is triggered before change event" );
+ tail();
+ }
+ ]);
+ }
+
+ asyncTest( "Small select triggers change after popup closes", function() {
+ testChangeAfterClose( $( "#small-select-change-after-close" ),
+ ".smallSelectTriggersChangeAfterPopupCloses",
+ {
+ src: $( "#small-select-change-after-close-listbox" ),
+ event: "popupafteropen"
+ },
+ {
+ src: $( "#small-select-change-after-close-listbox" ),
+ event: "popupafterclose"
+ }, start);
+ });
+
+ asyncTest( "Large select triggers change after dialog closes", function() {
+ testChangeAfterClose( $( "#large-select-change-after-close" ),
+ ".largeSelectTriggersChangeAfterPopupCloses",
+ { src: $( document ), event: "pageshow" },
+ {
+ src: function() { return $( "#large-select-change-after-close-dialog" ); },
+ event: "pagehide"
+ },
+ start);
+ });
+
})(jQuery);
Something went wrong with that request. Please try again.