From 9a81cd414a2c534b96de0d82c3d00d94651e5a7b Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Fri, 15 Nov 2019 04:28:40 -0400 Subject: [PATCH] fix(b-table, b-table-lite): handle edge case with row events when table is removed from dom. instantiate row event handlers only when listeners are registered (fixes #4384) (#4388) * fix(b-table, b-table-lite): handle edge case with row events when table is removed from dom (fixes #4384) * Update mixin-tbody.js * Update mixin-tbody-row.js * Update table-tbody-row-events.spec.js * Update table-tbody-row-events.spec.js * Update mixin-tbody-row.js * only emit events if listeners are registered * Update table-tbody-row-events.spec.js * Update mixin-tbody-row.js * Update mixin-tbody.js * Update mixin-tbody.js * Update mixin-tbody-row.js * Update mixin-tbody.js --- .../table/helpers/mixin-tbody-row.js | 10 ++- src/components/table/helpers/mixin-tbody.js | 43 ++++++----- .../table/table-tbody-row-events.spec.js | 74 +++++++++++++++++++ 3 files changed, 104 insertions(+), 23 deletions(-) diff --git a/src/components/table/helpers/mixin-tbody-row.js b/src/components/table/helpers/mixin-tbody-row.js index 2a7c77b96f4..5bc4537c9d7 100644 --- a/src/components/table/helpers/mixin-tbody-row.js +++ b/src/components/table/helpers/mixin-tbody-row.js @@ -69,17 +69,19 @@ export default { rowHovered(evt) { // `mouseenter` handler (non-bubbling) // `this.tbodyRowEvtStopped` from tbody mixin - if (!this.tbodyRowEvtStopped(evt)) { + const type = 'row-hovered' + if (this.$listeners[type] && !this.tbodyRowEvtStopped(evt)) { // `this.emitTbodyRowEvent` from tbody mixin - this.emitTbodyRowEvent('row-hovered', evt) + this.emitTbodyRowEvent(type, evt) } }, rowUnhovered(evt) { // `mouseleave` handler (non-bubbling) // `this.tbodyRowEvtStopped` from tbody mixin - if (!this.tbodyRowEvtStopped(evt)) { + const type = 'row-unhovered' + if (this.$listeners[type] && !this.tbodyRowEvtStopped(evt)) { // `this.emitTbodyRowEvent` from tbody mixin - this.emitTbodyRowEvent('row-unhovered', evt) + this.emitTbodyRowEvent(type, evt) } }, // Render helpers diff --git a/src/components/table/helpers/mixin-tbody.js b/src/components/table/helpers/mixin-tbody.js index ae27c17dc61..278334255c7 100644 --- a/src/components/table/helpers/mixin-tbody.js +++ b/src/components/table/helpers/mixin-tbody.js @@ -24,13 +24,12 @@ export default { // `this.$refs.itemRows` is an array of item TR components/elements // Rows should all be B-TR components, but we map to TR elements // Also note that `this.$refs.itemRows` may not always be in document order - const tbody = this.$refs.tbody.$el || this.$refs.tbody - const trs = (this.$refs.itemRows || []).map(tr => tr.$el || tr) - // TODO: This may take time for tables many rows, so we may want to cache - // the result of this during each render cycle on a non-reactive - // property. We clear out the cache as each render starts, and - // populate it on first access of this method if null - return arrayFrom(tbody.children).filter(tr => arrayIncludes(trs, tr)) + const refs = this.$refs || {} + const tbody = refs.tbody ? refs.tbody.$el || refs.tbody : null + const trs = (refs.itemRows || []).map(tr => tr.$el || tr) + return tbody && tbody.children && tbody.children.length > 0 && trs && trs.length > 0 + ? arrayFrom(tbody.children).filter(tr => arrayIncludes(trs, tr)) + : [] }, getTbodyTrIndex(el) { // Returns index of a particular TBODY item TR @@ -102,6 +101,7 @@ export default { } }, onTBodyRowClicked(evt) { + // Row-clicked handler is only added when needed if (this.tbodyRowEvtStopped(evt)) { // If table is busy, then don't propagate return @@ -113,18 +113,21 @@ export default { this.emitTbodyRowEvent('row-clicked', evt) }, onTbodyRowMiddleMouseRowClicked(evt) { - if (!this.tbodyRowEvtStopped(evt) && evt.which === 2) { - this.emitTbodyRowEvent('row-middle-clicked', evt) + const type = 'row-middle-clicked' + if (this.$listeners[type] && !this.tbodyRowEvtStopped(evt) && evt.which === 2) { + this.emitTbodyRowEvent(type, evt) } }, onTbodyRowContextmenu(evt) { - if (!this.tbodyRowEvtStopped(evt)) { - this.emitTbodyRowEvent('row-contextmenu', evt) + const type = 'row-contextmenu' + if (this.$listeners[type] && !this.tbodyRowEvtStopped(evt)) { + this.emitTbodyRowEvent(type, evt) } }, onTbodyRowDblClicked(evt) { - if (!this.tbodyRowEvtStopped(evt) && !filterEvent(evt)) { - this.emitTbodyRowEvent('row-dblclicked', evt) + const type = 'row-dblclicked' + if (this.$listeners[type] && !this.tbodyRowEvtStopped(evt) && !filterEvent(evt)) { + this.emitTbodyRowEvent(type, evt) } }, // Note: Row hover handlers are handled by the tbody-row mixin @@ -187,22 +190,24 @@ export default { $rows.push(this.renderBottomRow ? this.renderBottomRow() : h()) } + // Note: these events will only emit if a listener is registered const handlers = { - // TODO: We may want to to only instantiate these handlers - // if there is an event listener registered auxclick: this.onTbodyRowMiddleMouseRowClicked, - // TODO: Perhaps we do want to automatically prevent the - // default context menu from showing if there is - // a `row-contextmenu` listener registered. + // TODO: + // Perhaps we do want to automatically prevent the + // default context menu from showing if there is a + // `row-contextmenu` listener registered contextmenu: this.onTbodyRowContextmenu, // The following event(s) is not considered A11Y friendly dblclick: this.onTbodyRowDblClicked - // hover events (mouseenter/mouseleave) ad handled by tbody-row mixin + // Hover events (`mouseenter`/`mouseleave`) are handled by `tbody-row` mixin } + // Add in click/keydown listeners if needed if (hasRowClickHandler) { handlers.click = this.onTBodyRowClicked handlers.keydown = this.onTbodyRowKeydown } + // Assemble rows into the tbody const $tbody = h( BTbody, diff --git a/src/components/table/table-tbody-row-events.spec.js b/src/components/table/table-tbody-row-events.spec.js index 75627793a37..a2e62e6038f 100644 --- a/src/components/table/table-tbody-row-events.spec.js +++ b/src/components/table/table-tbody-row-events.spec.js @@ -82,6 +82,10 @@ describe('table > tbody row events', () => { propsData: { fields: testFields, items: testItems + }, + listeners: { + // Row-dblclicked will only occur if there is a registered listener + 'row-dblclicked': () => {} } }) expect(wrapper).toBeDefined() @@ -104,6 +108,10 @@ describe('table > tbody row events', () => { fields: testFields, items: testItems, busy: true + }, + listeners: { + // Row-dblclicked will only occur if there is a registered listener + 'row-dblclicked': () => {} } }) expect(wrapper).toBeDefined() @@ -121,6 +129,10 @@ describe('table > tbody row events', () => { propsData: { fields: testFields, items: testItems + }, + listeners: { + // Row-middle-clicked will only occur if there is a registered listener + 'row-middle-clicked': () => {} } }) expect(wrapper).toBeDefined() @@ -143,6 +155,10 @@ describe('table > tbody row events', () => { fields: testFields, items: testItems, busy: true + }, + listeners: { + // Row-middle-clicked will only occur if there is a registered listener + 'row-middle-clicked': () => {} } }) expect(wrapper).toBeDefined() @@ -160,6 +176,10 @@ describe('table > tbody row events', () => { propsData: { fields: testFields, items: testItems + }, + listeners: { + // Row-contextmenu will only occur if there is a registered listener + 'row-contextmenu': () => {} } }) expect(wrapper).toBeDefined() @@ -182,6 +202,10 @@ describe('table > tbody row events', () => { fields: testFields, items: testItems, busy: true + }, + listeners: { + // Row-contextmenu will only occur if there is a registered listener + 'row-contextmenu': () => {} } }) expect(wrapper).toBeDefined() @@ -199,6 +223,10 @@ describe('table > tbody row events', () => { propsData: { fields: testFields, items: testItems + }, + listeners: { + // Row-hovered will only occur if there is a registered listener + 'row-hovered': () => {} } }) expect(wrapper).toBeDefined() @@ -215,12 +243,33 @@ describe('table > tbody row events', () => { wrapper.destroy() }) + it('should not emit row-hovered event when a row is hovered and no listener', async () => { + const wrapper = mount(BTable, { + propsData: { + fields: testFields, + items: testItems + } + }) + expect(wrapper).toBeDefined() + const $rows = wrapper.findAll('tbody > tr') + expect($rows.length).toBe(3) + expect(wrapper.emitted('row-hovered')).not.toBeDefined() + $rows.at(1).trigger('mouseenter') + expect(wrapper.emitted('row-hovered')).not.toBeDefined() + + wrapper.destroy() + }) + it('should not emit row-hovered event when a row is hovered and table busy', async () => { const wrapper = mount(BTable, { propsData: { fields: testFields, items: testItems, busy: true + }, + listeners: { + // Row-hovered will only occur if there is a registered listener + 'row-hovered': () => {} } }) expect(wrapper).toBeDefined() @@ -238,6 +287,10 @@ describe('table > tbody row events', () => { propsData: { fields: testFields, items: testItems + }, + listeners: { + // Row-unhovered will only occur if there is a registered listener + 'row-unhovered': () => {} } }) expect(wrapper).toBeDefined() @@ -254,12 +307,33 @@ describe('table > tbody row events', () => { wrapper.destroy() }) + it('should not emit row-nhovered event when a row is hovered and no listener', async () => { + const wrapper = mount(BTable, { + propsData: { + fields: testFields, + items: testItems + } + }) + expect(wrapper).toBeDefined() + const $rows = wrapper.findAll('tbody > tr') + expect($rows.length).toBe(3) + expect(wrapper.emitted('row-unhovered')).not.toBeDefined() + $rows.at(1).trigger('mouseleave') + expect(wrapper.emitted('row-unhovered')).not.toBeDefined() + + wrapper.destroy() + }) + it('should not emit row-unhovered event when a row is unhovered and table busy', async () => { const wrapper = mount(BTable, { propsData: { fields: testFields, items: testItems, busy: true + }, + listeners: { + // Row-unhovered will only occur if there is a registered listener + 'row-unhovered': () => {} } }) expect(wrapper).toBeDefined()