From 32e1eda3facca3db95c0bd47abe218e217fbf01d Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 8 Mar 2024 09:54:10 -0500 Subject: [PATCH] A11Y: Update bulk selection keyboard shortcuts (#26069) * A11Y: Update bulk selection keyboard shortcuts Still a draft, but in current state this: - adds `shift+b` as a keyboard shortcut to toggle bulk select - adds `shift+d` as a keyboard shortcut to dismiss selected topic(s) (this replaces `x r` and `x t` shortcuts) - adds `x` as a keyboard shortcut to toggle selection (while in bulk select mode) - fixes a bug with the `shift+a` shortcut, which was not working properly Note that there is a breaking change here. Previously we had: - `x r` to dismiss new topics - `x t` to dismiss unread topics However, this meant that we couldn't use `x` for selection, because the itsatrap library does not allow the same character to be used both as a single character shortcut and as the start of a sequence. The proposed solution here is more consistent with other apps (Gmail, Github) that use `x` to toggle selection. Also, we never show both "Dismiss New" and "Dismiss Unread" in the same screen, hence it makes sense to consolidate both actions under `shift+d`. * Address review --- .../modal/keyboard-shortcuts-help.js | 11 ++++--- .../discourse/app/lib/keyboard-shortcuts.js | 29 ++++++++++++++--- .../acceptance/keyboard-shortcuts-test.js | 18 +++++------ config/locales/client.en.yml | 5 +-- .../page_objects/components/topic_list.rb | 8 +++++ .../components/topic_list_header.rb | 4 +++ spec/system/topic_bulk_select_spec.rb | 32 +++++++++++++++++++ spec/system/topic_page_spec.rb | 19 ++++++++++- 8 files changed, 104 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/modal/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/app/components/modal/keyboard-shortcuts-help.js index aac374e7529..2ac7161dc06 100644 --- a/app/assets/javascripts/discourse/app/components/modal/keyboard-shortcuts-help.js +++ b/app/assets/javascripts/discourse/app/components/modal/keyboard-shortcuts-help.js @@ -88,11 +88,14 @@ export default class KeyboardShortcutsHelp extends Component { keysDelimiter: PLUS, }), help: buildShortcut("application.help", { keys1: ["?"] }), - dismiss_new: buildShortcut("application.dismiss_new", { - keys1: ["x", "r"], + bulk_select: buildShortcut("application.toggle_bulk_select", { + keys1: [SHIFT, "b"], }), - dismiss_topics: buildShortcut("application.dismiss_topics", { - keys1: ["x", "t"], + dismiss: buildShortcut("application.dismiss", { + keys1: [SHIFT, "d"], + }), + x: buildShortcut("application.x", { + keys1: ["x"], }), log_out: buildShortcut("application.log_out", { keys1: [SHIFT, "z"], diff --git a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js index 42332ed5828..620fb08ac25 100644 --- a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js +++ b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js @@ -116,12 +116,14 @@ const DEFAULT_BINDINGS = { "shift+f11": { handler: "fullscreenComposer", global: true }, "shift+u": { handler: "deferTopic" }, "shift+a": { handler: "toggleAdminActions" }, + "shift+b": { handler: "toggleBulkSelect" }, t: { postAction: "replyAsNewTopic" }, u: { handler: "goBack", anonymous: true }, - "x r": { - click: "#dismiss-new-bottom,#dismiss-new-top", - }, // dismiss new - "x t": { click: "#dismiss-topics-bottom,#dismiss-topics-top" }, // dismiss topics + x: { handler: "bulkSelectItem" }, + "shift+d": { + click: + "#dismiss-new-bottom, #dismiss-new-top, #dismiss-topics-bottom, #dismiss-topics-top", + }, // dismiss new or unread }; const animationDuration = 100; @@ -412,6 +414,13 @@ export default { this._moveSelection({ direction: -1, scrollWithinPosts: true }); }, + bulkSelectItem() { + const elem = document.querySelector( + ".selected input.bulk-select, .selected .select-post" + ); + elem?.click(); + }, + goBack() { history.back(); }, @@ -895,7 +904,17 @@ export default { }, toggleAdminActions() { - this.appEvents.trigger("topic:toggle-actions"); + document.querySelector(".toggle-admin-menu")?.click(); + }, + + toggleBulkSelect() { + const bulkSelect = document.querySelector("button.bulk-select"); + + if (bulkSelect) { + bulkSelect.click(); + } else { + getOwner(this).lookup("controller:topic").send("toggleMultiSelect"); + } }, toggleArchivePM() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/keyboard-shortcuts-test.js b/app/assets/javascripts/discourse/tests/acceptance/keyboard-shortcuts-test.js index c4f5f9903db..1709997f8fc 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/keyboard-shortcuts-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/keyboard-shortcuts-test.js @@ -153,8 +153,7 @@ acceptance("Keyboard Shortcuts - Authenticated Users", function (needs) { exists("#dismiss-topics-top"), "dismiss unread top button is present" ); - await triggerKeyEvent(document, "keypress", "X"); - await triggerKeyEvent(document, "keypress", "T"); + await triggerKeyEvent(document, "keydown", "D", { shiftKey: true }); assert.ok( exists("#dismiss-read-confirm"), "confirmation modal to dismiss unread is present" @@ -183,8 +182,8 @@ acceptance("Keyboard Shortcuts - Authenticated Users", function (needs) { exists("#dismiss-topics-bottom"), "dismiss unread bottom button is hidden" ); - await triggerKeyEvent(document, "keypress", "X"); - await triggerKeyEvent(document, "keypress", "T"); + + await triggerKeyEvent(document, "keydown", "D", { shiftKey: true }); assert.ok( exists("#dismiss-read-confirm"), "confirmation modal to dismiss unread is present" @@ -212,8 +211,8 @@ acceptance("Keyboard Shortcuts - Authenticated Users", function (needs) { document.getElementById("ember-testing-container").scrollTop = 0; await visit("/new"); assert.ok(exists("#dismiss-new-top"), "dismiss new top button is present"); - await triggerKeyEvent(document, "keypress", "X"); - await triggerKeyEvent(document, "keypress", "R"); + + await triggerKeyEvent(document, "keydown", "D", { shiftKey: true }); assert.strictEqual(resetNewCalled, 1); // we get rid of all but one topic so the bottom dismiss button doesn't @@ -229,8 +228,8 @@ acceptance("Keyboard Shortcuts - Authenticated Users", function (needs) { exists("#dismiss-new-bottom"), "dismiss new bottom button has been hidden" ); - await triggerKeyEvent(document, "keypress", "X"); - await triggerKeyEvent(document, "keypress", "R"); + + await triggerKeyEvent(document, "keydown", "D", { shiftKey: true }); assert.strictEqual(resetNewCalled, 2); // restore the original topic list @@ -252,8 +251,7 @@ acceptance("Keyboard Shortcuts - Authenticated Users", function (needs) { "dismiss new bottom button is present" ); - await triggerKeyEvent(document, "keypress", "X"); - await triggerKeyEvent(document, "keypress", "R"); + await triggerKeyEvent(document, "keydown", "D", { shiftKey: true }); assert.strictEqual(resetNewCalled, 1); }); diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 695ff8fbd7a..6d823a16fe6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4254,8 +4254,9 @@ en: search: "%{shortcut} Search" filter_sidebar: "%{shortcut} Filter sidebar" help: "%{shortcut} Open keyboard help" - dismiss_new: "%{shortcut} Dismiss New" - dismiss_topics: "%{shortcut} Dismiss Topics" + toggle_bulk_select: "%{shortcut} Toggle bulk select" + dismiss: "%{shortcut} Dismiss selected topic(s)" + x: "%{shortcut} Toggle selection (in bulk select mode)" log_out: "%{shortcut} Log Out" composing: title: "Composing" diff --git a/spec/system/page_objects/components/topic_list.rb b/spec/system/page_objects/components/topic_list.rb index dfb1e8df5cb..9be201d9b7f 100644 --- a/spec/system/page_objects/components/topic_list.rb +++ b/spec/system/page_objects/components/topic_list.rb @@ -46,6 +46,14 @@ module PageObjects page.has_no_css?("#{topic_list_item_unread_badge(topic)}") end + def has_checkbox_selected_on_row?(n) + page.has_css?("#{TOPIC_LIST_ITEM_SELECTOR}:nth-child(#{n}) input.bulk-select:checked") + end + + def has_no_checkbox_selected_on_row?(n) + page.has_no_css?("#{TOPIC_LIST_ITEM_SELECTOR}:nth-child(#{n}) input.bulk-select:checked") + end + def click_topic_checkbox(topic) find("#{topic_list_item_class(topic)} input#bulk-select-#{topic.id}").click end diff --git a/spec/system/page_objects/components/topic_list_header.rb b/spec/system/page_objects/components/topic_list_header.rb index a3bfc07fea0..81191c5ba14 100644 --- a/spec/system/page_objects/components/topic_list_header.rb +++ b/spec/system/page_objects/components/topic_list_header.rb @@ -50,6 +50,10 @@ module PageObjects find("#topic-bulk-action-options__silent").click end + def click_dismiss_read_confirm + find("#dismiss-read-confirm").click + end + private def bulk_select_dropdown_item(name) diff --git a/spec/system/topic_bulk_select_spec.rb b/spec/system/topic_bulk_select_spec.rb index f671489027c..48073907c12 100644 --- a/spec/system/topic_bulk_select_spec.rb +++ b/spec/system/topic_bulk_select_spec.rb @@ -87,5 +87,37 @@ describe "Topic bulk select", type: :system do visit("/latest") expect(topic_list).to have_no_unread_badge(topics.first) end + + it "works with keyboard shortcuts" do + sign_in(admin) + visit("/latest") + + send_keys([:shift, "b"]) + send_keys("j") + send_keys("x") # toggle select + expect(topic_list).to have_checkbox_selected_on_row(1) + + send_keys("x") # toggle deselect + expect(topic_list).to have_no_checkbox_selected_on_row(1) + + # watch topic and add a reply so we have something in /unread + topic = topics.first + visit("/t/#{topic.slug}/#{topic.id}") + topic_page.watch_topic + expect(topic_page).to have_read_post(1) + Fabricate(:post, topic: topic) + + visit("/unread") + expect(topic_list).to have_topics + + send_keys([:shift, "b"]) + send_keys("j") + send_keys("x") + send_keys([:shift, "d"]) + + topic_list_header.click_dismiss_read_confirm + + expect(topic_list).to have_no_topics + end end end diff --git a/spec/system/topic_page_spec.rb b/spec/system/topic_page_spec.rb index b8325c3b38a..4a1efbc2ca9 100644 --- a/spec/system/topic_page_spec.rb +++ b/spec/system/topic_page_spec.rb @@ -2,6 +2,7 @@ describe "Topic page", type: :system do fab!(:topic) + fab!(:admin) before { Fabricate(:post, topic: topic, cooked: <<~HTML) }

@@ -58,7 +59,7 @@ describe "Topic page", type: :system do PostDestroyer.new(Discourse.system_user, post2).destroy PostDestroyer.new(Discourse.system_user, post3).destroy - sign_in Fabricate(:admin) + sign_in admin end it "displays the gap to admins, and allows them to expand it" do @@ -69,4 +70,20 @@ describe "Topic page", type: :system do expect(page).to have_css(".topic-post", count: 4) end end + + it "supports shift+a kbd shortcut to toggle admin menu" do + sign_in admin + + visit("/t/#{topic.slug}/#{topic.id}") + + expect(".topic-admin-menu-button-container").to be_present + + send_keys([:shift, "a"]) + + expect(page).to have_css(".topic-admin-popup-menu") + + send_keys([:shift, "a"]) + + expect(page).to have_no_css(".topic-admin-popup-menu") + end end