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.

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

Jump to bottom

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
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
18 changes: 9 additions & 9 deletions tests/unit/widget/classes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" );
} );

Expand Down
1 change: 1 addition & 0 deletions ui/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
},

"env": {
"es6": true,
"browser": true,
"jquery": true,
"node": false
Expand Down
56 changes: 36 additions & 20 deletions ui/widget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -509,7 +505,7 @@ $.Widget.prototype = {
return elements;
} )
.some( function( elements ) {
return elements.is( element );
return elements.has( element );
} );

if ( !isTracked ) {
Expand All @@ -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 ] );
Expand All @@ -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 ) );
Expand Down Expand Up @@ -583,6 +589,7 @@ $.Widget.prototype = {
},

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

Expand All @@ -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 ) {
Expand Down Expand Up @@ -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() );
},
Expand Down
Loading