From 6d30e01d1c6851c978982e2611b38a91b5617e8e Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Fri, 22 Jan 2021 19:05:14 -0300 Subject: [PATCH] A11Y: Structure user menu as tabs. (#11789) * A11Y: Structure user menu as tabs. Although the user menu content has the appearance of tabs and relies on the functionality of tabs to make sense in terms of content and focus order, it is not marked up correctly as tabs and tab panels. See [WAI-ARIA Authoring Practices 1.1](https://www.w3.org/TR/wai-aria-practices-1.1/#tabpanel) and the [example](https://www.w3.org/TR/wai-aria-practices-1.1/examples/tabs/tabs-2/tabs.html) for details. * Make plugin api backwards compatible --- .../discourse/app/lib/plugin-api.js | 4 +- .../discourse/app/widgets/button.js | 24 +++++- .../app/widgets/quick-access-panel.js | 9 ++ .../discourse/app/widgets/user-menu.js | 67 +++++++++++---- .../integration/widgets/user-menu-test.js | 10 ++- .../stylesheets/common/base/menu-panel.scss | 85 +++++++++---------- 6 files changed, 132 insertions(+), 67 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/plugin-api.js b/app/assets/javascripts/discourse/app/lib/plugin-api.js index b536e80dbba..ce4145cd5e8 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-api.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-api.js @@ -737,10 +737,10 @@ class PluginApi { * example: * * api.addUserMenuGlyph({ - * label: 'awesome.label', + * title: 'awesome.label', * className: 'my-class', * icon: 'my-icon', - * href: `/some/path` + * data: { url: `/some/path` }, * }); * */ diff --git a/app/assets/javascripts/discourse/app/widgets/button.js b/app/assets/javascripts/discourse/app/widgets/button.js index bb93acdbb5a..fe5c6d93aeb 100644 --- a/app/assets/javascripts/discourse/app/widgets/button.js +++ b/app/assets/javascripts/discourse/app/widgets/button.js @@ -38,6 +38,17 @@ export const ButtonClass = { attributes.title = title; } + if (attrs.role) { + attributes["role"] = attrs.role; + } + + if (attrs.tabAttrs) { + const tab = attrs.tabAttrs; + attributes["aria-selected"] = tab["aria-selected"]; + attributes["tabindex"] = tab["tabindex"]; + attributes["aria-controls"] = tab["aria-controls"]; + } + if (attrs.disabled) { attributes.disabled = "true"; } @@ -51,11 +62,20 @@ export const ButtonClass = { return attributes; }, + _buildIcon(attrs) { + const icon = iconNode(attrs.icon, { class: attrs.iconClass }); + if (attrs["aria-label"]) { + icon.properties.attributes["role"] = "img"; + icon.properties.attributes["aria-hidden"] = false; + } + return icon; + }, + html(attrs) { const contents = []; const left = !attrs.iconRight; if (attrs.icon && left) { - contents.push(iconNode(attrs.icon, { class: attrs.iconClass })); + contents.push(this._buildIcon(attrs)); } if (attrs.label) { contents.push( @@ -75,7 +95,7 @@ export const ButtonClass = { contents.push(attrs.contents); } if (attrs.icon && !left) { - contents.push(iconNode(attrs.icon, { class: attrs.iconClass })); + contents.push(this._buildIcon(attrs)); } return contents; diff --git a/app/assets/javascripts/discourse/app/widgets/quick-access-panel.js b/app/assets/javascripts/discourse/app/widgets/quick-access-panel.js index 23145723b53..8c6f0996f2a 100644 --- a/app/assets/javascripts/discourse/app/widgets/quick-access-panel.js +++ b/app/assets/javascripts/discourse/app/widgets/quick-access-panel.js @@ -40,6 +40,15 @@ export default createWidget("quick-access-panel", { return Promise.resolve([]); }, + buildAttributes() { + const attributes = this.attrs; + attributes["aria-labelledby"] = this.key; + attributes["tabindex"] = "0"; + attributes["role"] = "tabpanel"; + + return attributes; + }, + newItemsLoaded() {}, itemHtml(item) {}, // eslint-disable-line no-unused-vars diff --git a/app/assets/javascripts/discourse/app/widgets/user-menu.js b/app/assets/javascripts/discourse/app/widgets/user-menu.js index e532b38a211..7c89f3da0c5 100644 --- a/app/assets/javascripts/discourse/app/widgets/user-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/user-menu.js @@ -23,51 +23,82 @@ export function addUserMenuGlyph(glyph) { createWidget("user-menu-links", { tagName: "div.menu-links-header", + _tabAttrs(quickAccessType) { + return { + "aria-controls": `quick-access-${quickAccessType}`, + "aria-selected": "false", + tabindex: "-1", + }; + }, + + // TODO: Remove when 2.7 gets released. + _structureAsTab(extraGlyph) { + const glyph = extraGlyph; + // Assume glyph is a button if it has a data-url field. + if (!glyph.data || !glyph.data.url) { + glyph.title = glyph.label; + glyph.data = { url: glyph.href }; + + glyph.label = null; + glyph.href = null; + } + + glyph.role = "tab"; + glyph.tabAttrs = this._tabAttrs(glyph.actionParam); + + return glyph; + }, + profileGlyph() { return { - label: "user.preferences", + title: "user.preferences", className: "user-preferences-link", icon: "user", - href: `${this.attrs.path}/summary`, action: UserMenuAction.QUICK_ACCESS, actionParam: QuickAccess.PROFILE, - "aria-label": "user.preferences", + data: { url: `${this.attrs.path}/summary` }, + role: "tab", + tabAttrs: this._tabAttrs(QuickAccess.PROFILE), }; }, notificationsGlyph() { return { - label: "user.notifications", + title: "user.notifications", className: "user-notifications-link", icon: "bell", - href: `${this.attrs.path}/notifications`, action: UserMenuAction.QUICK_ACCESS, actionParam: QuickAccess.NOTIFICATIONS, - "aria-label": "user.notifications", + data: { url: `${this.attrs.path}/notifications` }, + role: "tab", + tabAttrs: this._tabAttrs(QuickAccess.NOTIFICATIONS), }; }, bookmarksGlyph() { return { + title: "user.bookmarks", action: UserMenuAction.QUICK_ACCESS, actionParam: QuickAccess.BOOKMARKS, - label: "user.bookmarks", className: "user-bookmarks-link", icon: "bookmark", - href: `${this.attrs.path}/activity/bookmarks`, + data: { url: `${this.attrs.path}/activity/bookmarks` }, "aria-label": "user.bookmarks", + role: "tab", + tabAttrs: this._tabAttrs(QuickAccess.BOOKMARKS), }; }, messagesGlyph() { return { + title: "user.private_messages", action: UserMenuAction.QUICK_ACCESS, actionParam: QuickAccess.MESSAGES, - label: "user.private_messages", className: "user-pms-link", icon: "envelope", - href: `${this.attrs.path}/messages`, - "aria-label": "user.private_messages", + data: { url: `${this.attrs.path}/messages` }, + role: "tab", + tabAttrs: this._tabAttrs(QuickAccess.MESSAGES), }; }, @@ -82,7 +113,7 @@ createWidget("user-menu-links", { if (this.isActive(glyph)) { glyph = this.markAsActive(glyph); } - return this.attach("link", $.extend(glyph, { hideLabel: true })); + return this.attach("flat-button", glyph); }, html() { @@ -94,7 +125,8 @@ createWidget("user-menu-links", { g = g(this); } if (g) { - glyphs.push(g); + const structuredGlyph = this._structureAsTab(g); + glyphs.push(structuredGlyph); } }); } @@ -108,9 +140,10 @@ createWidget("user-menu-links", { glyphs.push(this.profileGlyph()); - return h("ul.menu-links-row", [ + return h("div.menu-links-row", [ h( - "li.glyphs", + "div.glyphs", + { attributes: { "aria-label": "Menu links", role: "tablist" } }, glyphs.map((l) => this.glyphHtml(l)) ), ]); @@ -121,6 +154,7 @@ createWidget("user-menu-links", { // the full page. definition.action = null; definition.actionParam = null; + definition.url = definition.data.url; if (definition.className) { definition.className += " active"; @@ -128,6 +162,9 @@ createWidget("user-menu-links", { definition.className = "active"; } + definition.tabAttrs["tabindex"] = "0"; + definition.tabAttrs["aria-selected"] = "true"; + return definition; }, diff --git a/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js b/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js index 891c07101c7..afe1ad3bf68 100644 --- a/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js +++ b/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js @@ -79,7 +79,9 @@ discourseModule("Integration | Component | Widget | user-menu", function ( const routeToStub = sinon.stub(DiscourseURL, "routeTo"); await click(".user-notifications-link"); assert.ok( - routeToStub.calledWith(queryAll(".user-notifications-link")[0].href), + routeToStub.calledWith( + queryAll(".user-notifications-link").data("url") + ), "a second click should redirect to the full notifications page" ); }, @@ -120,7 +122,7 @@ discourseModule("Integration | Component | Widget | user-menu", function ( }, async test(assert) { - const userPmsLink = queryAll(".user-pms-link")[0]; + const userPmsLink = queryAll(".user-pms-link").data("url"); assert.ok(userPmsLink); await click(".user-pms-link"); @@ -143,7 +145,7 @@ discourseModule("Integration | Component | Widget | user-menu", function ( const routeToStub = sinon.stub(DiscourseURL, "routeTo"); await click(".user-pms-link"); assert.ok( - routeToStub.calledWith(userPmsLink.href), + routeToStub.calledWith(userPmsLink), "a second click should redirect to the full private messages page" ); }, @@ -171,7 +173,7 @@ discourseModule("Integration | Component | Widget | user-menu", function ( const routeToStub = sinon.stub(DiscourseURL, "routeTo"); await click(".user-bookmarks-link"); assert.ok( - routeToStub.calledWith(queryAll(".user-bookmarks-link")[0].href), + routeToStub.calledWith(queryAll(".user-bookmarks-link").data("url")), "a second click should redirect to the full bookmarks page" ); }, diff --git a/app/assets/stylesheets/common/base/menu-panel.scss b/app/assets/stylesheets/common/base/menu-panel.scss index 9d93eb26f86..2fde6944ae1 100644 --- a/app/assets/stylesheets/common/base/menu-panel.scss +++ b/app/assets/stylesheets/common/base/menu-panel.scss @@ -380,76 +380,73 @@ div.menu-links-header { display: flex; width: 100%; z-index: 2; + justify-content: space-between; - li { + .glyphs { display: inline-flex; align-items: center; - flex-wrap: wrap; + flex-wrap: nowrap; + width: 100%; + justify-content: space-between; + padding: 0; - &.glyphs { - flex-wrap: nowrap; - width: 100%; - justify-content: space-between; - padding: 0; - - a { - display: flex; - flex: 1 1 auto; - padding: 0.65em 0.25em 0.75em; - justify-content: center; - } - } - - a, button { - // This is to make sure active and inactive tab icons have the same - // size. `box-sizing` does not work and I have no idea why. - border: 1px solid transparent; - &:not(.active):hover { - border-bottom: 0; - margin-top: -1px; - } + display: flex; + flex: 1 1 auto; + padding: 0.65em 0.25em 0.75em; + justify-content: center; + } + } + + button { + // This is to make sure active and inactive tab icons have the same + // size. `box-sizing` does not work and I have no idea why. + border: 1px solid transparent; + &:not(.active):hover { + border-bottom: 0; + margin-top: -1px; + } + } + + button.active { + border: 1px solid var(--primary-low); + border-bottom: 1px solid var(--secondary); + position: relative; + + .d-icon { + color: var(--primary-high); } - a.active { - border: 1px solid var(--primary-low); - border-bottom: 1px solid var(--secondary); - position: relative; - - .d-icon { - color: var(--primary-high); - } - - &:focus, - &:hover { - background-color: inherit; - } + &:focus, + &:hover { + background-color: inherit; } } } - a:hover, - a:focus { + + button:hover, + button:focus { background-color: var(--highlight-medium); outline: none; } - a { + button { padding: 0.3em 0.5em; } - li { + .glyphs { display: table-cell; width: auto; text-align: center; } - li:first-child { + .glyphs:first-child { text-align: left; } - li:last-child { + .glyphs:last-child { text-align: right; } .fa, - a { + button { color: var(--primary-med-or-secondary-med); } }