Escape key not closing add funds dialog

[jkup]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).

Will fix #3800

Auditors @bbondy @bsclifton

I'm not sure that code in preferences.js was necessary because as far as I can tell there is no way to close a parent modal when a child modal is open (which is how other sites do it!).

This approach has each component store the previouslyFocused element before it mounts and then restores focus to it before it unmounts. This lets us easily return focus back to a parent modal so you can hit escape multiple times to close multiple modals.

This also has the nice added benefit of restoring keyboard only users to their previously focused item after closing a modal.

[luixxiul]

@jkup Sweet! Would you please consider to add a function to close the modal by clicking outside the modal dialog?

[jkup]

@luixxiul yesss! definitely.. I'll add that as soon as I can figure out why my code isn't working!

[bsclifton]

@jkup here's a patch which solves the issue (although, it's a sloppy fix):

diff --git a/js/about/preferences.js b/js/about/preferences.js
index aa3ff31..6ffb1a0 100644
--- a/js/about/preferences.js
+++ b/js/about/preferences.js
@@ -1891,12 +1891,15 @@ class AboutPreferences extends React.Component {

   setOverlayVisible (isVisible, overlayName) {
     let stateDiff = {}
-    stateDiff[`${overlayName}OverlayVisible`] = isVisible
-    if (overlayName === 'addFunds' && isVisible === false) {
+    const childVisible = this.state.bitcoinOverlayVisible || this.state.qrcodeOverlayVisible
+    if (childVisible && overlayName === 'addFunds' && isVisible === false) {
       // Hide the child overlays when the parent is closed
       stateDiff['bitcoinOverlayVisible'] = false
       stateDiff['qrcodeOverlayVisible'] = false
+    } else {
+      stateDiff[`${overlayName}OverlayVisible`] = isVisible
     }
+
     this.setState(stateDiff)
     // Tell ledger when Add Funds overlay is closed
     if (isVisible === false && overlayName === 'addFunds') {

The problem is: stateDiff[${overlayName}OverlayVisible] = isVisible was always being set to false, since it's being called with false. So the child modals and the parent are hidden.

When states need to be nested (which are most UIs), having a datastructure that you can use like a stack to record per-state information is ideal. You should be able to use an array with push/pop to record the parent/child relationships. What would you push? One quick example would be: when showing the overlay, push an onClose handler.

class AboutPreferences extends React.Component {
  constructor () {
    super()
    //..
    this.state = {
      stateStack = []

  //..

  // the call which shows the QR code
  // it passes in the handler which should fire when it's closed
  setOverlayVisible('qrcode', true, () => {
    this.setState({bitcoinOverlayVisible: false, qrcodeOverlayVisible: false})
    ipc.send(messages.ADD_FUNDS_CLOSED)
  })

  //..

  // the actual implementation for setOverlayVisible
  setOverlayVisible (isVisible, overlayName, onCloseHandler) {
    const stateStack = this.state.stateStack
    if (isVisible === false) {
      const onClose = this.stateStack.pop()
      if (onClose) onClose()
    } else {
      this.state.stateStack.push(onCloseHandler)
    }
    this.setState({stateStack: stateStack)
  }

[bsclifton]

@jkup any update on this one?

[jkup]

sorry this one slipped... I'll work on it now!

[bsclifton]

This works as-is... but if the focus changes it breaks. I'm going to have to reject this one for now