Enable skip links on all pages by colabottles · Pull Request #10839 · forem/forem

colabottles · 2020-10-14T07:03:50Z

What type of PR is this? (check all applicable)

Description

Adds IDs to all <main> sections for skip-to-content links to work as intended for keyboard navigation.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, as well
as any relevant images for UI changes.

Added tests?

yes
no, because they aren't needed
no, because I need help

Added to documentation?

docs.forem.com
readme
no documentation needed

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

CLAassistant · 2020-10-14T07:03:57Z

All committers have signed the CLA.

rhymes · 2020-10-20T07:23:31Z

Hi @colabottles, I'm closing this as an invalid PR as it's missing a reference issue and lacks a description or context

nickytonline · 2020-10-21T02:10:46Z

Reopening as the description and related issue have been supplied.

nickytonline

This is looking great @colabottles. I have some comments and a request change. Looking forward to getting this accessibility fix in.

nickytonline · 2020-10-21T02:26:19Z

 }) => {
  return (
-    <main class="crayons-layout__content">
+    <main class="crayons-layout__content" id="articles-list">


refactor (blocking): The id fixes in conjunction with converting <div /> to <main /> tags here and all below enable skip links on all the pages where they were not present. The only thing is the id is articles-list which makes sense on the main page, but for other pages it does not.

We could probably go with a more generic id value like skip-link, or something to that effect as it makes it clear what it represents for any page the skip link is on.

I did articles-list because I wasn't sure on how you wanted to have it re: naming conventions. I can go back in and make the changes if you want to go with skip-link which makes more sense for the inside pages.

I think main-content would make a lot of sense!

nickytonline · 2020-10-29T02:22:18Z

I've restarted the buidl, but it looks like these may be valid failures in the build @colabottles. Not sure if you have access to the build logs, but they're here, https://travis-ci.com/github/forem/forem/builds/195148902.

Most, if not all seem to be related to admin page tests failing in RSpec. Here's an example:

 1) /admin/badges behaves like an InternalPolicy dependant request when user is a single_resource_admin responds with 200 OK
     Failure/Error: Unable to find matching line in /home/travis/build/forem/forem/app/views/layouts/admin.html.erb
     
     ActionView::SyntaxErrorInTemplate:
       Encountered a syntax error while rendering template: check <!DOCTYPE html>
       <html lang="en">
       <head>
         <meta charset="utf-8">
         <meta http-equiv="X-UA-Compatible" content="IE=edge">
         <meta name="viewport" content="width=device-width, initial-scale=1">
         <%# The above 3 meta tags *must* come first in the head; any other head content must come *after* these tags %>
         <meta name="description" content="">
         <meta name="author" content="">
         <%= csrf_meta_tags %>
         <%= favicon_link_tag SiteConfig.favicon_url %>
         <title><%= controller_name.titleize %></title>
     
         <!-- StimulusJS -->
         <%= javascript_packs_with_chunks_tag "admin", defer: true %>
     
         <script src="https://code.jquery.com/jquery-3.4.1.slim.min.js" integrity="sha384-J6qa4849blE2+poT4WnyKhv5vZF5SrPo0iEjwBvKU7imGFAV0wwj1yYfoRSJoZ+n" crossorigin="anonymous"></script>
     
         <!-- Bootstrap and FontAwesome -->
         <link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" integrity="sha384-wvfXpqpZZVQGK6TAh5PVlGOfQNHSoD2xbE+QkPxCAFlNEevoEH3Sl0sibVcOQVnN" crossorigin="anonymous">
         <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.4.1/css/bootstrap.min.css" integrity="sha384-Vkoo8x4CGsO3+Hhxv8T/Q5PaXtkKtu6ug5TOeNV6gBiFeWPGFN9MuhOf23Q9Ifjh" crossorigin="anonymous">
         <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-select/1.13.16/css/bootstrap-select.min.css" integrity="sha256-g19F2KOr/H58l6XdI/rhCdEK3NmB8OILHwP/QYBQ8M4=" crossorigin="anonymous" />
     
         <%= stylesheet_link_tag "admin" %>
     
       </head>
       <body class="admin default-header"
         data-honeybadger-key="<%= ApplicationConfig["HONEYBADGER_JS_API_KEY"] %>"
         data-release-footprint="<%= ApplicationConfig["RELEASE_FOOTPRINT"] %>"
         data-ga-tracking="<%= SiteConfig.ga_tracking_id %>">
         <header class="crayons-header" role="banner">
           <div class="crayons-header__container">
             <%= render "layouts/logo" %>
     
             <h1 class="pl-4 fs-l">
               <% if controller_name == "admin_portals" %>
                 Admin
               <% else %>
                 <a href="/admin">Admin</a>
                 <span class="color-base-40">&raquo;</span> <span class="fw-bold"><%= controller_name.titleize %></span>
               <% end %>
             </h1>
     
             <div class="crayons-header__links">
               <a href="https://forem.gitbook.io/forem-admin-guide/" class="crayons-btn crayons-btn--ghost-brand hidden mr-2 whitespace-nowrap s:block">
                 Forem Admin Guide
               </a>
     
               <a href="/" class="crayons-btn hidden mr-2 whitespace-nowrap s:block">
                 Go to <%= community_name %> home page
               </a>
             </div>
           </div>
         </header>
     
         <div class="crayons-layout <%= controller_name == "admin_portals" ? "" : "crayons-layout--2-cols" %>">
           <% unless controller_name == "admin_portals" %>
           <div class="crayons-layout__left-sidebar">
             <nav class="hidden m:block">
               <%= render "admin/shared/navbar" %>
             </nav>
           </div>
     
           <main class="crayons-layout__content min-w-0" id="articles-list">
             <% flash.each do |type, message| %>
               <div class="alert alert-<%= type == "notice" || type == "success" ? "success" : "danger" %>">
                 <button class="close" data-dismiss="alert" aria-label="Close">
                   <i class="fa fa-times" aria-hidden="true"></i>
                 </button>
                 <%= message %>
               </div>
             <% end %>
             <%= yield %>
           </main>
         </div>
     
         <!-- Bootstrap Dependencies -->
         <script src="https://cdn.jsdelivr.net/npm/popper.js@1.16.0/dist/umd/popper.min.js" integrity="sha384-Q6E9RHvbIyZFJoft+2mJbHaEWldlvI9IOYy5n3zV9zzTtmI3UksdQRVvoxMfooAo" crossorigin="anonymous"></script>
         <script src="https://stackpath.bootstrapcdn.com/bootstrap/4.4.1/js/bootstrap.min.js" integrity="sha384-wfSDF2E50Y2D1uUdj0O3uMBJnjuUD4Ih7YwaYd1iqfktj0Uod8GCExl3Og8ifwB6" crossorigin="anonymous"></script>
         <script src="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-select/1.13.16/js/bootstrap-select.min.js" integrity="sha256-COIM4OdXvo3jkE0/jD/QIEDe3x0jRuqHhOdGTkno3uM=" crossorigin="anonymous"></script>
     
         <%= javascript_include_tag "internal" %>
     
       </body>
       </html>
     Shared Example Group: "an InternalPolicy dependant request" called from ./spec/requests/admin/badges_spec.rb:18
     # ./app/views/layouts/admin.html.erb:88: syntax error, unexpected end-of-input, expecting `end'
     # ./app/views/layouts/admin.html.erb:88: syntax error, unexpected end-of-input, expecting `end'
     # ./app/lib/middleware/time_zone_setter.rb:10:in `call'
     # ./app/middlewares/set_cookie_domain.rb:13:in `call'
     # ./spec/requests/admin/badges_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./spec/requests/shared_examples/internal_policy_dependant_request.rb:12:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:135:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:135:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # SyntaxError:
     #   /home/travis/build/forem/forem/app/views/layouts/admin.html.erb:88: syntax error, unexpected end-of-input, expecting `end'
     #   ./app/views/layouts/admin.html.erb:88: syntax error, unexpected end-of-input, expecting `end'

nickytonline · 2020-11-06T15:26:10Z

Thanks so much for this work @colabottles!

I'm going to take this for a spin locally. The only thing left is naming the id attributes for <main /> tags. I'm going to see what makes the best sense for updating the id values as mentioned previously, the value "articles-list" doesn't really make sense.

nickytonline · 2020-11-06T15:31:52Z

Hmm, there appears to be an error in some SASS when building Storybook. Could be the things I just committed, giving it a look. https://app.netlify.com/sites/storybookdevto/deploys/5fa569d62163b30007b3ddf8

10:26:52 AM: ERR! Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
10:26:52 AM: ERR! SassError: Invalid CSS after "  }": expected selector or at-rule, was "}"
10:26:52 AM: ERR!         on line 138 of app/assets/stylesheets/components/header.scss
10:26:52 AM: ERR!         from line 20 of app/assets/stylesheets/crayons.scss
10:26:52 AM: ERR! >>   }
10:26:52 AM: ERR!
10:26:52 AM: ERR!    ---^
10:26:52 AM: ERR!
10:26:52 AM: ERR! SassError: SassError: Invalid CSS after "  }": expected selector or at-rule, was "}"
10:26:52 AM: ERR!         on line 138 of app/assets/stylesheets/components/header.scss
10:26:52 AM: ERR!         from line 20 of app/assets/stylesheets/crayons.scss
10:26:52 AM: ERR! >>   }
10:26:52 AM: ERR!
10:26:52 AM: ERR!    ---^

…sual cue.

marcysutton · 2020-11-17T20:39:29Z

One comment about focus styles, since I saw a commit about "Removed outline on skip link as the opacity change on focus is the visual cue": the focus style needs to meet contrast requirements to replace the default one. There are a lot of faint/subtle interaction styles around the site, so I wanted to give a heads up!

nickytonline · 2020-12-01T21:22:57Z

Going to work on getting the specs fixed for this.

nickytonline · 2020-12-02T01:01:28Z

As mentioned previously too, the ID will go from articles-list to main-content as the articles-list ID value does not make sense on most pages aside from the home page.

The old ID is used in JS, so I need to ensure the places it was used still works.

rhymes · 2020-12-02T10:34:45Z

@nickytonline if that's possible in the context of this refactoring, it'd be great to use a js- prefixed qualifier, this way we know that it's related to that, IIRC @msarit suggested this a while ago and I think it's an awesome idea :D

nickytonline · 2020-12-02T16:00:20Z

@nickytonline if that's possible in the context of this refactoring, it'd be great to use a js- prefixed qualifier, this way we know that it's related to that, IIRC @msarit suggested this a while ago and I think it's an awesome idea :D

You could go with the js- qualifier, but then the URL would look like this when the Skip link would be clicked on via the keyboard, e.g. https://dev.to/#js-main-content. Perhaps not a big deal, but I've never found much use for the js- qualifier except for maybe in a CSS class name, but even then I've never really bothered.

The issue about the ID is more related to ensuring all the places where it was used in JS as a selector get adjusted. In a lot of cases it's not used because the main landmarks originally had no ID.

rhymes · 2020-12-02T16:14:26Z

@nickytonline you're totally right, that would be a bad idea indeed to have js- in the URL :D

marcysutton · 2020-12-03T05:23:08Z

I tested this out locally, and found some routes that still needed a main element with matching id:

/connect
/dashboard/* (e.g. /dashboard/user_followers)
/readinglist
/{username}/{post}/manage (e.g. /marcysutton_21/5-tips-to-make-your-dog-more-fluffy-3n7g/manage)

Love seeing this come together, though! it will be really helpful for keyboard access.

nickytonline · 2020-12-03T21:55:00Z

I tested this out locally, and found some routes that still needed a main element with matching id:
* `/connect`

* `/dashboard/*` (e.g. `/dashboard/user_followers`)

* `/readinglist`

* `/{username}/{post}/manage` (e.g. `/marcysutton_21/5-tips-to-make-your-dog-more-fluffy-3n7g/manage`)
Love seeing this come together, though! it will be really helpful for keyboard access.

Don't know if you have time to do those extra <main /> elements @colabottles, but if you don't, just let me know and I will do it along with fixing tests. Thanks again for helping us with this accessibility issue!

colabottles · 2020-12-04T16:30:11Z

I tested this out locally, and found some routes that still needed a main element with matching id:
* `/connect`

* `/dashboard/*` (e.g. `/dashboard/user_followers`)

* `/readinglist`

* `/{username}/{post}/manage` (e.g. `/marcysutton_21/5-tips-to-make-your-dog-more-fluffy-3n7g/manage`)
Love seeing this come together, though! it will be really helpful for keyboard access.
Don't know if you have time to do those extra <main /> elements @colabottles, but if you don't, just let me know and I will do it along with fixing tests. Thanks again for helping us with this accessibility issue!

Unfortunately I don't have any free time today or next week @nickytonline unfortunately. Super busy pushing projects out by EOY and the talk I'm giving Saturday. Any time I can help as best I can otherwise though, just give me a shout!

Lewiscowles1986 · 2021-01-26T16:41:14Z

        <%= render "admin/shared/navbar" %>
      </nav>
    </div>
-    <% end %>


Suggested change

<% end %>

<% end %>

msarit · 2021-02-04T16:47:16Z

Hey @colabottles! 👋🏾
Checking in to see if you're still working on this PR? I know you had a bunch of responsibilities come up 😅
Let us know if you need help or have questions.

nickytonline · 2021-02-09T23:52:16Z

I spoke with Todd on Twitter DMs and he's got other things to focus on at the moment, so I'm going to add this to @forem/frontend work.

aitchiss · 2021-02-22T11:17:25Z

Closing this PR for the time being - thank you @colabottles - as @nickytonline mentioned above we're absorbing this work into our frontend team where we'll build upon your changes here in small increments 👍

Thanks again for this contribution - it's going to be a really positive change for the accessibility of Forems!

Added id of articles-list to all main tags for skip-to-content link.

b34f4de

pr-triage Bot added the PR: unreviewed bot applied label for PR's with no review label Oct 14, 2020

rhymes closed this Oct 20, 2020

rhymes added the invalid label Oct 20, 2020

nickytonline reopened this Oct 21, 2020

nickytonline suggested changes Oct 21, 2020

View reviewed changes

pr-triage Bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 21, 2020

nickytonline changed the title ~~Added id of articles-list to all main tags for skip-to-content link.~~ Enable skip links on all pages Oct 21, 2020

nickytonline removed the invalid label Oct 21, 2020

Merge branch 'master' into feature

69b395e

pr-triage Bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 28, 2020

Merge branch 'master' into feature

637398d

nickytonline reviewed Nov 6, 2020

View reviewed changes

Comment thread app/assets/stylesheets/components/header.scss Outdated

Comment thread app/assets/stylesheets/components/header.scss

Comment thread app/assets/stylesheets/components/header.scss Outdated

nickytonline added 2 commits November 6, 2020 10:20

Update app/assets/stylesheets/components/header.scss

f9bb884

Update app/assets/stylesheets/components/header.scss

8f2600d

nickytonline added 3 commits November 6, 2020 10:55

Fixed broken SASS.

1038531

Merge remote-tracking branch 'origin/master' into feature

ffc4bb6

Removed outline on skip link as the opacity change on focus is the vi…

4044db5

…sual cue.

Merge remote-tracking branch 'origin/master' into feature

1f16d02

colabottles requested a review from jacobherrington as a code owner December 1, 2020 21:27

colabottles requested a review from a team December 1, 2020 21:27

Lewiscowles1986 reviewed Jan 26, 2021

View reviewed changes

nickytonline added the internal team only internal tasks only for Forem team members label Feb 9, 2021

aitchiss mentioned this pull request Feb 15, 2021

Provide skip links: Tweak skip link style, refactor main area ID #12694

Merged

14 tasks

aitchiss closed this Feb 22, 2021

Conversation

colabottles commented Oct 14, 2020 • edited by nickytonline Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

Added to documentation?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Uh oh!

CLAassistant commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhymes commented Oct 20, 2020

Uh oh!

nickytonline commented Oct 21, 2020

Uh oh!

nickytonline left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickytonline Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

colabottles Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

marcysutton Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

nickytonline commented Oct 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickytonline commented Nov 6, 2020

Uh oh!

nickytonline commented Nov 6, 2020

Uh oh!

marcysutton commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickytonline commented Dec 1, 2020

Uh oh!

nickytonline commented Dec 2, 2020

Uh oh!

rhymes commented Dec 2, 2020

Uh oh!

nickytonline commented Dec 2, 2020

Uh oh!

rhymes commented Dec 2, 2020

Uh oh!

marcysutton commented Dec 3, 2020

Uh oh!

nickytonline commented Dec 3, 2020

Uh oh!

colabottles commented Dec 4, 2020

Uh oh!

Lewiscowles1986 Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

msarit commented Feb 4, 2021

Uh oh!

nickytonline commented Feb 9, 2021

Uh oh!

aitchiss commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

colabottles commented Oct 14, 2020 •

edited by nickytonline

Loading

CLAassistant commented Oct 14, 2020 •

edited

Loading

nickytonline left a comment •

edited

Loading

marcysutton commented Nov 17, 2020 •

edited

Loading