table: fix moving columns bug

In the previous tutorial, we make columns swappable. I mentioned that in some cases two columns can swap really fast and flash. There is a flaw in our code. In this tutorial, we are going to fix it.

What

First of all, when we spot a bug, we need to identify what the bug is and try limiting the factors.

This one is working as expected.

This one flashes.

So what's the bug? When we move "website" column to the right, we expect it to swap with "company" column. The "website" column does swap with "company" column, but they somehow swap again and again when we keep moving towards right.

Let's try another column. This works as expected. When moving "company" column to left or right, "company" column only swap once with the "address" column .

It's not obvious where the bug occurs, but we know that "website" column has bug and "company" column does not. We can inspect on them further more. We move our cursor much slower this time. We can see that sometimes the columns swap correctly, but as soon as we move our cursor again. Even we are still moving "website" towards right, "website" column swaps back with its left column "company", which is not correct.

So the bug occurs when we move a column to right, it should only swap with its right column, but it sometimes swaps with its left column. When we constantly moving our cursor, the columns swap repeatedly and fast. That's why they flash.

We have identified the bug. Next, we want to collect as much information as possible. Does this bug also occurs when we move a column to left? Let's test it out.

Moving "company" column to left works fine.

Moving "website" column to left has bugs.

Why

After we identified the bug, we want to figure out why it occurs. We tested "company" column and "website" column. The bug only occurs when we move "website" column but not "company" column. What's the difference between them? The only difference I can tell is their sizes. The "company" column is much wider than the "website" column, but this information doesn't seem to be helpful. So we look for next clue. How do we swap the columns?

function makeColumnsSwappable(columnsContainer, elementsToPatch = []) {
  columnsContainer.addEventListener('pointerdown', e => {
    let columnElements = [...columnsContainer.children]
    const firstTarget = e.target
    let firstTargetIndex = columnElements.indexOf(firstTarget)

    function preventDefault(e) {
      e.preventDefault()
    }

    document.addEventListener('selectstart', preventDefault)

    if (firstTargetIndex === -1)
      return

    function handleMove(e) {
      const secondTarget = e.target
      const secondTargetIndex = columnElements.indexOf(secondTarget)

      if (secondTargetIndex === -1)
        return

      if (firstTarget === secondTarget)
        return

      // (2) this is where we want to swap columns
      swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)

      elementsToPatch.forEach((columnsContainer) => {
        swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
      })

      columnElements = [...columnsContainer.children]
      firstTargetIndex = columnElements.indexOf(firstTarget)
    }

    function swapColumns(container, firstTargetIndex, secondTargetIndex) {
      const columns = container.children
      const firstTarget = columns[firstTargetIndex]
      const secondTarget = columns[secondTargetIndex]
      const isMoveToLeft = firstTargetIndex > secondTargetIndex
      const isMoveToRight = firstTargetIndex < secondTargetIndex

      // (3) this is where we actually swap the column elements
      if (isMoveToLeft) {
        secondTarget.insertAdjacentElement('beforebegin', firstTarget)
      } else if (isMoveToRight) {
        secondTarget.insertAdjacentElement('afterend', firstTarget)
      }
    }

    // (1) this is where we handle 'pointermove' event
    columnsContainer.addEventListener('pointermove', handleMove)

    document.addEventListener('pointerup', () => {
      columnsContainer.removeEventListener('pointermove', handleMove)
      document.removeEventListener('selectstart', preventDefault)
    }, { once: true })
  })
}

We swap two column elements by getting two targets, the first target is the one on 'pointerdown', and the second target is the one on 'pointermove'. We assume that we are moving first column to left if firstTargetIndex > secondTargetIndex, and moving first column to right if firstTargetIndex < secondTargetIndex. We use that information to move elements with insertAdjacentElement.

function swapColumns(container, firstTargetIndex, secondTargetIndex) {
  const columns = container.children
  const firstTarget = columns[firstTargetIndex]
  const secondTarget = columns[secondTargetIndex]
  const isMoveToLeft = firstTargetIndex > secondTargetIndex
  const isMoveToRight = firstTargetIndex < secondTargetIndex

  if (isMoveToLeft) {
    secondTarget.insertAdjacentElement('beforebegin', firstTarget)
  } else if (isMoveToRight) {
    secondTarget.insertAdjacentElement('afterend', firstTarget)
  }
}

We can probably tell that using firstTargetIndex and secondTargetIndex to get moving direction is not ideal. But the logic works sometimes. So what goes wrong? Let's look closely to the cases that works and that doesn't this time.

When we are moving "company" column, it looks like the cursor is always on top of it.

When we are moving "website" column, as soon as it swap with "company" column, the cursor is on top of the "company" column.

In our swapping columns logic, we do nothing if first target and second target are the same, and we swap two columns if first target and second target are different. This reflects in the result. The reason that swapping "website" column with "company" column has bug is because "company" column is much wider than "website" column. After "website" column swaps with "company" column, the cursor becomes on top of "company" column. As soon as we move cursor again, they swap. After they swap, the cursor is still on top of "company" column. This repeats again and again on 'pointermove'. That's why they flash.

How

After we find out the reason of the bug, we can start thinking how to fix it. If you'd like to follow along, here is the code from the previous tutorial.

When we are moving a column to right, we only want it to swap with the column on its right. To do that, we need to check if cursor is moving towards right. The same logic applies to moving a column to left.

function makeColumnsSwappable(columnsContainer, elementsToPatch = []) {
  columnsContainer.addEventListener('pointerdown', e => {
    // save first cursor x position on 'pointerdown'
    // e.clientX is the horizontal distance
    // from the left edge of browser window
    // to where the event is triggered
    let lastCursorX = e.clientX
    let columnElements = [...columnsContainer.children]
    const firstTarget = e.target
    let firstTargetIndex = columnElements.indexOf(firstTarget)

    function preventDefault(e) {
      e.preventDefault()
    }

    document.addEventListener('selectstart', preventDefault)

    if (firstTargetIndex === -1)
      return

    function handleMove(e) {
      // save new cursor x position on 'pointermove'
      const newCursorX = e.clientX
      const secondTarget = e.target
      const secondTargetIndex = columnElements.indexOf(secondTarget)

      if (secondTargetIndex === -1)
        return

      if (firstTarget === secondTarget)
        return

      // compare `newCursorX` and `lastCursorX` to check 
      // which direction the cursor is moving towards
      const isMoveToLeft = newCursorX < lastCursorX
      const isMoveToRight = newCursorX > lastCursorX
      // update `lastCursorX` for next comparison 
      lastCursorX = newCursorX

      swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)

      elementsToPatch.forEach((columnsContainer) => {
        swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
      })

      columnElements = [...columnsContainer.children]
      firstTargetIndex = columnElements.indexOf(firstTarget)
    }

    function swapColumns(container, firstTargetIndex, secondTargetIndex) {
      const columns = container.children
      const firstTarget = columns[firstTargetIndex]
      const secondTarget = columns[secondTargetIndex]
      const isMoveToLeft = firstTargetIndex > secondTargetIndex
      const isMoveToRight = firstTargetIndex < secondTargetIndex

      if (isMoveToLeft) {
        secondTarget.insertAdjacentElement('beforebegin', firstTarget)
      } else if (isMoveToRight) {
        secondTarget.insertAdjacentElement('afterend', firstTarget)
      }
    }

    columnsContainer.addEventListener('pointermove', handleMove)

    document.addEventListener('pointerup', () => {
      columnsContainer.removeEventListener('pointermove', handleMove)
      document.removeEventListener('selectstart', preventDefault)
    }, { once: true })
  })
}

Update logic with new isMoveToLeft and isMoveToRight

function handleMove(e) {
  //...
  // pass `isMoveToLeft` and `isMoveToRight`
  swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight)

  elementsToPatch.forEach((columnsContainer) => {
    // pass `isMoveToLeft` and `isMoveToRight`
    swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight)
  })
  // ...
}

// update to take two more parameter 
// `isMoveToLeft` and `isMoveToRight`
function swapColumns(container, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight) {
  const columns = container.children
  const firstTarget = columns[firstTargetIndex]
  const secondTarget = columns[secondTargetIndex]
  // no longer using `firstTargetIndex` and `secondTargetIndex`
  // to calculate `isMoveToLeft` and `isMoveToRight` from here

  if (isMoveToLeft) {
    secondTarget.insertAdjacentElement('beforebegin', firstTarget)
  } else if (isMoveToRight) {
    secondTarget.insertAdjacentElement('afterend', firstTarget)
  }
}

Great! By making the logic correct, the bug is fixed!

One last thing to do is to refactor our code a little bit. function swapColumns now takes 5 parameters in order, if we accidently pass the arguments in a wrong order, the function won't work. The parameters are all required, so what can we do? Well, we can use object to make those parameters as a group, so the order won't matter.

// `swapColumns` now only have two parameters
// the second one is an object
function swapColumns(container, {
  firstTargetIndex,
  secondTargetIndex,
  isMoveToLeft,
  isMoveToRight
}) { //...

Group the arguments to an object

function handleMove(e) {
  //...
  swapColumns(columnsContainer, {
    // as long as we provide these in this object
    // the order of them doesn't matter at all
    firstTargetIndex,
    secondTargetIndex,
    isMoveToLeft,
    isMoveToRight
  })

  elementsToPatch.forEach((columnsContainer) => {
    swapColumns(columnsContainer, {
      // the order is different than above
      isMoveToLeft,
      isMoveToRight,
      secondTargetIndex,
      firstTargetIndex
    })
  })
  // ...
}

Since the two objects we pass to swapColumns have the exact same properties, We can also group it like this.

  const swapColumnInfo = {
    firstTargetIndex,
    secondTargetIndex,
    isMoveToLeft,
    isMoveToRight
  }

  swapColumns(columnsContainer, swapColumnInfo)

  elementsToPatch.forEach((columnsContainer) => {
    swapColumns(columnsContainer, swapColumnInfo)
  })

The code should still work. Note that grouping all swapColumns parameters to one object is also possible. It will require slightly more change. You can try it out as an exercise. Just to be clear, this refactor is not necessary as we probably won't use swapColumns elsewhere. I merely want to show that how to modify parameters to make a function easier to use.

Blog

table: fix moving columns bug

Gohomewho

What

Why

How

Join Our Newsletter. No Spam, Only the good stuff.

Related