From 14dc4f329ad9a6347713fd864a0a996928bff2fb Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 25 Jul 2017 22:01:48 -0700 Subject: [PATCH 1/2] perf(jqLite): avoid repeated add/removeAttribute in jqLiteRemoveClass Fixes #16078 Closes #16131 --- src/jqLite.js | 12 +++++++----- test/jqLiteSpec.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/jqLite.js b/src/jqLite.js index ad59fbd1af2b..792029532250 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -420,13 +420,15 @@ function jqLiteHasClass(element, selector) { function jqLiteRemoveClass(element, cssClasses) { if (cssClasses && element.setAttribute) { + var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ') + .replace(/[\n\t]/g, ' '); + forEach(cssClasses.split(' '), function(cssClass) { - element.setAttribute('class', trim( - (' ' + (element.getAttribute('class') || '') + ' ') - .replace(/[\n\t]/g, ' ') - .replace(' ' + trim(cssClass) + ' ', ' ')) - ); + cssClass = trim(cssClass); + existingClasses = existingClasses.replace(' ' + cssClass + ' ', ' '); }); + + element.setAttribute('class', trim(existingClasses)); } } diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index 60c9981e2429..b627d061860f 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -914,6 +914,23 @@ describe('jqLite', function() { }); + // JQLite specific implementation/performance tests + if (_jqLiteMode) { + it('should only get/set the attribute once when multiple classes added', function() { + var fakeElement = { + nodeType: 1, + setAttribute: jasmine.createSpy(), + getAttribute: jasmine.createSpy().and.returnValue('') + }; + var jqA = jqLite(fakeElement); + + jqA.addClass('foo bar baz'); + expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); + expect(fakeElement.setAttribute).toHaveBeenCalledOnceWith('class', 'foo bar baz'); + }); + } + + it('should not add duplicate classes', function() { var jqA = jqLite(a); expect(a.className).toBe(''); @@ -1031,6 +1048,23 @@ describe('jqLite', function() { jqA.removeClass('foo baz noexistent'); expect(a.className).toBe('bar'); }); + + + // JQLite specific implementation/performance tests + if (_jqLiteMode) { + it('should get/set the attribute once when removing multiple classes', function() { + var fakeElement = { + nodeType: 1, + setAttribute: jasmine.createSpy(), + getAttribute: jasmine.createSpy().and.returnValue('foo bar baz') + }; + var jqA = jqLite(fakeElement); + + jqA.removeClass('foo baz noexistent'); + expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); + expect(fakeElement.setAttribute).toHaveBeenCalledOnceWith('class', 'bar'); + }); + } }); }); From 9764e8fd380287a039be470b0f95c4f96b040fac Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Wed, 2 Aug 2017 22:12:08 -0700 Subject: [PATCH 2/2] perf(jqLite): avoid setting class attribute when not changed --- src/jqLite.js | 16 +++++++++++----- test/jqLiteSpec.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/jqLite.js b/src/jqLite.js index 792029532250..f7913ac4312f 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -422,13 +422,16 @@ function jqLiteRemoveClass(element, cssClasses) { if (cssClasses && element.setAttribute) { var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ') .replace(/[\n\t]/g, ' '); + var newClasses = existingClasses; forEach(cssClasses.split(' '), function(cssClass) { cssClass = trim(cssClass); - existingClasses = existingClasses.replace(' ' + cssClass + ' ', ' '); + newClasses = newClasses.replace(' ' + cssClass + ' ', ' '); }); - element.setAttribute('class', trim(existingClasses)); + if (newClasses !== existingClasses) { + element.setAttribute('class', trim(newClasses)); + } } } @@ -436,15 +439,18 @@ function jqLiteAddClass(element, cssClasses) { if (cssClasses && element.setAttribute) { var existingClasses = (' ' + (element.getAttribute('class') || '') + ' ') .replace(/[\n\t]/g, ' '); + var newClasses = existingClasses; forEach(cssClasses.split(' '), function(cssClass) { cssClass = trim(cssClass); - if (existingClasses.indexOf(' ' + cssClass + ' ') === -1) { - existingClasses += cssClass + ' '; + if (newClasses.indexOf(' ' + cssClass + ' ') === -1) { + newClasses += cssClass + ' '; } }); - element.setAttribute('class', trim(existingClasses)); + if (newClasses !== existingClasses) { + element.setAttribute('class', trim(newClasses)); + } } } diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index b627d061860f..8ca865d31112 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -928,6 +928,20 @@ describe('jqLite', function() { expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); expect(fakeElement.setAttribute).toHaveBeenCalledOnceWith('class', 'foo bar baz'); }); + + + it('should not set the attribute when classes not changed', function() { + var fakeElement = { + nodeType: 1, + setAttribute: jasmine.createSpy(), + getAttribute: jasmine.createSpy().and.returnValue('foo bar') + }; + var jqA = jqLite(fakeElement); + + jqA.addClass('foo'); + expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); + expect(fakeElement.setAttribute).not.toHaveBeenCalled(); + }); } @@ -1064,6 +1078,20 @@ describe('jqLite', function() { expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); expect(fakeElement.setAttribute).toHaveBeenCalledOnceWith('class', 'bar'); }); + + + it('should not set the attribute when classes not changed', function() { + var fakeElement = { + nodeType: 1, + setAttribute: jasmine.createSpy(), + getAttribute: jasmine.createSpy().and.returnValue('foo bar') + }; + var jqA = jqLite(fakeElement); + + jqA.removeClass('noexistent'); + expect(fakeElement.getAttribute).toHaveBeenCalledOnceWith('class'); + expect(fakeElement.setAttribute).not.toHaveBeenCalled(); + }); } }); });