From eb237e634a440f4b26e6c097e7afb9a135bb3781 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 23 Mar 2022 13:03:56 +0300 Subject: [PATCH] A11Y: Focus last viewed topic in topic lists (take 3) (#16257) Another attempt at fixing https://meta.discourse.org/t/discourse-with-a-screen-reader/178105/88?u=osama. Previous PR (reverted): #16240. The problems with the previous PR were: 1. As you scrolled down a topics list, the first topic of every new batch of topics would receive focus and the indicator would show up. 2. Similar to 1, clicking the `See X new or updated topics` notice would also focus a random topic from the new topics that were just loaded. 3. Topics in the suggested topics list received focus too 4. Our custom focus indicator appeared on mobile, but it shouldn't. This commit should have none of these problems. --- .../app/components/topic-list-item.js | 52 ++++++++++++++++--- .../app/templates/components/topic-list.hbs | 3 +- .../app/templates/discovery/topics.hbs | 4 +- .../discourse/app/templates/tag/show.hbs | 1 + .../last-visited-topic-focus-test.js | 26 ++++++++++ .../stylesheets/common/base/_topic-list.scss | 11 ++++ 6 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/acceptance/last-visited-topic-focus-test.js diff --git a/app/assets/javascripts/discourse/app/components/topic-list-item.js b/app/assets/javascripts/discourse/app/components/topic-list-item.js index 7e3a46dec8..a1cec9d310 100644 --- a/app/assets/javascripts/discourse/app/components/topic-list-item.js +++ b/app/assets/javascripts/discourse/app/components/topic-list-item.js @@ -1,4 +1,7 @@ -import discourseComputed, { observes } from "discourse-common/utils/decorators"; +import discourseComputed, { + bind, + observes, +} from "discourse-common/utils/decorators"; import Component from "@ember/component"; import DiscourseURL from "discourse/lib/url"; import I18n from "I18n"; @@ -58,6 +61,11 @@ export default Component.extend({ if (this.selected && this.selected.includes(this.topic)) { this.element.querySelector("input.bulk-select").checked = true; } + const title = this.element.querySelector(".main-link .title"); + if (title) { + title.addEventListener("focus", this._onTitleFocus); + title.addEventListener("blur", this._onTitleBlur); + } }); } }, @@ -98,6 +106,11 @@ export default Component.extend({ if (this.includeUnreadIndicator) { this.messageBus.unsubscribe(this.unreadIndicatorChannel); } + const title = this.element?.querySelector(".main-link .title"); + if (title) { + title.removeEventListener("focus", this._onTitleFocus); + title.removeEventListener("blur", this._onTitleBlur); + } }, @discourseComputed("topic.id") @@ -259,12 +272,21 @@ export default Component.extend({ return; } - const $topic = $(this.element); - $topic - .addClass("highlighted") - .attr("data-islastviewedtopic", opts.isLastViewedTopic); - - $topic.on("animationend", () => $topic.removeClass("highlighted")); + this.element.classList.add("highlighted"); + this.element.setAttribute( + "data-islastviewedtopic", + opts.isLastViewedTopic + ); + this.element.addEventListener("animationend", () => { + this.element.classList.remove("highlighted"); + }); + if ( + this.focusLastVisitedTopic && + opts.isLastViewedTopic && + !this.site.mobileView + ) { + this.element.querySelector(".main-link .title").focus(); + } }); }, @@ -279,4 +301,20 @@ export default Component.extend({ this.highlight(); } }), + + @bind + _onTitleFocus() { + if (this.element && !this.isDestroying && !this.isDestroyed) { + const mainLink = this.element.querySelector(".main-link"); + mainLink.classList.add("focused"); + } + }, + + @bind + _onTitleBlur() { + if (this.element && !this.isDestroying && !this.isDestroyed) { + const mainLink = this.element.querySelector(".main-link"); + mainLink.classList.remove("focused"); + } + }, }); diff --git a/app/assets/javascripts/discourse/app/templates/components/topic-list.hbs b/app/assets/javascripts/discourse/app/templates/components/topic-list.hbs index 3ff3f75731..3a554aea95 100644 --- a/app/assets/javascripts/discourse/app/templates/components/topic-list.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/topic-list.hbs @@ -42,7 +42,8 @@ lastVisitedTopic=lastVisitedTopic selected=selected lastChecked=lastChecked - tagsForUser=tagsForUser}} + tagsForUser=tagsForUser + focusLastVisitedTopic=focusLastVisitedTopic}} {{raw "list/visited-line" lastVisitedTopic=lastVisitedTopic topic=topic}} {{/each}} diff --git a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs index 3c2997c58c..946d599382 100644 --- a/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/discovery/topics.hbs @@ -62,7 +62,9 @@ model=model showResetNew=showResetNew showDismissRead=showDismissRead resetNew=( topics=model.topics discoveryList=true scrollOnLoad=true - onScroll=discoveryTopicList.saveScrollPosition}} + onScroll=discoveryTopicList.saveScrollPosition + focusLastVisitedTopic=true + }} {{/if}} {{plugin-outlet name="after-topic-list" tagName="span" connectorTagName="div" args=(hash category=category)}} diff --git a/app/assets/javascripts/discourse/app/templates/tag/show.hbs b/app/assets/javascripts/discourse/app/templates/tag/show.hbs index 917209dd74..49dedfdf66 100644 --- a/app/assets/javascripts/discourse/app/templates/tag/show.hbs +++ b/app/assets/javascripts/discourse/app/templates/tag/show.hbs @@ -88,6 +88,7 @@ changeSort=(action "changeSort") onScroll=discoveryTopicList.saveScrollPosition scrollOnLoad=true + focusLastVisitedTopic=true }} {{/if}} {{/discovery-topics-list}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/last-visited-topic-focus-test.js b/app/assets/javascripts/discourse/tests/acceptance/last-visited-topic-focus-test.js new file mode 100644 index 0000000000..b2456c72da --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/last-visited-topic-focus-test.js @@ -0,0 +1,26 @@ +import { acceptance, query } from "discourse/tests/helpers/qunit-helpers"; +import { test } from "qunit"; +import { visit } from "@ember/test-helpers"; +import { cloneJSON } from "discourse-common/lib/object"; +import topicFixtures from "discourse/tests/fixtures/topic"; + +acceptance("Last Visited Topic Focus", function (needs) { + needs.pretender((server, helper) => { + const fixture = cloneJSON(topicFixtures["/t/54077.json"]); + fixture.id = 11996; + fixture.slug = + "its-really-hard-to-navigate-the-create-topic-reply-pane-with-the-keyboard"; + server.get("/t/11996.json", () => helper.response(fixture)); + }); + test("last visited topic receives focus when you return back to the topic list", async function (assert) { + await visit("/"); + await visit( + "/t/its-really-hard-to-navigate-the-create-topic-reply-pane-with-the-keyboard/11996" + ); + await visit("/"); + const visitedTopicTitle = query( + '.topic-list-body tr[data-topic-id="11996"] .main-link' + ); + assert.ok(visitedTopicTitle.classList.contains("focused")); + }); +}); diff --git a/app/assets/stylesheets/common/base/_topic-list.scss b/app/assets/stylesheets/common/base/_topic-list.scss index ed62a5c702..90646ddcac 100644 --- a/app/assets/stylesheets/common/base/_topic-list.scss +++ b/app/assets/stylesheets/common/base/_topic-list.scss @@ -234,6 +234,17 @@ .raw-topic-link > * { pointer-events: none; } + + &.focused { + box-shadow: inset 3px 0 0 var(--tertiary); + } + /* we have a custom focus indicator so we can remove the native one */ + .title:focus { + outline: none; + } + .title:focus-visible { + outline: none; + } } .unread-indicator {