Make header toolbar dropdown menu accessible #5897

foxbunny · 2023-08-21T13:11:23Z

This patch makes the following changes:

Implements a new <ind-menu> custom element that is used to add necessary aria-* attributes to a combination of <button> and <menu> in the tollbar menu
Adds two folders, client/{js,styles}/custom-elements, that will be used to house Indico custom elements
Includes the ind-menu module in client/js/index.js
Restyles the toolbar menu to allow buttons to be used as menu items

See #5896

This PR requires the #5895 to be merged

CHANGES.rst

foxbunny · 2023-09-22T07:57:28Z

Rebased only the relevant changes on top of master, and also addressed code style issues discussed elsewhere.

foxbunny · 2023-10-02T06:00:30Z

Rebased on master, updated folder structure to match previous PRs.

ThiefMaster · 2023-10-17T12:49:37Z

I rebased it to the latest master and cleaned up some merge problems in the changelog.

The one thing I'm missing a bit in the new version is the little arrow indicating that a top menu item opens a submenu:

Also, can we avoid this little bit of white spacing between the main menu and its submenu?

ThiefMaster · 2023-10-17T12:47:11Z

indico/web/client/styles/partials/_main.scss

+  * {
+    box-sizing: border-box;
+  }


I think this is no longer needed since you added it globally in a previous PR?

I'm going to clean this up later once I get done with category display page. Still have some issues to address there. But yeah, I'll remove this once I get it it.

ThiefMaster · 2023-10-17T12:48:55Z

indico/web/client/styles/partials/_main.scss

@@ -11,26 +11,63 @@

 /* Styles for the main menu */

-.toolbar.global-menu {
-  @include font-family-title-light();


Any reason for removing this (which changes the font? If it needs to be larger fine, but new font looks strange there.

I tried adding it back locally, and IMHO it looks better (master vs your PR + this line added back):

So the idea is to first bring all styling under the DS, and then improve the design afterwards.

but new font looks strange there.

It's not the new font. It's just inheriting it from one of the ancestors. I want to flush out SUI first before I address font-related stuff.

I'd just prefer to have a "FIXME" / "remove later" with a 'hardcoded' font in here when merging it, instead of having it "broken" in master for some time and only change to the proper font in a later PR

(we already use 3.3/master in production on a smaller instance and I don't want to give them the surprise of a "strange" font change once i deploy the latest master there)

Fair enough. I should note that Roboto Light is most certainly not the appropriate font for this use case. In fact, any "Light" font is not a good use case for normal size text (and below) in general. You may use it on larger headings, but that's a different use case. Not all font rendering engines are made the same, and light fonts generally tend to render poorly on anything that isn't a Mac.

All in all we'll need to address the font choices anyway at some point. My long-term plan after the migration to DS is complete is to pick a good font set that works well across the board in terms of readability, licensing, and also from the aesthetic perspective.

Sure, but IMHO those "more invasive" changes are better to keep for later since we can do all the nice improvements at first without exposing users to big "oh wth why does this look completely different now?!?!" surprises :)

"oh wth why does this look completely different now?!?!"

This is called a "canoe" problem. It's a temporary distraction that does not have no serious impact on usability. We generally shouldn't upgrade canoe problems into serious failures. ;)

indico/web/client/js/custom_elements/ind_menu.js

ThiefMaster · 2023-10-17T12:54:00Z

indico/web/client/js/custom_elements/ind_menu.js

+          });
+        });
+
+        this.addEventListener('keydown', function(ev) {


ThiefMaster · 2023-10-17T12:57:02Z

indico/web/client/js/custom_elements/ind_menu.js

+          trigger.setAttribute('aria-expanded', trigger.getAttribute('aria-expanded') !== 'true');
+        });
+
+        this.addEventListener('focusout', function() {


Generally I prefer arrow functions for callbacks (also the ones above/below), but this one is interesting because of the this semantics.

I think in terms of readability and maintainability (I actually had to double-check the docs how this is used by addEventListener) it might be better to just use arrows and put a more explicit const elem = evt.target; (or .currentTarget) inside the callback.

Generally I prefer arrow functions for callbacks

Sure. I've switched to arrows in newer PRs.

would be nice to update it in here as well :)

foxbunny · 2023-11-08T08:16:33Z

@ThiefMaster Where does this tool tip come from? Tried searching the exact text but nothing shows up.

ThiefMaster · 2023-11-08T08:19:16Z

you find it when searching for event-type-tooltip-lecture

foxbunny · 2023-11-08T08:20:43Z

Ugh, more jQuery :D

ThiefMaster · 2023-12-06T11:55:03Z

indico/web/client/js/custom_elements/ind_with_tooltip.js

+
+import {TipBase} from 'indico/custom_elements/TipBase';
+
+const tipDelay = 1000; // ms


This feels too slow. What about using something around 100-250ms? (I tested it and it feels way more reactive that way)

So there's this:

But then again, many authors use about .5s delay instead.

So not sure what is "typical" really. At any rate, 100~250 is way too low. This delay is used to minimize distraction while tabbing through the interface.

OK fair enough. 500ms feels fine as well.

(If we ever move the event creation dialog to react I could absolutely see the button to just become "Create event" and the type selection going in the dialog itself instad of being nested in a dropdown)

ThiefMaster · 2023-12-06T11:57:44Z

indico/web/client/js/custom_elements/ind_menu.js

+          trigger.setAttribute('aria-expanded', trigger.getAttribute('aria-expanded') !== 'true');
+        });
+
+        this.addEventListener('focusout', function() {


would be nice to update it in here as well :)

ThiefMaster · 2023-12-06T11:58:54Z

Can we avoid this one-pixel gap between the menu and the expanded dropdown?

ThiefMaster · 2023-12-06T12:00:15Z

Would it be possible to open the tooltip to the right instead of to the top? For the first dropdown item it doesn't matter, but right now it's pretty much impossible to create a lecture after hovering meeting/conference - you have to move the mouse pointer outside the dropdown first so the tooltip closes, since it covers the dropdown entries above.

ThiefMaster · 2023-12-06T12:03:33Z

Is the very dark gray (#333) of the menu bar required for contrast or would something slightly brighter such as #444 or #555 (the previous color) still have strong enough contrast (especially since the text is a bit larger now than it was before - which in this particular place actually looks better than it looked before with the smaller text IMHO ;))?

foxbunny · 2023-12-06T12:08:25Z

Is the very dark gray (#333) of the menu bar required for contrast or would something slightly brighter such as #444 or #555 (the previous color) still have strong enough contrast

As far as grays go, I think the cut-off is #737373. So yeah, we can use that. Should make for better contrast with the tooltip, too.

ThiefMaster · 2023-12-06T12:11:14Z

indico/web/client/js/custom_elements/TipBase.js

+let viewportWidth = document.documentElement.clientWidth;
+let viewportHeight = document.documentElement.clientHeight;
+
+window.addEventListener('DOMContentLoaded', () => requestIdleCallback(updateClientGeometry));


domReady.then(...);?

ThiefMaster · 2023-12-06T12:11:26Z

indico/web/client/js/custom_elements/TipBase.js

+    const refCenter = `${referenceRect.x + referenceRect.width / 2}px`;
+    let arrowBorder;
+
+    console.log(referenceRect);


remove that ;)

ThiefMaster · 2023-12-06T12:12:27Z

indico/web/client/js/custom_elements/ind_menu.js

+          'The <menu> element must come after <button>'
+        );
+
+        trigger.setAttribute('aria-expanded', false);


I guess this should be 'false' since AFAIK DOM attributes are always strings? (even though the false is probably implicitly converted to a string

Yes, it's always converted to string, and that is negligible performance hit too, so you'd just waste two characters to be pedantic. :)

Just asking because you (correctly) check for !== 'true' below and I could already see a less experienced developer checking for === false for whatever reason (after all, it's set to false!) and wondering why it doesn't work.

Well, what would it take for them to learn how coercion works in JavaScript?

"Explicit is better than implicit." (even though that's from the zen of python and we have javascript here ;))

There's another saying: 'When in Rome, do as the Romans do.' This is JavaScript. Coercion is everywhere. You're not escaping that. Gotta learn at some point.

I don't have a super strong opinion on keeping/changing it so I'll leave it up to you

ThiefMaster · 2023-12-06T12:16:15Z

As far as grays go, I think the cut-off is #737373

I propose sticking with #555 then (that's our default $black [not commenting on whether or not it makes sense to call a dark gray "black" ;)] in the palette) :)

foxbunny · 2023-12-07T08:13:35Z

@ThiefMaster I think I got everything. Let me know if there's anything else.

ThiefMaster · 2023-12-08T08:24:02Z

indico/web/client/js/custom_elements/tips.scss

+
+ind-with-tooltip,
+ind-with-toggletip {
+  --tooltip-surface-color: #000;


Would it cause any a11y problems if we changed this to #333? Feels a bit very dark right now.

(Might also be personal preference, but for anything dark-mode-related I generally prefer the "dim" version over the "full black background" version, and I guess this also applies here even if it's just a tooltip)

For the tooltip, I would leave it as is, because we cannot predict what ends up behind it. This way, we make sure it stands out.

At least right now we don't have anything darker than the menus and the meeting page background....

(so far the preference within the team is on the #333 version btw)

I suppose we can always change it. I'm currently on the road. I'll get back to this later in the afternoon.

- Correctly indicate the roles and states of the dropdown menu - Use a vanilla JavaScript implementation of the dropdown - Introduce new folders `client/{js,styles}/custom-elements` - Improve accessibility of the tooltips for the #create-* buttons

- Correctly indicate the roles and states of the dropdown menu - Use a vanilla JavaScript implementation of the dropdown - Improve accessibility of the tooltips for the #create-* buttons

foxbunny force-pushed the nav-toolbar branch from bfdd179 to 565bf64 Compare August 21, 2023 13:12

ThiefMaster reviewed Aug 21, 2023

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

foxbunny force-pushed the nav-toolbar branch 2 times, most recently from 415df9b to 73cf794 Compare September 22, 2023 07:56

foxbunny force-pushed the nav-toolbar branch from 73cf794 to 262c38f Compare September 22, 2023 08:03

foxbunny force-pushed the nav-toolbar branch from 262c38f to ab32f85 Compare October 2, 2023 05:59

foxbunny marked this pull request as ready for review October 2, 2023 06:00

foxbunny force-pushed the nav-toolbar branch 2 times, most recently from e2599fe to 61bc0b9 Compare October 2, 2023 06:43

foxbunny mentioned this pull request Oct 2, 2023

[UX/DX] Design system migration #5967

Open

10 tasks

ThiefMaster force-pushed the nav-toolbar branch from 61bc0b9 to f6a386d Compare October 17, 2023 12:40

ThiefMaster reviewed Oct 17, 2023

View reviewed changes

foxbunny marked this pull request as draft October 17, 2023 13:07

foxbunny force-pushed the nav-toolbar branch from f6a386d to 7997604 Compare November 8, 2023 10:39

foxbunny force-pushed the nav-toolbar branch from 7997604 to 4594263 Compare November 23, 2023 06:06

ThiefMaster force-pushed the nav-toolbar branch from 4594263 to 99e4e6e Compare December 6, 2023 11:18

ThiefMaster reviewed Dec 6, 2023

View reviewed changes

foxbunny force-pushed the nav-toolbar branch from 99e4e6e to d232f3b Compare December 7, 2023 08:11

foxbunny marked this pull request as ready for review December 7, 2023 08:12

ThiefMaster reviewed Dec 8, 2023

View reviewed changes

foxbunny force-pushed the nav-toolbar branch from d232f3b to ffa945a Compare December 8, 2023 16:26

ThiefMaster changed the base branch from master to a11y December 10, 2023 23:11

ThiefMaster force-pushed the nav-toolbar branch from ffa945a to 8b8c411 Compare December 10, 2023 23:11

ThiefMaster enabled auto-merge (squash) December 10, 2023 23:14

ThiefMaster merged commit 003267c into indico:a11y Dec 10, 2023
10 checks passed

ThiefMaster added this to the v3.3 milestone Dec 10, 2023

ThiefMaster added the a11y label Feb 2, 2024


		import {TipBase} from 'indico/custom_elements/TipBase';

		const tipDelay = 1000; // ms

Make header toolbar dropdown menu accessible #5897

Make header toolbar dropdown menu accessible #5897

Conversation

foxbunny commented Aug 21, 2023

foxbunny commented Sep 22, 2023 • edited

foxbunny commented Oct 2, 2023

ThiefMaster commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxbunny Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxbunny commented Nov 8, 2023

ThiefMaster commented Nov 8, 2023

foxbunny commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster Dec 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster commented Dec 6, 2023

ThiefMaster commented Dec 6, 2023

ThiefMaster commented Dec 6, 2023 • edited

foxbunny commented Dec 6, 2023 • edited by ThiefMaster

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster commented Dec 6, 2023

foxbunny commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxbunny commented Sep 22, 2023 •

edited

foxbunny Oct 18, 2023 •

edited

ThiefMaster Dec 6, 2023 •

edited

ThiefMaster commented Dec 6, 2023 •

edited

foxbunny commented Dec 6, 2023 •

edited by ThiefMaster