Skip to content

Widget: Increase performance for large sortable/draggable collections #2313

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −29
Draft

Widget: Increase performance for large sortable/draggable collections #2313

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions eslint.config.mjs
View file
Original file line number Diff line number Diff line change
@@ -62,6 +62,7 @@ export default [
ecmaVersion: 5,
sourceType: "script",
globals: {
...globals.es2015,
...globals.browser,
...globals.jquery,
define: false,
@@ -142,6 +143,7 @@ export default [
ecmaVersion: 5,
sourceType: "script",
globals: {
...globals.es2015,
...globals.browser,
...globals.jquery,
define: false,
18 changes: 9 additions & 9 deletions tests/unit/widget/classes.js
View file
Original file line number Diff line number Diff line change
@@ -155,27 +155,27 @@ QUnit.test( "Classes elements are untracked as they are removed from the DOM", f
var widget = $( "#widget" ).classesWidget();
var instance = widget.classesWidget( "instance" );

assert.equal( instance.classesElementLookup[ "ui-classes-span" ].length, 3,
assert.equal( instance.classesElementLookup[ "ui-classes-span" ].size, 3,
"Widget is tracking 3 ui-classes-span elements" );
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].length, 3,
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].size, 3,
"Widget is tracking 3 ui-core-span-null elements" );
assert.equal( instance.classesElementLookup[ "ui-core-span" ].length, 3,
assert.equal( instance.classesElementLookup[ "ui-core-span" ].size, 3,
"Widget is tracking 3 ui-core-span elements" );

widget.find( "span" ).eq( 0 ).remove();
assert.equal( instance.classesElementLookup[ "ui-classes-span" ].length, 2,
assert.equal( instance.classesElementLookup[ "ui-classes-span" ].size, 2,
"After removing 1 span from dom 2 ui-classes-span elements are tracked" );
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].length, 2,
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].size, 2,
"After removing 1 span from dom 2 ui-core-span-null elements are tracked" );
assert.equal( instance.classesElementLookup[ "ui-core-span" ].length, 2,
assert.equal( instance.classesElementLookup[ "ui-core-span" ].size, 2,
"After removing 1 span from dom 2 ui-core-span elements are tracked" );

widget.find( "span" ).remove();
assert.equal( instance.classesElementLookup[ "ui-classes-span" ].length, 0,
assert.equal( instance.classesElementLookup[ "ui-classes-span" ].size, 0,
"No ui-classes-span elements are tracked after removing all spans" );
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].length, 0,
assert.equal( instance.classesElementLookup[ "ui-core-span-null" ].size, 0,
"No ui-core-span-null elements are tracked after removing all spans" );
assert.equal( instance.classesElementLookup[ "ui-core-span" ].length, 0,
assert.equal( instance.classesElementLookup[ "ui-core-span" ].size, 0,
"No ui-core-span elements are tracked after removing all spans" );
} );

56 changes: 36 additions & 20 deletions ui/widget.js
View file
Original file line number Diff line number Diff line change
@@ -301,10 +301,10 @@ $.Widget.prototype = {
this.uuid = widgetUuid++;
this.eventNamespace = "." + this.widgetName + this.uuid;

this.bindings = $();
this.bindings = new Set();
this.hoverable = $();
this.focusable = $();
this.classesElementLookup = {};
this.classesElementLookup = Object.create( null );

if ( element !== this ) {
$.data( element, this.widgetFullName, this );
@@ -355,7 +355,7 @@ $.Widget.prototype = {

this._destroy();
$.each( this.classesElementLookup, function( key, value ) {
that._removeClass( value, key );
that._removeClass( $( Array.from( value ) ), key );
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not that worried about this since it's in destroy. That shouldn't happen too often.

} );

// We can probably remove the unbind calls in 2.0
@@ -368,7 +368,7 @@ $.Widget.prototype = {
.removeAttr( "aria-disabled" );

// Clean up events and states
this.bindings.off( this.eventNamespace );
$( Array.from( this.bindings ) ).off( this.eventNamespace );
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not that worried about this since it's in destroy. That shouldn't happen too often.

},

_destroy: $.noop,
@@ -450,16 +450,12 @@ $.Widget.prototype = {
currentElements = this.classesElementLookup[ classKey ];
if ( value[ classKey ] === this.options.classes[ classKey ] ||
!currentElements ||
!currentElements.length ) {
!currentElements.size ) {
continue;
}

// We are doing this to create a new jQuery object because the _removeClass() call
// on the next line is going to destroy the reference to the current elements being
// tracked. We need to save a copy of this collection so that we can add the new classes
// below.
elements = $( currentElements.get() );
this._removeClass( currentElements, classKey );
elements = $( Array.from( currentElements ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

We re-wrap here, but the previous code was already creating a new jQuery object, so it shouldn't be worse than before.

Also, I don't understand the comment above; how would we lose the reference here? It's already in the currentElement object, that wouldn't just disappear...

this._removeClass( elements, classKey );

// We don't use _addClass() here, because that uses this.options.classes
// for generating the string of classes. We want to use the value passed in from
@@ -509,7 +505,7 @@ $.Widget.prototype = {
return elements;
} )
.some( function( elements ) {
return elements.is( element );
return elements.has( element );
} );

if ( !isTracked ) {
@@ -525,12 +521,24 @@ $.Widget.prototype = {
function processClassString( classes, checkOption ) {
var current, i;
for ( i = 0; i < classes.length; i++ ) {
current = that.classesElementLookup[ classes[ i ] ] || $();
current = that.classesElementLookup[ classes[ i ] ] || new Set();
if ( options.add ) {
bindRemoveEvent();
current = $( $.uniqueSort( current.get().concat( options.element.get() ) ) );

// This function is invoked synchronously, so the reference
// to `current` is not an issue.
// eslint-disable-next-line no-loop-func
$.each( options.element, function( _i, node ) {
current.add( node );
} );
} else {
current = $( current.not( options.element ).get() );

// This function is invoked synchronously, so the reference
// to `current` is not an issue.
// eslint-disable-next-line no-loop-func
$.each( options.element, function( _i, node ) {
current.delete( node );
} );
}
that.classesElementLookup[ classes[ i ] ] = current;
full.push( classes[ i ] );
@@ -553,9 +561,7 @@ $.Widget.prototype = {
_untrackClassesElement: function( event ) {
var that = this;
$.each( that.classesElementLookup, function( key, value ) {
if ( $.inArray( event.target, value ) !== -1 ) {
that.classesElementLookup[ key ] = $( value.not( event.target ).get() );
}
value.delete( event.target );
} );

this._off( $( event.target ) );
@@ -583,6 +589,7 @@ $.Widget.prototype = {
},

_on: function( suppressDisabledCheck, element, handlers ) {
var i;
var delegateElement;
var instance = this;

@@ -600,7 +607,11 @@ $.Widget.prototype = {
delegateElement = this.widget();
} else {
element = delegateElement = $( element );
this.bindings = this.bindings.add( element );
for ( i = 0; i < element.length; i++ ) {
$.each( element, function( _i, node ) {
instance.bindings.add( node );
} );
}
}

$.each( handlers, function( event, handler ) {
@@ -637,12 +648,17 @@ $.Widget.prototype = {
},

_off: function( element, eventName ) {
var that = this;

eventName = ( eventName || "" ).split( " " ).join( this.eventNamespace + " " ) +
this.eventNamespace;
element.off( eventName );

$.each( element, function( _i, node ) {
that.bindings.delete( node );
} );

// Clear the stack to avoid memory leaks (#10056)
this.bindings = $( this.bindings.not( element ).get() );
this.focusable = $( this.focusable.not( element ).get() );
this.hoverable = $( this.hoverable.not( element ).get() );
},