From d03d223fd991a21b8d692c7959ca9972b01b86b7 Mon Sep 17 00:00:00 2001
From: Nolan Lawson <nolan@nolanlawson.com>
Date: Sat, 23 Nov 2019 11:25:36 -0800
Subject: [PATCH] fix: remove aria-pressed entirely (#1647)

More progress on #1633
---
 src/routes/_components/IconButton.html        | 36 ++++++-------------
 .../_components/compose/ComposeToolbar.html   |  4 +--
 .../profile/AccountProfileFollow.html         |  1 -
 .../_components/status/StatusToolbar.html     |  3 --
 tests/spec/015-compose-content-warnings.js    |  8 ++---
 tests/spec/127-compose-polls.js               |  5 +--
 6 files changed, 16 insertions(+), 41 deletions(-)

diff --git a/src/routes/_components/IconButton.html b/src/routes/_components/IconButton.html
index 995208a7..905cca51 100644
--- a/src/routes/_components/IconButton.html
+++ b/src/routes/_components/IconButton.html
@@ -1,14 +1,12 @@
-<!-- Toggle buttons should have an immutable label, e.g. a mute/unmute button should just always say
-     "mute." For sighted users, though, I think it's nice if the title changes when the action changes.
-     See http://w3c.github.io/aria-practices/#button
-     We also have toggleButton === false which can make it just a normal button that changes its aria-label.
-     -->
-<button type="button"
-        {title}
+<!-- Normally "pressable" icons would be toggle buttons, but to avoid having the titles and labels mismatched
+     due to guidelines from http://w3c.github.io/aria-practices/#button , we just use normal buttons and change
+     the aria-label instead. See discussion in: https://github.com/nolanlawson/pinafore/issues/1633 -->
+<button id={elementId}
+        type="button"
+        title={ariaLabel}
         aria-label={ariaLabel}
-        aria-pressed={ariaPressed}
         aria-hidden={ariaHidden ? 'true' : undefined}
-        tabindex="{ariaHidden ? '-1' : '0'}"
+        tabindex={ariaHidden ? '-1' : '0'}
         class={computedClass}
         {disabled}
         ref:node
@@ -99,14 +97,11 @@
 
   export default {
     oncreate () {
-      const { clickListener, elementId } = this.get()
+      const { clickListener } = this.get()
       if (clickListener) {
         this.onClick = this.onClick.bind(this)
         this.refs.node.addEventListener('click', this.onClick)
       }
-      if (elementId) {
-        this.refs.node.setAttribute('id', elementId)
-      }
       if (process.env.NODE_ENV !== 'production') {
         const { pressable, pressedLabel, label } = this.get()
         if (pressable && ((!pressedLabel || !label) || pressedLabel === label)) {
@@ -125,15 +120,14 @@
       muted: false,
       disabled: false,
       svgClassName: undefined,
-      elementId: undefined,
+      elementId: '',
       pressable: false,
       pressed: false,
       pressedLabel: undefined,
       className: undefined,
       sameColorWhenPressed: false,
       ariaHidden: false,
-      clickListener: true,
-      toggleButton: true // whether or not to actually present it as a toggle button to screen readers (when pressable)
+      clickListener: true
     }),
     store: () => store,
     computed: {
@@ -146,15 +140,7 @@
         sameColorWhenPressed ? 'same-pressed' : 'not-same-pressed',
         className
       )),
-      title: ({ pressable, pressed, pressedLabel, label }) => pressable ? (pressed ? pressedLabel : label) : label,
-      ariaLabel: ({ toggleButton, pressable, pressed, label, pressedLabel }) => {
-        if (!pressable || toggleButton) {
-          return label // per Aria Practices, toggleButtons never change their label
-        } else {
-          return pressed ? pressedLabel : label
-        }
-      },
-      ariaPressed: ({ toggleButton, pressable, pressed }) => (toggleButton && pressable) ? !!pressed : undefined
+      ariaLabel: ({ pressable, pressed, label, pressedLabel }) => ((pressable && pressed) ? pressedLabel : label)
     },
     methods: {
       animate (animation) {
diff --git a/src/routes/_components/compose/ComposeToolbar.html b/src/routes/_components/compose/ComposeToolbar.html
index 0eacc60e..fd5128b2 100644
--- a/src/routes/_components/compose/ComposeToolbar.html
+++ b/src/routes/_components/compose/ComposeToolbar.html
@@ -20,7 +20,7 @@
       pressedLabel="Remove poll"
       href="#fa-bar-chart"
       on:click="onPollClick()"
-      pressable="true"
+      pressable={true}
       pressed={poll && poll.options && poll.options.length}
     />
     <IconButton
@@ -35,7 +35,7 @@
       pressedLabel="Remove content warning"
       href="#fa-exclamation-triangle"
       on:click="onContentWarningClick()"
-      pressable="true"
+      pressable={true}
       pressed={contentWarningShown}
     />
   </div>
diff --git a/src/routes/_components/profile/AccountProfileFollow.html b/src/routes/_components/profile/AccountProfileFollow.html
index fd76f055..1c263a9c 100644
--- a/src/routes/_components/profile/AccountProfileFollow.html
+++ b/src/routes/_components/profile/AccountProfileFollow.html
@@ -13,7 +13,6 @@
     {href}
     {pressable}
     {pressed}
-    toggleButton={false}
     big={!$isVeryTinyMobileSize}
     on:click="onFollowButtonClick(event)"
     ref:icon
diff --git a/src/routes/_components/status/StatusToolbar.html b/src/routes/_components/status/StatusToolbar.html
index af37f366..e06a64b0 100644
--- a/src/routes/_components/status/StatusToolbar.html
+++ b/src/routes/_components/status/StatusToolbar.html
@@ -5,7 +5,6 @@
     pressedLabel="Close reply"
     pressable={true}
     pressed={replyShown}
-    toggleButton={false}
     href={replyIcon}
     clickListener={false}
     elementId={replyKey}
@@ -15,7 +14,6 @@
     pressedLabel="Unboost"
     pressable={!reblogDisabled}
     pressed={reblogged}
-    toggleButton={false}
     disabled={reblogDisabled}
     href={reblogIcon}
     clickListener={false}
@@ -27,7 +25,6 @@
     pressedLabel="Unfavorite"
     pressable={true}
     pressed={favorited}
-    toggleButton={false}
     href="#fa-star"
     clickListener={false}
     elementId={favoriteKey}
diff --git a/tests/spec/015-compose-content-warnings.js b/tests/spec/015-compose-content-warnings.js
index 0706f8b8..1bc8d89d 100644
--- a/tests/spec/015-compose-content-warnings.js
+++ b/tests/spec/015-compose-content-warnings.js
@@ -13,19 +13,16 @@ test('Changes content warnings', async t => {
     .expect(composeContentWarning.exists).notOk()
     .expect(contentWarningButton.getAttribute('aria-label')).eql('Add content warning')
     .expect(contentWarningButton.getAttribute('title')).eql('Add content warning')
-    .expect(contentWarningButton.getAttribute('aria-pressed')).eql('false')
     .click(contentWarningButton)
     .expect(composeContentWarning.exists).ok()
-    .expect(contentWarningButton.getAttribute('aria-label')).eql('Add content warning')
+    .expect(contentWarningButton.getAttribute('aria-label')).eql('Remove content warning')
     .expect(contentWarningButton.getAttribute('title')).eql('Remove content warning')
-    .expect(contentWarningButton.getAttribute('aria-pressed')).eql('true')
     .typeText(composeContentWarning, 'hello content warning', { paste: true })
     .typeText(composeInput, 'secret text', { paste: true })
     .click(notificationsNavButton)
     .click(homeNavButton)
-    .expect(contentWarningButton.getAttribute('aria-label')).eql('Add content warning')
+    .expect(contentWarningButton.getAttribute('aria-label')).eql('Remove content warning')
     .expect(contentWarningButton.getAttribute('title')).eql('Remove content warning')
-    .expect(contentWarningButton.getAttribute('aria-pressed')).eql('true')
     .expect(composeContentWarning.value).eql('hello content warning')
     .expect(composeInput.value).eql('secret text')
     .selectText(composeInput)
@@ -38,7 +35,6 @@ test('Changes content warnings', async t => {
     .expect(composeContentWarning.exists).notOk()
     .expect(contentWarningButton.getAttribute('aria-label')).eql('Add content warning')
     .expect(contentWarningButton.getAttribute('title')).eql('Add content warning')
-    .expect(contentWarningButton.getAttribute('aria-pressed')).eql('false')
 })
 
 test('Considers content warnings for length limits', async t => {
diff --git a/tests/spec/127-compose-polls.js b/tests/spec/127-compose-polls.js
index 19323b9b..abd61265 100644
--- a/tests/spec/127-compose-polls.js
+++ b/tests/spec/127-compose-polls.js
@@ -28,7 +28,6 @@ test('Can add and remove poll', async t => {
     .expect(composePoll.exists).notOk()
     .expect(pollButton.getAttribute('aria-label')).eql('Add poll')
     .expect(pollButton.getAttribute('title')).eql('Add poll')
-    .expect(pollButton.getAttribute('aria-pressed')).eql('false')
     .click(pollButton)
     .expect(composePoll.exists).ok()
     .expect(getComposePollNthInput(1).value).eql('')
@@ -37,9 +36,8 @@ test('Can add and remove poll', async t => {
     .expect(getComposePollNthInput(4).exists).notOk()
     .expect(composePollMultipleChoice.checked).notOk()
     .expect(composePollExpiry.value).eql(POLL_EXPIRY_DEFAULT.toString())
-    .expect(pollButton.getAttribute('aria-label')).eql('Add poll')
+    .expect(pollButton.getAttribute('aria-label')).eql('Remove poll')
     .expect(pollButton.getAttribute('title')).eql('Remove poll')
-    .expect(pollButton.getAttribute('aria-pressed')).eql('true')
     .click(pollButton)
     .expect(composePoll.exists).notOk()
 })
@@ -51,7 +49,6 @@ test('Can add and remove poll options', async t => {
     .expect(composePoll.exists).notOk()
     .expect(pollButton.getAttribute('aria-label')).eql('Add poll')
     .expect(pollButton.getAttribute('title')).eql('Add poll')
-    .expect(pollButton.getAttribute('aria-pressed')).eql('false')
     .click(pollButton)
     .expect(composePoll.exists).ok()
     .typeText(getComposePollNthInput(1), 'first', { paste: true })