From 4f9fb5f25382e609346011c02810567c929e60b9 Mon Sep 17 00:00:00 2001
From: Nolan Lawson <nolan@nolanlawson.com>
Date: Sun, 8 Dec 2019 18:03:26 -0800
Subject: [PATCH] fix: settings pages preserve focus (#1666)

fixes #1658
---
 .../_components/settings/SettingsLayout.html  | 18 +++---
 .../settings/SettingsListButton.html          |  1 +
 .../_components/settings/SettingsNavItem.html |  1 +
 .../_pages/settings/instances/index.html      | 61 ++++++++++---------
 tests/spec/010-focus.js                       | 29 ++++++++-
 tests/utils.js                                |  4 ++
 6 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/src/routes/_components/settings/SettingsLayout.html b/src/routes/_components/settings/SettingsLayout.html
index c42fc1f3..f2ba5d86 100644
--- a/src/routes/_components/settings/SettingsLayout.html
+++ b/src/routes/_components/settings/SettingsLayout.html
@@ -1,10 +1,12 @@
-<SettingsNav {page} {label}/>
+<FocusRestoration realm={page}>
+  <SettingsNav {page} {label}/>
 
-<div class="settings">
-  <FreeTextLayout>
-    <slot></slot>
-  </FreeTextLayout>
-</div>
+  <div class="settings">
+    <FreeTextLayout>
+      <slot></slot>
+    </FreeTextLayout>
+  </div>
+</FocusRestoration>
 <style>
   .settings {
     margin: 20px;
@@ -24,11 +26,13 @@
 <script>
   import SettingsNav from './SettingsNav.html'
   import FreeTextLayout from '../FreeTextLayout'
+  import FocusRestoration from '../FocusRestoration.html'
 
   export default {
     components: {
       FreeTextLayout,
-      SettingsNav
+      SettingsNav,
+      FocusRestoration
     }
   }
 </script>
diff --git a/src/routes/_components/settings/SettingsListButton.html b/src/routes/_components/settings/SettingsListButton.html
index 73092716..8b5d5779 100644
--- a/src/routes/_components/settings/SettingsListButton.html
+++ b/src/routes/_components/settings/SettingsListButton.html
@@ -1,4 +1,5 @@
 <a {href}
+   id="settings-list-button-{href}"
    on:click="fire('click', event)"
    rel="prefetch"
    aria-label={ariaLabel || label}
diff --git a/src/routes/_components/settings/SettingsNavItem.html b/src/routes/_components/settings/SettingsNavItem.html
index 2c63ae49..0749f49e 100644
--- a/src/routes/_components/settings/SettingsNavItem.html
+++ b/src/routes/_components/settings/SettingsNavItem.html
@@ -1,4 +1,5 @@
 <a class="settings-nav-item {className}"
+   id="settings-nav-item-{href}"
    aria-label={ariaLabel}
    rel="prefetch"
    {href} >
diff --git a/src/routes/_pages/settings/instances/index.html b/src/routes/_pages/settings/instances/index.html
index 8befd9fd..a1a5aa87 100644
--- a/src/routes/_pages/settings/instances/index.html
+++ b/src/routes/_pages/settings/instances/index.html
@@ -2,35 +2,40 @@
   <h1>Instances</h1>
 
   {#if $isUserLoggedIn}
-  <p>Instances you've logged in to:</p>
-  <RadioGroup id="instance-switch" label="Switch to instance" length={numInstances}>
-    <SettingsList label="Instances">
-      {#each instanceStates as instance}
-      <SettingsListRow>
-        <SettingsListButton className="instance-switcher-instance-name"
-                            href="/settings/instances/{instance.name}"
-                            label={instance.name}
-                            ariaLabel={instance.label} />
-        <div class="instance-switcher-button-wrapper">
-          <RadioGroupButton
-                  id="instance-switch"
-                  className="instance-switcher-button"
-                  label={instance.switchLabel}
-                  checked={instance.current}
-                  index={instance.index}
-                  on:click="onSwitchToThisInstance(event, instance.name)">
-            <SvgIcon className="instance-switcher-button-svg"
-                     href={instance.current ? '#fa-star' : '#fa-star-o'} />
-          </RadioGroupButton>
-        </div>
-      </SettingsListRow>
-      {/each}
-    </SettingsList>
-  </RadioGroup>
-  <p><a rel="prefetch" href="/settings/instances/add">Add another instance</a></p>
+    <p>Instances you've logged in to:</p>
+    <RadioGroup id="instance-switch" label="Switch to instance" length={numInstances}>
+      <SettingsList label="Instances">
+        {#each instanceStates as instance}
+        <SettingsListRow>
+          <SettingsListButton className="instance-switcher-instance-name"
+                              href="/settings/instances/{instance.name}"
+                              label={instance.name}
+                              ariaLabel={instance.label} />
+          <div class="instance-switcher-button-wrapper">
+            <RadioGroupButton
+                    id="instance-switch"
+                    className="instance-switcher-button"
+                    label={instance.switchLabel}
+                    checked={instance.current}
+                    index={instance.index}
+                    on:click="onSwitchToThisInstance(event, instance.name)">
+              <SvgIcon className="instance-switcher-button-svg"
+                       href={instance.current ? '#fa-star' : '#fa-star-o'} />
+            </RadioGroupButton>
+          </div>
+        </SettingsListRow>
+        {/each}
+      </SettingsList>
+    </RadioGroup>
+    <p>
+      <a rel="prefetch" href="/settings/instances/add" id="log-in-link-1">Add another instance</a>
+    </p>
   {:else}
-  <p>You're not logged in to any instances.</p>
-  <p><a rel="prefetch" href="/settings/instances/add">Log in to an instance</a> to start using Pinafore.</p>
+    <p>You're not logged in to any instances.</p>
+    <p>
+      <a rel="prefetch" href="/settings/instances/add" id="log-in-link-2">Log in to an instance</a>
+      to start using Pinafore.
+    </p>
   {/if}
 </SettingsLayout>
 <style>
diff --git a/tests/spec/010-focus.js b/tests/spec/010-focus.js
index 4b3b3436..90077aba 100644
--- a/tests/spec/010-focus.js
+++ b/tests/spec/010-focus.js
@@ -14,7 +14,7 @@ import {
   getActiveElementTagName,
   getActiveElementClassList,
   getNthStatusSensitiveMediaButton,
-  getActiveElementAriaLabel
+  getActiveElementAriaLabel, settingsNavButton, getActiveElementHref
 } from '../utils'
 import { loginAsFoobar } from '../roles'
 import { Selector as $ } from 'testcafe'
@@ -155,3 +155,30 @@ test('preserves focus two levels deep', async t => {
     .expect(getUrl()).eql('http://localhost:4002/')
     .expect(getActiveElementClassList()).contains('status-author-name')
 })
+
+test('preserves focus on settings page', async t => {
+  await loginAsFoobar(t)
+  await t
+    .click(settingsNavButton)
+    .click($('a[href="/settings/instances"]'))
+    .expect(getUrl()).eql('http://localhost:4002/settings/instances')
+    .click($('a[href="/settings/instances/add"]'))
+    .expect(getUrl()).eql('http://localhost:4002/settings/instances/add')
+  await goBack()
+  await t
+    .expect(getUrl()).eql('http://localhost:4002/settings/instances')
+    .expect(getActiveElementHref()).eql('/settings/instances/add')
+  await goBack()
+  await t
+    .expect(getUrl()).eql('http://localhost:4002/settings')
+    .expect(getActiveElementHref()).eql('/settings/instances')
+    .click($('a[href="/settings/instances"]'))
+    .expect(getUrl()).eql('http://localhost:4002/settings/instances')
+    .click($('a.settings-nav-item[href="/settings"]'))
+    .expect(getUrl()).eql('http://localhost:4002/settings')
+  await goBack()
+  await t
+    .expect(getUrl()).eql('http://localhost:4002/settings/instances')
+    .expect(getActiveElementHref()).eql('/settings')
+    .expect(getActiveElementClassList()).contains('settings-nav-item')
+})
diff --git a/tests/utils.js b/tests/utils.js
index 2b7ab0cc..fa822185 100644
--- a/tests/utils.js
+++ b/tests/utils.js
@@ -132,6 +132,10 @@ export const getActiveElementClassList = exec(() =>
   (document.activeElement && (document.activeElement.getAttribute('class') || '').split(/\s+/)) || []
 )
 
+export const getActiveElementHref = exec(() =>
+  (document.activeElement && (document.activeElement.getAttribute('href') || ''))
+)
+
 export const getActiveElementTagName = exec(() =>
   (document.activeElement && document.activeElement.tagName) || ''
 )