bbc / lrud

Left, Right, Up, Down. A spatial navigation library for devices with input via directional controls.
Apache License 2.0
98 stars 21 forks source link

Error when unregistering node #23

Closed NickGoward closed 5 years ago

NickGoward commented 5 years ago

Describe the bug When a node is unregistered, LRUD tries to assign focus to something else. This seems to cause an issue when the node being unregistered is higher in the tree than the currently focused node.

To Reproduce

const nav = new Lrud()

nav.register('root', {
  orientation: 'horizontal'
})

nav.register('node1', {
  orientation: 'vertical',
  parent: 'root'
})

nav.register('node2', {
  orientation: 'vertical',
  parent: 'root'
})

nav.register('item1', {
  parent: 'node1',
  selectAction: {}
})

nav.register('item2', {
  parent: 'node2',
  selectAction: {}
})

nav.assignFocus('node2')

nav.unregisterNode('item1')

Expected behavior Should remove item1. Instead, LRUD throws an error.

index.min.js:158 Uncaught TypeError: Cannot read property 'id' of null
    at e.digDown (index.min.js:158)
    at e.unregisterNode (index.min.js:86)

Personally, I would prefer LRUD not to attempt to focus on anything when I remove a node.

thomascgray commented 5 years ago

Cheers for this @NickGoward . Apologies for the late reply.

Thanks for the reproducible test case. The test should pass, so it suggests there is a fix to be done anyway.

However, as we both said and as you commented, I think we should introduce an optional flag along the lines of "please don't try and find focus" to the unregisterNode function call, so I'll look into that too.

thomascgray commented 5 years ago

This has been fixed as of https://github.com/bbc/lrud/pull/24 and publish as lrud@3.5.0 https://www.npmjs.com/package/lrud