From fc199d42fa0f1eaa0ae3860134352812d916a976 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 20 Jun 2023 15:47:17 +1000 Subject: [PATCH] FIX: Add aria-label attribute to cooked hashtags (#22182) This commit adds an aria-label attribute to cooked hashtags using the post/chat message decorateCooked functionality. I have just used the inner content of the hashtag (the tag/category/channel name) for the label -- we can reexamine at some point if we want something different like "Link to dev category" or something, but from what I can tell things like Twitter don't even have aria-labels for hashtags so the text would be read out directly. This commit also refactors any ruby specs checking the HTML of hashtags to use rspec-html-matchers which is far clearer than having to maintain the HTML structure in a HEREDOC for comparison, and gives better spec failures. c.f. https://meta.discourse.org/t/hashtags-are-getting-a-makeover/248866/23?u=martin --- .../hashtag-post-decorations.js | 13 +- .../discourse/app/lib/hashtag-autocomplete.js | 7 +- .../discourse/initializers/chat-decorators.js | 9 +- .../lib/chat/channel_archive_service_spec.rb | 17 ++- plugins/chat/spec/models/chat/message_spec.rb | 17 ++- .../spec/system/hashtag_autocomplete_spec.rb | 66 ++++++++-- spec/lib/email/styles_spec.rb | 10 +- spec/lib/oneboxer_spec.rb | 28 +++- spec/lib/pretty_text_spec.rb | 121 +++++++++++++----- spec/system/hashtag_autocomplete_spec.rb | 87 +++++++++++-- 10 files changed, 290 insertions(+), 85 deletions(-) diff --git a/app/assets/javascripts/discourse/app/instance-initializers/hashtag-post-decorations.js b/app/assets/javascripts/discourse/app/instance-initializers/hashtag-post-decorations.js index 26c5a5406ff..75b462d0ddc 100644 --- a/app/assets/javascripts/discourse/app/instance-initializers/hashtag-post-decorations.js +++ b/app/assets/javascripts/discourse/app/instance-initializers/hashtag-post-decorations.js @@ -1,5 +1,5 @@ import { withPluginApi } from "discourse/lib/plugin-api"; -import { replaceHashtagIconPlaceholder } from "discourse/lib/hashtag-autocomplete"; +import { decorateHashtags } from "discourse/lib/hashtag-autocomplete"; export default { after: "hashtag-css-generator", @@ -10,13 +10,10 @@ export default { withPluginApi("0.8.7", (api) => { if (siteSettings.enable_experimental_hashtag_autocomplete) { - api.decorateCookedElement( - (post) => replaceHashtagIconPlaceholder(post, site), - { - onlyStream: true, - id: "hashtag-icons", - } - ); + api.decorateCookedElement((post) => decorateHashtags(post, site), { + onlyStream: true, + id: "hashtag-icons", + }); } }); }, diff --git a/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js b/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js index 7fc8a221a1a..defdb468bf5 100644 --- a/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js +++ b/app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js @@ -25,8 +25,9 @@ export function cleanUpHashtagTypeClasses() { export function getHashtagTypeClasses() { return hashtagTypeClasses; } -export function replaceHashtagIconPlaceholder(element, site) { +export function decorateHashtags(element, site) { element.querySelectorAll(".hashtag-cooked").forEach((hashtagEl) => { + // Replace the empty icon placeholder span with actual icon HTML. const iconPlaceholderEl = hashtagEl.querySelector( ".hashtag-icon-placeholder" ); @@ -41,6 +42,10 @@ export function replaceHashtagIconPlaceholder(element, site) { .trim(); iconPlaceholderEl.replaceWith(domFromString(hashtagIconHTML)[0]); } + + // Add an aria-label to the hashtag element so that screen readers + // can read the hashtag text. + hashtagEl.setAttribute("aria-label", `${hashtagEl.innerText.trim()}`); }); } diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-decorators.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-decorators.js index 6678209fdce..1e9fc651220 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-decorators.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-decorators.js @@ -1,5 +1,5 @@ import { decorateGithubOneboxBody } from "discourse/instance-initializers/onebox-decorators"; -import { replaceHashtagIconPlaceholder } from "discourse/lib/hashtag-autocomplete"; +import { decorateHashtags } from "discourse/lib/hashtag-autocomplete"; import { withPluginApi } from "discourse/lib/plugin-api"; import highlightSyntax from "discourse/lib/highlight-syntax"; import I18n from "I18n"; @@ -73,10 +73,9 @@ export default { } ); - api.decorateChatMessage( - (element) => replaceHashtagIconPlaceholder(element, site), - { id: "hashtagIcons" } - ); + api.decorateChatMessage((element) => decorateHashtags(element, site), { + id: "hashtagIcons", + }); }, _getScrollParent(node, maxParentSelector) { diff --git a/plugins/chat/spec/lib/chat/channel_archive_service_spec.rb b/plugins/chat/spec/lib/chat/channel_archive_service_spec.rb index 3657f458735..8db6a804fc3 100644 --- a/plugins/chat/spec/lib/chat/channel_archive_service_spec.rb +++ b/plugins/chat/spec/lib/chat/channel_archive_service_spec.rb @@ -194,9 +194,20 @@ describe Chat::ChannelArchiveService do subject.new(@channel_archive).execute expect(@channel_archive.reload.complete?).to eq(true) pm_topic = Topic.private_messages.last - expect(pm_topic.first_post.cooked).to include( - "#{channel.title(user)}", - ) + expect(pm_topic.first_post.cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: channel.relative_url, + "data-type": "channel", + "data-slug": channel.slug, + "data-id": channel.id, + "data-ref": "#{channel.slug}::channel", + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + with_tag("span", text: channel.title(user)) + end end end diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index a9c332c0800..c5775fa8254 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -241,7 +241,7 @@ describe Chat::Message do ) end - it "supports hashtag-autocomplete plugin" do + it "supports hashtag autocomplete" do SiteSetting.chat_enabled = true SiteSetting.enable_experimental_hashtag_autocomplete = true @@ -250,9 +250,18 @@ describe Chat::Message do cooked = described_class.cook("##{category.slug}", user_id: user.id) - expect(cooked).to eq( - "

#{category.name}

", - ) + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end end it "supports censored plugin" do diff --git a/plugins/chat/spec/system/hashtag_autocomplete_spec.rb b/plugins/chat/spec/system/hashtag_autocomplete_spec.rb index 7a772fb6e2b..cf6d090a007 100644 --- a/plugins/chat/spec/system/hashtag_autocomplete_spec.rb +++ b/plugins/chat/spec/system/hashtag_autocomplete_spec.rb @@ -68,15 +68,63 @@ describe "Using #hashtag autocompletion to search for and lookup channels", type cooked_hashtags = page.all(".hashtag-cooked", count: 3) - expect(cooked_hashtags[0]["outerHTML"]).to eq(<<~HTML.chomp) - Random - HTML - expect(cooked_hashtags[1]["outerHTML"]).to eq(<<~HTML.chomp) - Raspberry - HTML - expect(cooked_hashtags[2]["outerHTML"]).to eq(<<~HTML.chomp) - razed - HTML + expect(cooked_hashtags[0]["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: channel2.relative_url, + "data-type": "channel", + "data-slug": "random", + "data-id": channel2.id, + "aria-label": "Random", + }, + ) do + with_tag( + "svg", + with: { + class: + "fa d-icon d-icon-comment svg-icon hashtag-color--channel-#{channel2.id} svg-string", + }, + ) { with_tag("use", with: { href: "#comment" }) } + end + + expect(cooked_hashtags[1]["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": "raspberry-beret", + "data-id": category.id, + "aria-label": "Raspberry", + }, + ) do + with_tag( + "span", + with: { + class: "hashtag-category-badge hashtag-color--category-#{category.id}", + }, + ) + end + + expect(cooked_hashtags[2]["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": "razed", + "data-id": tag.id, + "aria-label": "razed", + }, + ) do + with_tag( + "svg", + with: { + class: "fa d-icon d-icon-tag svg-icon hashtag-color--tag-#{tag.id} svg-string", + }, + ) { with_tag("use", with: { href: "#tag" }) } + end end context "when a user cannot access the category for a cooked channel hashtag" do diff --git a/spec/lib/email/styles_spec.rb b/spec/lib/email/styles_spec.rb index a5190008940..0363358a7f5 100644 --- a/spec/lib/email/styles_spec.rb +++ b/spec/lib/email/styles_spec.rb @@ -109,12 +109,10 @@ RSpec.describe Email::Styles do end it "replaces hashtag-cooked text with raw #hashtag" do - hashtag_html = - "Dev Zone" - frag = html_fragment(hashtag_html) - expect(frag.at("a").text.chomp).to eq("#dev") - hashtag_html = - "Dev Zone" + category = Fabricate(:category, name: "dev", slug: "dev") + post = Fabricate(:post, raw: "this is #dev") + post.rebake! + hashtag_html = post.cooked frag = html_fragment(hashtag_html) expect(frag.at("a").text.chomp).to eq("#dev") end diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb index f814494cba9..24edcd95472 100644 --- a/spec/lib/oneboxer_spec.rb +++ b/spec/lib/oneboxer_spec.rb @@ -190,9 +190,31 @@ RSpec.describe Oneboxer do .inner_html .chomp .strip - expect(preview).to eq(<<~HTML.chomp.strip) - This post has some hashtags, #{category.name} and #{tag.name} - HTML + expect(preview).to include("This post has some hashtags") + expect(preview).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end + expect(preview).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": tag.name, + "data-id": tag.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end end end diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb index d918c174cbd..df6fc039632 100644 --- a/spec/lib/pretty_text_spec.rb +++ b/spec/lib/pretty_text_spec.rb @@ -1776,24 +1776,60 @@ RSpec.describe PrettyText do cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id) - expect(cooked).to include("#unknown::tag") - expect(cooked).to include( - "known", - ) - expect(cooked).to include( - "known", - ) - expect(cooked).to include( - "testing", - ) - expect(cooked).to include("#secret") + expect(cooked).to have_tag("span", text: "#unknown::tag", with: { class: "hashtag-raw" }) + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category2.url, + "data-type": "category", + "data-slug": category2.slug, + "data-id": category2.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": tag.name, + "data-id": tag.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end + expect(cooked).to have_tag("span", text: "#secret", with: { class: "hashtag-raw" }) # If the user hash access to the private category it should be cooked with the details + icon group.add(user) cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id) - expect(cooked).to include( - "secret", - ) + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: private_category.url, + "data-type": "category", + "data-slug": private_category.slug, + "data-id": private_category.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id) @@ -1809,10 +1845,18 @@ RSpec.describe PrettyText do expect(cooked).to eq(html.strip) cooked = PrettyText.cook("test #known::tag", user_id: user.id) - html = <<~HTML -

test known

- HTML - expect(cooked).to eq(html.strip) + expect(cooked).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": tag.name, + "data-id": tag.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end # ensure it does not fight with the autolinker expect(PrettyText.cook(" http://somewhere.com/#known")).not_to include("hashtag") @@ -2051,22 +2095,35 @@ HTML replacement: "discourse", ) - expect(PrettyText.cook("@test #test test")).to match_html(<<~HTML) -

- @test - #test - tdiscourset -

- HTML + cooked = PrettyText.cook("@test #test test") + expect(cooked).to have_tag("a", text: "@test", with: { class: "mention", href: "/u/test" }) + expect(cooked).to have_tag( + "a", + text: "#test", + with: { + class: "hashtag", + href: "/c/test/#{category.id}", + }, + ) + expect(cooked).to include("tdiscourset") SiteSetting.enable_experimental_hashtag_autocomplete = true - expect(PrettyText.cook("@test #test test")).to match_html(<<~HTML) -

- @test - test - tdiscourset -

- HTML + cooked = PrettyText.cook("@test #test test") + expect(cooked).to have_tag("a", text: "@test", with: { class: "mention", href: "/u/test" }) + expect(cooked).to have_tag( + "a", + text: "test", + with: { + class: "hashtag-cooked", + href: "/c/test/#{category.id}", + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + }, + ) do + with_tag("span", with: { class: "hashtag-icon-placeholder" }) + end + expect(cooked).to include("tdiscourset") end it "supports overlapping words" do diff --git a/spec/system/hashtag_autocomplete_spec.rb b/spec/system/hashtag_autocomplete_spec.rb index 545f827181f..960ac099cdd 100644 --- a/spec/system/hashtag_autocomplete_spec.rb +++ b/spec/system/hashtag_autocomplete_spec.rb @@ -53,25 +53,53 @@ describe "Using #hashtag autocompletion to search for and lookup categories and ) end - it "cooks the selected hashtag clientside with the correct url and icon" do + it "cooks the selected hashtag clientside in the composer preview with the correct url and icon" do visit_topic_and_initiate_autocomplete hashtag_results = page.all(".hashtag-autocomplete__link", count: 2) hashtag_results[0].click expect(page).to have_css(".hashtag-cooked") cooked_hashtag = page.find(".hashtag-cooked") - expected = <<~HTML.chomp - Cool Category - HTML - expect(cooked_hashtag["outerHTML"].squish).to eq(expected) + + expect(cooked_hashtag["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + }, + ) do + with_tag( + "span", + with: { + class: "hashtag-category-badge hashtag-color--category-#{category.id}", + }, + ) + end visit_topic_and_initiate_autocomplete hashtag_results = page.all(".hashtag-autocomplete__link", count: 2) hashtag_results[1].click expect(page).to have_css(".hashtag-cooked") cooked_hashtag = page.find(".hashtag-cooked") - expect(cooked_hashtag["outerHTML"].squish).to eq(<<~HTML.chomp) - cooltag - HTML + expect(cooked_hashtag["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": tag.name, + "data-id": tag.id, + }, + ) do + with_tag( + "svg", + with: { + class: "fa d-icon d-icon-tag svg-icon hashtag-color--tag-#{tag.id} svg-string", + }, + ) { with_tag("use", with: { href: "#tag" }) } + end end it "cooks the hashtags for tag and category correctly serverside when the post is saved to the database" do @@ -85,12 +113,43 @@ describe "Using #hashtag autocompletion to search for and lookup categories and cooked_hashtags = page.all(".hashtag-cooked", count: 2) - expect(cooked_hashtags[0]["outerHTML"]).to eq(<<~HTML.chomp) - Cool Category - HTML - expect(cooked_hashtags[1]["outerHTML"]).to eq(<<~HTML.chomp) - cooltag - HTML + expect(cooked_hashtags[0]["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: category.url, + "data-type": "category", + "data-slug": category.slug, + "data-id": category.id, + "aria-label": category.name, + }, + ) do + with_tag( + "span", + with: { + class: "hashtag-category-badge hashtag-color--category-#{category.id}", + }, + ) + end + + expect(cooked_hashtags[1]["outerHTML"]).to have_tag( + "a", + with: { + class: "hashtag-cooked", + href: tag.url, + "data-type": "tag", + "data-slug": tag.name, + "data-id": tag.id, + "aria-label": tag.name, + }, + ) do + with_tag( + "svg", + with: { + class: "fa d-icon d-icon-tag svg-icon hashtag-color--tag-#{tag.id} svg-string", + }, + ) { with_tag("use", with: { href: "#tag" }) } + end end context "when a user cannot access the category for a hashtag cooked in another post" do