AO3-6553 Better semantics and a11y for the media list on the homepage #4581

neuroalien · 2023-07-17T12:07:19Z

Pull Request Checklist

Have you read "How to write the perfect pull request"?
Have you read the contributing guidelines?
Have you added tests for any changed functionality?
Have you added the Jira issue number
as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
Do you have fewer than 5 pull requests already open? If not, please wait
until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-6553

Purpose

What does this PR do?

Turn homepage media list into navigation

Semantically, the list of media (plus All Fandoms) is a navigation
element, not a menu.

The preferred element to use would be nav, but we don't use this
anywhere else (yet), so for consistency I'm adding a div with the role
navigation.

(Per https://www.w3.org/TR/html-aria/#docconformance, uls are not
allowed to have the role navigation.)

Bonus i18n for media on homepage

This is not strictly related to the a11y issue, but we're trying to add
i18n if we can when we touch a file. (Can remove if needed.)

Testing Instructions

How can the Archive's QA team verify that this is working as you intended?

If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.

References

Are there other relevant issues/pull requests/mailing list discussions?

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.

Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.

sarken · 2023-07-19T07:46:01Z

app/views/menu/_menu_fandoms.html.erb

-    <% Media.for_menu.each do |medium| %>
-      <% unless medium.id.nil? %>
-        <li id="medium_<%= medium.id %>"><%= link_to ts("#{medium.name}", key: 'header.fandom'), medium_fandoms_path(medium) %></li>
+<div role="navigation" aria-label="<%= t(".media") %>">


I think we need to make a new partial for this section of the homepage -- this partial isn't just the navigation on the homepage, but also the Fandoms dropdown menu in the site navigation, so this change would make it inconsistent with the other dropdowns (and possibly negatively affect keyboard navigation of that part, since the plugin we use does rely on the menu role to some extent):

otwarchive/app/views/layouts/_header.html.erb

Lines 40 to 49 in f09c964

<h3 class="landmark heading"><%= ts('Site Navigation', key: 'header') %></h3>

<ul class="primary navigation actions" role="navigation">

<li class="dropdown">

<%= link_to ts('Fandoms', key: 'header'), menu_fandoms_path %>

<%= render 'menu/menu_fandoms' %>

</li>

<li class="dropdown">

<%= link_to ts('Browse', key: 'header'), menu_browse_path %>

<%= render 'menu/menu_browse' %>

</li>

Reusing this partial in the two dissimilar places to be DRY was entirely my boneheaded idea, so you can curse me for that. That's why it has the nonsensical menu role in the first place. #:weary:

Since this would only apply to the homepage, I think it's a great time to try out the <nav> element. There's no reason we shouldn't be able to use that element (it's quite a few years old at this point), and even if something weird arises, it will be confined to a) a little-used page and b) an area with a readily-available alternative that does the same thing (the Fandoms menu). I think it even makes sense to swap the div here for nav rather than adding a div around the list:

otwarchive/app/views/home/index.html.erb

Lines 18 to 25 in f09c964

<div class="browse module">

<h3 class="heading"><%= ts('Find your favorites') %></h3>

<% if logged_in? %>

<p class="note"><%= ts('Browse fandoms by media or favorite up to %{maximum} tags to have them listed here!', maximum: ArchiveConfig.MAX_FAVORITE_TAGS) %></p>

<% end %>

<%= render 'menu/menu_fandoms' %>

</div>

<% end %>

(You can have headings in nav, according to examples on HTML5: Edition for Web Authors 4.4.3 The nav element and mdn's nav element documenation.)

That would require a slight tweak to the CSS here to include .splash nav.module:

otwarchive/public/stylesheets/site/2.0/26-media-narrow.css

Lines 185 to 190 in f09c964

.splash div.module, .logged-in .splash div.module {

clear: both;

margin-left: 0;

margin-right: 0;

width: 100%;

}

sarken · 2023-07-19T07:50:27Z

app/views/menu/_menu_fandoms.html.erb

+<div role="navigation" aria-label="<%= t(".media") %>">
+  <ul class="menu">
+    <li><%= link_to t(".all_fandoms"), media_path %></li>
+    <% cache "menu-fandoms-version4", skip_digest: true do %>


If we end up updating this file, should we change the cache key? On one hand, there shouldn't be a visible difference between the new i18ned version and the current non-18ned version. On the other hand, maybe we should do it to be sure the i18ned version does look the same?

sarken · 2023-07-19T08:04:41Z

app/views/menu/_menu_fandoms.html.erb

+    <% cache "menu-fandoms-version4", skip_digest: true do %>
+      <% Media.for_menu.each do |medium| %>
+        <% unless medium.id.nil? %>
+          <li id="medium_<%= medium.id %>"><%= link_to t(".#{medium.translation_key}"), medium_fandoms_path(medium) %></li>


You can ignore this comment; it's observational rather than something you need to action.

This poses a thrilling question for future!us: media are tags. Should we be translating tag names?

It makes sense here because it's site navigation more than a tag, imo, but will we want to show the localized media name, e.g., in the Parent Tags section on a fandom's tag landing page?

I kind of feel like that page should list the tag as-is (because I doubt we'll translate fandoms in the Parent Tags section, e.g., Good Omens (TV) on Crowley's landing page), how important is it that the site navigation and the parent tags match? 🤔

I think it might be a little premature to i18n the media tags (and in particular this menu). Since it's inside a cache block, and displayed to everyone, it's the type of thing that can lead to site-wide, extremely visible bugs if anyone ever decides to add translations for it.

(Currently, the only pages displayed with a locale are the FAQ pages — but if, say, the cache expires right before someone views a FAQ page in French, and it's that page load that ends up setting the cache, all of a sudden everyone across the entire site is seeing the French version of the menu.)

sarken

Just nitpicks and stuff I screwed up 100 years ago, nothing actually wrong with the accessibility change itself!

sarken · 2023-07-24T07:07:39Z

app/views/home/_fandoms.html.erb

@@ -0,0 +1,10 @@
+<ul class="menu">


I think you can take the class off in addition to the role -- it doesn't seem to be used for the styling and doesn't make much sense here anyway.

sarken · 2023-07-24T07:09:02Z

app/views/home/_fandoms.html.erb

+  <% cache "homepage-fandoms-version1", skip_digest: true do %>
+    <% Media.for_menu.each do |medium| %>
+      <% unless medium.id.nil? %>
+        <li id="medium_<%= medium.id %>"><%= link_to t(".#{medium.translation_key}"), medium_fandoms_path(medium) %></li>


Can you take the id attribute off? I'm not really sure what we use it for, if anything, in the dropdown menus, but having the same id in both places makes for invalid HTML because IDs have to be unique. I just made a mess all over the place with my genius reuse of that partial. 🙃

sarken · 2023-07-24T07:13:24Z

app/views/home/index.html.erb

      <% if logged_in? %>
-        <p class="note"><%= ts('Browse fandoms by media or favorite up to %{maximum} tags to have them listed here!', maximum: ArchiveConfig.MAX_FAVORITE_TAGS) %></p>
+        <p class="note"><%= t(".browse_or_favorite", maximum: ArchiveConfig.MAX_FAVORITE_TAGS) %></p>


I think we need to pass ArchiveConfig.MAX_FAVORITE_TAGS as count so Translation can have multiple versions of this sentence ready in case we ever change MAX_FAVORITE_TAGS.

Possibly clueless question: what does passing it as count mean? Is there some Rails magic to renaming maximum to count, or should I use a different way of invoking translations?

I think sarken means this: https://guides.rubyonrails.org/i18n.html#pluralization

Edit: So there is indeed Rails magic to the count name.

Thank you! Hashtag today-I-learned :D

Yup, that's what I meant!

When we use count, we generally define one: and other: English translations to go with it. It's pretty unlikely we'll ever drop MAX_FAVORITE_TAGS down to one, but if you could update en.yml to have both just to be safe, this will be ready to merge:

index: browse_or_favorite: one: Browse fandoms by media or favorite up to %{count} tag to have it listed here! other: Browse fandoms by media or favorite up to %{count} tags to have them listed here!

ETA: Oops, should probably be "it" rather than "them" for the one, self.

Ahaha, I was coming here to nitpick that very thing :D

(You have to admit that "favourite up to 1 tag" sounds pretty sad though, and the "have it listed here" only adds to the desolation...)

It really does. Something like "or favorite a tag/choose a favorite tag" sounds a bit less sad, or at least less awkward, but it's probably not worth getting bogged down in wording that shouldn't see the light of day unless Catastrophe™ strikes. (Unless you want to! I don't mind workshopping wording; I just don't want you to feel obligated.)

sarken · 2023-07-24T07:14:50Z

spec/models/media_spec.rb

+describe Media do
+  describe "#translation_key" do
+    it "returns a parameterized version of the media name using underscore as a separator" do
+      media = FactoryBot.create(:media, name: "Widgets; Tools & Doodads")


Will create(:media, name: etc) work without the FactoryBot. here?

public/stylesheets/site/2.0/26-media-narrow.css

tickinginstant · 2023-07-27T11:35:09Z

app/views/home/_fandoms.html.erb

+  <% cache "homepage-fandoms-version1", skip_digest: true do %>
+    <% Media.for_menu.each do |medium| %>
+      <% unless medium.id.nil? %>
+        <li><%= link_to t(".#{medium.translation_key}"), medium_fandoms_path(medium) %></li>


For whatever it's worth, I'm still team "don't feed this through i18n this until the caching questions from #4584 (comment) are settled." This is no longer an active bug, since you can't set your locale on the homepage yet, but it could become a bug later if we don't opt to override the cache function.

(Also, I think mobility or globalize would be a better choice for internationalizing medium names. As is, this would make it more difficult for other sites using the otwarchive software to use their own custom media names.)

I don't have the spoons to look into alternate ways of translating, so I've removed the contentious commit. We can cross or burn that bridge when we come closer to it. ;)

Thank you all for reviewing!

Semantically, the list of media (plus All Fandoms) on the homepage is a navigation element, not a menu. The list of media used in the main site menu needs to stay a `menu`, so we're unDRYing the partial and creating a new partial just for use on the homepage. The preferred element to use in modern web development is `nav`. We don't use this anywhere else (yet), so this will be our test case. (Per https://www.w3.org/TR/html-aria/#docconformance, `ul`s are not allowed to have the role `navigation`.)

neuroalien force-pushed the AO3-6553-media-list-homepage branch from 791e8e4 to dc7915a Compare July 17, 2023 12:13

github-actions bot added the Awaiting Review label Jul 17, 2023

sarken requested changes Jul 19, 2023

View reviewed changes

sarken added Reviewed: Action Needed and removed Awaiting Review labels Jul 19, 2023

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jul 20, 2023

neuroalien force-pushed the AO3-6553-media-list-homepage branch from 343f630 to 953f34e Compare July 22, 2023 10:51

CristinaRO requested a review from sarken July 22, 2023 11:01

sarken reviewed Jul 24, 2023

View reviewed changes

sarken added Reviewed: Action Needed and removed Coder Has Actioned Review labels Jul 24, 2023

neuroalien force-pushed the AO3-6553-media-list-homepage branch from 953f34e to 473e3f4 Compare July 24, 2023 21:18

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jul 24, 2023

neuroalien requested a review from sarken July 24, 2023 21:52

neuroalien force-pushed the AO3-6553-media-list-homepage branch 2 times, most recently from e84a467 to 1caf0dc Compare July 26, 2023 23:55

sarken approved these changes Jul 27, 2023

View reviewed changes

sarken added Reviewed: Ready to Merge and removed Coder Has Actioned Review labels Jul 27, 2023

tickinginstant reviewed Jul 27, 2023

View reviewed changes

neuroalien force-pushed the AO3-6553-media-list-homepage branch from 1caf0dc to 6bf0d0e Compare July 27, 2023 21:57

tickinginstant approved these changes Jul 29, 2023

View reviewed changes

brianjaustin added the Priority: High label Jul 29, 2023

sarken merged commit 53395fd into otwcode:master Aug 13, 2023

sarken mentioned this pull request Aug 16, 2023

AO3-6553 Use div with a role rather than nav element #4608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AO3-6553 Better semantics and a11y for the media list on the homepage #4581

AO3-6553 Better semantics and a11y for the media list on the homepage #4581

neuroalien commented Jul 17, 2023

sarken Jul 19, 2023

sarken Jul 19, 2023

sarken Jul 19, 2023

tickinginstant Jul 19, 2023

sarken left a comment

sarken Jul 24, 2023

sarken Jul 24, 2023

sarken Jul 24, 2023

neuroalien Jul 24, 2023

Bilka2 Jul 24, 2023 •

edited

Loading

neuroalien Jul 24, 2023

sarken Jul 25, 2023 •

edited

Loading

neuroalien Jul 25, 2023

sarken Jul 26, 2023

sarken Jul 24, 2023

neuroalien Jul 24, 2023

tickinginstant Jul 27, 2023

neuroalien Jul 27, 2023

	<h3 class="landmark heading"><%= ts('Site Navigation', key: 'header') %></h3>
	<ul class="primary navigation actions" role="navigation">
	<li class="dropdown">
	<%= link_to ts('Fandoms', key: 'header'), menu_fandoms_path %>
	<%= render 'menu/menu_fandoms' %>
	</li>
	<li class="dropdown">
	<%= link_to ts('Browse', key: 'header'), menu_browse_path %>
	<%= render 'menu/menu_browse' %>
	</li>

	<div class="browse module">
	<h3 class="heading"><%= ts('Find your favorites') %></h3>
	<% if logged_in? %>
	<p class="note"><%= ts('Browse fandoms by media or favorite up to %{maximum} tags to have them listed here!', maximum: ArchiveConfig.MAX_FAVORITE_TAGS) %></p>
	<% end %>
	<%= render 'menu/menu_fandoms' %>
	</div>
	<% end %>

	.splash div.module, .logged-in .splash div.module {
	clear: both;
	margin-left: 0;
	margin-right: 0;
	width: 100%;
	}

AO3-6553 Better semantics and a11y for the media list on the homepage #4581

AO3-6553 Better semantics and a11y for the media list on the homepage #4581

Conversation

neuroalien commented Jul 17, 2023

Pull Request Checklist

Issue

Purpose

Turn homepage media list into navigation

Bonus i18n for media on homepage

Testing Instructions

References

Credit

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

sarken left a 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

Bilka2 Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarken Jul 25, 2023 • edited Loading

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

Bilka2 Jul 24, 2023 •

edited

Loading

sarken Jul 25, 2023 •

edited

Loading