From aec491574dd85c549b5f8fa297215f66d69f4edf Mon Sep 17 00:00:00 2001
From: yan <yan@mit.edu>
Date: Mon, 18 Sep 2017 23:10:35 +0000
Subject: [PATCH] fix table multi-select after sort

fix https://github.com/brave/browser-laptop/issues/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
---
 .../components/common/sortableTable.js        | 34 +++++++++++++++----
 test/about/historyTest.js                     | 18 ++++++++++
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/app/renderer/components/common/sortableTable.js b/app/renderer/components/common/sortableTable.js
index 41e0f946462..97673747c38 100644
--- a/app/renderer/components/common/sortableTable.js
+++ b/app/renderer/components/common/sortableTable.js
@@ -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
@@ -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
@@ -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)))
         }
       }
     })
@@ -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)
     }
diff --git a/test/about/historyTest.js b/test/about/historyTest.js
index 9670ba96212..8a91be6f545 100644
--- a/test/about/historyTest.js
+++ b/test/about/historyTest.js
@@ -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')
@@ -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)