Skip to content

Commit

Permalink
fix table multi-select after sort
Browse files Browse the repository at this point in the history
fix brave#10971

test plan:
1. browse some sites, then go to about:history
2. click on any of the columns to sort the table
3. shift+click to select multiple entries
4. contiguous table entries should be selected, not random entries
  • Loading branch information
diracdeltas authored and syuan100 committed Nov 9, 2017
1 parent 1234b54 commit aec4915
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 6 deletions.
34 changes: 28 additions & 6 deletions app/renderer/components/common/sortableTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ class SortableTable extends React.Component {
this.sortTable = tableSort(this.table)
return this.sortTable
}

componentDidUpdate () {
this.sortTable.refresh()
componentDidUpdate (prevProps) {
if (this.props.rows &&
(!prevProps.rows ||
prevProps.rows.length !== this.props.rows.length)) {
this.sortTable.refresh()
}
}
/**
* If you want multi-select to span multiple tables, you can
Expand Down Expand Up @@ -92,15 +95,34 @@ class SortableTable extends React.Component {
: this.stateOwner.state.selection.add(globalIndex)
})
}
/**
* Converts a data-row-index property to an HTML element's rowIndex within the
* table. rowIndex changes when the table is sorted.
* @param {number} index
*/
rowIndexToHTMLRowIndex (index) {
const element = this.table.querySelector(`tr[data-row-index="${index}"]`)
return element.rowIndex
}
/**
* Converts an HTML element's rowIndex within the
* table to its data-row-index property.
* @param {number} index
*/
htmlRowIndexToRowIndex (index) {
const element = this.table.rows[index]
return parseInt(element.getAttribute('data-row-index'))
}
/**
* [Shift + click] can only multi-select within the same table.
* @param {number} index - The rowIndex attribute of the row element
*/
processShiftClick (index) {
let newSelection = Immutable.Set()
this.stateOwner.state.selection.forEach((globalIndex) => {
const tableParts = globalIndex.split('-')
const tableID = parseInt(tableParts[0])
const rowIndex = parseInt(tableParts[1])
const rowIndex = this.rowIndexToHTMLRowIndex(parseInt(tableParts[1]))
if (tableID === this.tableID) {
let startIndex
let endIndex
Expand All @@ -114,7 +136,7 @@ class SortableTable extends React.Component {
return
}
for (let i = startIndex; i <= endIndex; ++i) {
newSelection = newSelection.add(this.getGlobalIndex(i))
newSelection = newSelection.add(this.getGlobalIndex(this.htmlRowIndexToRowIndex(i)))
}
}
})
Expand Down Expand Up @@ -168,7 +190,7 @@ class SortableTable extends React.Component {
if (eventUtil.isForSecondaryAction(e)) {
this.processControlClick(clickedIndex)
} else if (e.shiftKey) {
this.processShiftClick(clickedIndex)
this.processShiftClick(targetElement.rowIndex)
} else {
this.processClick(clickedIndex)
}
Expand Down
18 changes: 18 additions & 0 deletions test/about/historyTest.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* global describe, it, before */

const Brave = require('../lib/brave')
const assert = require('assert')
const {urlInput} = require('../lib/selectors')
const {getTargetAboutUrl} = require('../../js/lib/appUrlUtil')
const aboutHistoryUrl = getTargetAboutUrl('about:history')
Expand Down Expand Up @@ -173,6 +174,23 @@ describe('about:history', function () {
.keys(Brave.keys.SHIFT)
.click('table.sortableTable td.title[data-sort="Brave"]')
})
it('selects multiple contiguous rows when shift clicked after sorting', function * () {
yield this.app.client
.tabByUrl(aboutHistoryUrl)
.loadUrl(aboutHistoryUrl)
.waitForVisible('.heading-title')
.click('.heading-title')
// wait for sort
.pause(200)
.click('table.sortableTable td.title[data-sort="Brave"]')
.keys(Brave.keys.SHIFT)
.click('table.sortableTable td.title[data-sort="https://www.facebook.com"]')
.keys(Brave.keys.SHIFT)
.waitForVisible('table.sortableTable tr.selected td.title[data-sort="Brave"]')
.waitForVisible('table.sortableTable tr.selected td.title[data-sort="https://brave.com/test"]')
.waitForVisible('table.sortableTable tr.selected td.title[data-sort="https://www.facebook.com"]')
.isExisting('table.sortableTable tr.selected td.title[data-sort="https://www.youtube.com"]', (isExisting) => assert(!isExisting))
})
it('deselects everything if something other than the table is clicked', function * () {
yield this.app.client
.tabByUrl(aboutHistoryUrl)
Expand Down

0 comments on commit aec4915

Please sign in to comment.