♿️(course glimpse) put the <a> only around media and title

this makes for a better UX for screen reader/keyboard users. The idea is to have a big clickable zone for mouse users with the link on the media cover + on the title, with the link on the title having a bigger clickable space than shown. We manually add/remove the card shadow when the link is hovered/focused
openfun · Mar 21, 2022 · ba43269 · ba43269
1 parent 54b6ec9
commit ba43269
Show file tree

Hide file tree

Showing 10 changed files with 142 additions and 68 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,9 +10,9 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Added
 
-- Add a `PaymentButton` component 
+- Add a `PaymentButton` component
 - Add a `PaymentInterface` component to lazy load the right payment component
-  according to the provider used 
+  according to the provider used
 - Create a `StepBreadcrumb` component to display progress within a step process
 - Create a `useStepManager` hook to manage step process
 - Create a React `Icon` component that can optionally take alternative text
@@ -35,6 +35,8 @@ Versioning](https://semver.org/spec/v2.0.0.html).
   description with every markup removed
 - Set font size to 1rem on some detail pages contents: Organization
   excerpt, Program body and Person main content
+- Change how course glimpse anchor is structured (allows text selection
+  in the course glimpse + better screen reader user experience)
 
 ### Fixed
 

diff --git a/src/frontend/js/components/CourseGlimpse/index.spec.tsx b/src/frontend/js/components/CourseGlimpse/index.spec.tsx
@@ -61,7 +61,8 @@ describe('components/CourseGlimpse', () => {
     expect(container.textContent?.indexOf('Course 42')).toBe(0);
 
     // The link that wraps the course glimpse should have no title as its content is explicit enough
-    expect(screen.getByRole('link')).not.toHaveAttribute('title');
+    const link = container.querySelector('.course-glimpse__link');
+    expect(link).not.toHaveAttribute('title');
     // The course glimpse shows the relevant information
     screen.getByRole('heading', { name: 'Course 42', level: 3 });
     screen.getByLabelText('Course code');
@@ -76,7 +77,10 @@ describe('components/CourseGlimpse', () => {
     // Check course logo
     const courseGlipseMedia = container.getElementsByClassName('course-glimpse__media');
     expect(courseGlipseMedia.length).toBe(1);
-    const img = courseGlipseMedia[0].firstChild;
+    expect(courseGlipseMedia[0]).toHaveAttribute('aria-hidden', 'true');
+    const mediaLink = courseGlipseMedia[0].firstChild;
+    expect(mediaLink).toHaveAttribute('tabindex', '-1');
+    const img = mediaLink?.firstChild;
     expect(img).toBeInstanceOf(HTMLImageElement);
     // The logo is rendered along with alt text "" as it is decorative and included in a link block
     expect(img).toHaveAttribute('alt', '');

diff --git a/src/frontend/js/components/CourseGlimpse/index.tsx b/src/frontend/js/components/CourseGlimpse/index.tsx
@@ -1,4 +1,4 @@
-import { memo } from 'react';
+import React, { memo } from 'react';
 import { defineMessages, FormattedMessage, useIntl } from 'react-intl';
 
 import { CommonDataProps } from 'types/commonDataProps';
@@ -36,26 +36,32 @@ const messages = defineMessages({
 const CourseGlimpseBase = ({ context, course }: CourseGlimpseProps & CommonDataProps) => {
   const intl = useIntl();
   return (
-    <a className="course-glimpse course-glimpse--link" href={course.absolute_url}>
-      <div className="course-glimpse__media">
-        {/* alt forced to empty string because it's a decorative image */}
-        {course.cover_image ? (
-          <img
-            alt=""
-            sizes={course.cover_image.sizes}
-            src={course.cover_image.src}
-            srcSet={course.cover_image.srcset}
-          />
-        ) : (
-          <div className="course-glimpse__media__empty">
-            <FormattedMessage {...messages.cover} />
-          </div>
-        )}
+    <div className="course-glimpse">
+      {/* the media link is only here for mouse users, so hide it for keyboard/screen reader users.
+      Keyboard/sr will focus the link on the title */}
+      <div aria-hidden="true" className="course-glimpse__media">
+        <a tabIndex={-1} href={course.absolute_url}>
+          {/* alt forced to empty string because it's a decorative image */}
+          {course.cover_image ? (
+            <img
+              alt=""
+              sizes={course.cover_image.sizes}
+              src={course.cover_image.src}
+              srcSet={course.cover_image.srcset}
+            />
+          ) : (
+            <div className="course-glimpse__media__empty">
+              <FormattedMessage {...messages.cover} />
+            </div>
+          )}
+        </a>
       </div>
       <div className="course-glimpse__content">
         <div className="course-glimpse__wrapper">
           <h3 className="course-glimpse__title" title={course.title}>
-            {course.title}
+            <a className="course-glimpse__link" href={course.absolute_url}>
+              <span className="course-glimpse__title-text">{course.title}</span>
+            </a>
           </h3>
           {course.organization_highlighted_cover_image ? (
             <div className="course-glimpse__organization-logo">
@@ -99,7 +105,7 @@ const CourseGlimpseBase = ({ context, course }: CourseGlimpseProps & CommonDataP
         ) : null}
         <CourseGlimpseFooter context={context} course={course} />
       </div>
-    </a>
+    </div>
   );
 };
 

diff --git a/src/frontend/scss/objects/_course_glimpses.scss b/src/frontend/scss/objects/_course_glimpses.scss
@@ -68,8 +68,19 @@ $course-glimpse-content-padding-sides: 0.7rem !default;
     @include sv-flex(1, 0, calc(25% - #{$r-course-glimpse-gutter * 2}));
   }
 
+  // We want to trigger the card shadow on hovering links inside the card,
+  // but not when hovering non-interactive text.
+  // we handle this via these pointer-events rules
+  pointer-events: none;
+
+  a,
+  button,
+  .icon[aria-label] {
+    pointer-events: auto;
+  }
+
   &:hover,
-  &:focus {
+  &:focus-within {
     color: inherit;
     text-decoration: none;
     border: 0;
@@ -119,6 +130,7 @@ $course-glimpse-content-padding-sides: 0.7rem !default;
     border-bottom-left-radius: 0;
     box-shadow: r-theme-val(course-glimpse, icon-shadow);
     z-index: 1;
+    pointer-events: none;
 
     .category-badge {
       padding: 0;
@@ -147,17 +159,56 @@ $course-glimpse-content-padding-sides: 0.7rem !default;
 
   &__title {
     @include font-size($h6-font-size);
-    -webkit-box-orient: vertical;
-    -webkit-line-clamp: 3;
     color: r-theme-val(course-glimpse, title-color);
-    display: block;
-    display: -webkit-box;
     font-family: $r-font-family-montserrat;
     font-weight: $font-weight-boldest;
     flex: 1 0 1.3em * 3; // 3 lines;
     line-height: 1.3em;
     margin-bottom: 1rem;
+  }
+
+  &__link {
+    color: inherit;
+    -webkit-box-orient: vertical;
+    -webkit-line-clamp: 3;
+    display: block;
+    display: -webkit-box;
     overflow: hidden;
+
+    // we extend the clickable zone of the link,
+    // so that the clickable zone actually sticks to the edges of the card
+    padding: 1.7rem $course-glimpse-content-padding-sides 1rem;
+    margin: -1.7rem #{$course-glimpse-content-padding-sides * -1} -1rem;
+
+    &:hover,
+    &:focus {
+      text-decoration: none;
+      color: inherit;
+      outline: 0;
+    }
+  }
+
+  &__link:focus &__title-text {
+    outline: 1px dotted;
+    outline: 1px auto -webkit-focus-ring-color;
+    outline-offset: 1px;
+  }
+
+  // we use focus-visible to prevent the outline on click, but we need to support safari
+  // that doesn't support focus-visible quite yet
+  @supports selector(:focus-visible) {
+    &__link:focus &__title-text {
+      outline: 0;
+    }
+    &__link:focus-visible &__title-text {
+      outline: 1px dotted;
+      outline: 1px auto -webkit-focus-ring-color;
+      outline-offset: 1px;
+    }
+  }
+
+  &__title-text {
+    display: block;
   }
 
   &__metadata {
@@ -205,6 +256,7 @@ $course-glimpse-content-padding-sides: 0.7rem !default;
     overflow: hidden;
     padding: 0.25rem;
     position: absolute;
+    pointer-events: none;
     right: 0;
     top: -3.53rem;
     width: 4.3rem;

diff --git a/src/richie/apps/courses/templates/courses/cms/fragment_course_glimpse.html b/src/richie/apps/courses/templates/courses/cms/fragment_course_glimpse.html
@@ -2,27 +2,33 @@
 {% comment %}Obviously, the context template variable "course" is required and must be a Course page extension{% endcomment %}
 
 {% with course_page=course.extended_object course_state=course.state main_organization_title=course.get_main_organization.extended_object.get_menu_title main_organization=course.get_main_organization course_variant=course_variant|default:'glimpse' %}
-<a class="course-{{ course_variant }}{% if course_page.publisher_is_draft is True %} course-{{ course_variant }}--draft{% endif %}" href="{{ course_page.get_absolute_url }}">
-    <div class="course-{{ course_variant }}__media">
-        {% get_placeholder_plugins "course_cover" course_page as cover_plugins or %}
-            <p class="course-{{ course_variant }}--empty">{% trans "Cover" %}</p>
-        {% endget_placeholder_plugins %}
-        {% blockplugin cover_plugins.0 %}
-            <img src="{% thumbnail instance.picture 300x170 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %}"
-                srcset="
-                    {% thumbnail instance.picture 800x457 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 300w
-                    {% if instance.picture.width >= 1600 %},{% thumbnail instance.picture 1600x914 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 600w{% endif %}
-                    {% if instance.picture.width >= 2400 %},{% thumbnail instance.picture 2400x1371 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 900w{% endif %}
-                "
-                sizes="300px"
-                {# alt forced to empty string for a11y because the image does not carry more information than the course title #}
-                alt=""
-            />
-        {% endblockplugin %}
+<div class="course-{{ course_variant }}{% if course_page.publisher_is_draft is True %} course-{{ course_variant }}--draft{% endif %}">
+    <div aria-hidden="true" class="course-{{ course_variant }}__media">
+        <a tabindex="-1" href="{{ course_page.get_absolute_url }}">
+            {% get_placeholder_plugins "course_cover" course_page as cover_plugins or %}
+                <p class="course-{{ course_variant }}--empty">{% trans "Cover" %}</p>
+            {% endget_placeholder_plugins %}
+            {% blockplugin cover_plugins.0 %}
+                <img src="{% thumbnail instance.picture 300x170 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %}"
+                    srcset="
+                        {% thumbnail instance.picture 800x457 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 300w
+                        {% if instance.picture.width >= 1600 %},{% thumbnail instance.picture 1600x914 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 600w{% endif %}
+                        {% if instance.picture.width >= 2400 %},{% thumbnail instance.picture 2400x1371 replace_alpha='#FFFFFF' crop upscale subject_location=instance.picture.subject_location %} 900w{% endif %}
+                    "
+                    sizes="300px"
+                    {# alt forced to empty string for a11y because the image does not carry more information than the course title #}
+                    alt=""
+                />
+            {% endblockplugin %}
+        </a>
     </div>
     <div class="course-{{ course_variant }}__content">
         <div class="course-{{ course_variant }}__wrapper">
-            <h{{ header_level|default:3 }} class="course-{{ course_variant }}__title" title="{{ course_page.get_title }}">{{ course_page.get_title }}</h{{ header_level|default:3 }}>
+            <h{{ header_level|default:3 }} class="course-{{ course_variant }}__title" title="{{ course_page.get_title }}">
+                <a class="course-{{ course_variant }}__link" href="{{ course_page.get_absolute_url }}">
+                    <span class="course-{{ course_variant }}__title-text">{{ course_page.get_title }}</span>
+                </a>
+            </h{{ header_level|default:3 }}>
             {% if main_organization %}
                 {% with organization_page=main_organization.extended_object %}
                     {% get_placeholder_plugins "logo" organization_page as plugins or %}
@@ -85,6 +91,6 @@
         </div>
         {% endblock course_glimpse_footer %}
     </div>
-</a>
+</div>
 {% endwith %}
 {% endspaceless %}
diff --git a/tests/apps/courses/test_cms_plugins_course.py b/tests/apps/courses/test_cms_plugins_course.py
@@ -74,7 +74,7 @@ def test_cms_plugins_course_render_on_public_page(self):
 
         # Create a course with a page in both english and french
         organization = OrganizationFactory(
-            page_title="public title",
+            page_title="organization public title",
             should_publish=True,
             fill_logo={
                 "original_filename": "org_logo.jpg",
@@ -83,7 +83,7 @@ def test_cms_plugins_course_render_on_public_page(self):
         )
 
         course = CourseFactory(
-            page_title={"en": "public title", "fr": "titre public"},
+            page_title={"en": "course public title", "fr": "titre public cours"},
             fill_organizations=[organization],
             fill_icons=[icon_category_main, icon_category_secondary],
             fill_cover={
@@ -111,15 +111,15 @@ def test_cms_plugins_course_render_on_public_page(self):
 
         # The course's url should be present
         self.assertIn(
-            '<a class="course-glimpse" href="/en/public-title/"',
+            '<a class="course-glimpse__link" href="/en/course-public-title/"',
             re.sub(" +", " ", str(response.content).replace("\\n", "")),
         )
 
         # The course's name should be present
         course_title = course_page.get_title()
         self.assertContains(
             response,
-            f'<h2 class="course-glimpse__title" title="{course_title:s}">{course_title:s}</h2>',
+            f'<span class="course-glimpse__title-text">{course_title:s}</span',
             status_code=200,
         )
         # The course's main organization should be present
@@ -141,7 +141,8 @@ def test_cms_plugins_course_render_on_public_page(self):
 
         # The course's cover should be present
         pattern = (
-            r'<div class="course-glimpse__media">'
+            r'<div aria-hidden="true" class="course-glimpse__media">'
+            r'<a tabindex="-1" href="/en/course-public-title/">'
             r'<img src="/media/filer_public_thumbnails/filer_public/.*cover\.jpg__300x170'
             r'.*alt=""'
         )
@@ -170,15 +171,15 @@ def test_cms_plugins_course_render_on_public_page(self):
 
         # The course's url should be present
         self.assertIn(
-            '<a class="course-glimpse" href="/fr/titre-public/"',
+            '<a class="course-glimpse__link" href="/fr/titre-public-cours/"',
             re.sub(" +", " ", str(response.content).replace("\\n", "")),
         )
 
         # The course's name should be present
         course_title = course_page.get_title()
         self.assertContains(
             response,
-            f'<h2 class="course-glimpse__title" title="{course_title:s}">{course_title:s}</h2>',
+            f'<span class="course-glimpse__title-text">{course_title:s}</span>',
             status_code=200,
         )
 
@@ -209,7 +210,8 @@ def test_cms_plugins_course_render_on_public_page(self):
 
         # The course's cover should be present
         pattern = (
-            r'<div class="course-glimpse__media">'
+            r'<div aria-hidden="true" class="course-glimpse__media">'
+            r'<a tabindex="-1" href="/fr/titre-public-cours/">'
             r'<img src="/media/filer_public_thumbnails/filer_public/.*cover\.jpg__300x170'
             r'.*alt=""'
         )
@@ -334,20 +336,21 @@ def test_cms_plugins_course_fallback_when_never_published(self):
         # The course path in french should be in the url but the locale in the
         # url should remain "en"
         self.assertIn(
-            '<a class="course-glimpse" href="/en/cours-public/"',
+            '<a class="course-glimpse__link" href="/en/cours-public/"',
             re.sub(" +", " ", str(response.content).replace("\\n", "")),
         )
 
         # The course's name should be present
         self.assertIn(
-            '<h2 class="course-glimpse__title" title="cours public">cours public</h2>',
+            '<span class="course-glimpse__title-text">cours public</span>',
             re.sub(" +", " ", str(response.content).replace("\\n", "")),
         )
         self.assertNotContains(response, "public course")
 
         # The course's cover should be present
         pattern = (
-            r'<div class="course-glimpse__media">'
+            r'<div aria-hidden="true" class="course-glimpse__media">'
+            r'<a tabindex="-1" href="/en/cours-public/">'
             r'<img src="/media/filer_public_thumbnails/filer_public/.*cover\.jpg__300x170'
             r'.*alt=""'
         )

diff --git a/tests/apps/courses/test_templates_category_detail.py b/tests/apps/courses/test_templates_category_detail.py
@@ -197,7 +197,7 @@ def test_templates_category_detail_cms_published_content_courses(self):
         self._extension_cms_published_content(
             CourseFactory,
             "course_categories",
-            '<h2 class="course-glimpse__title" title="{0:s}">{0:s}</h2>',
+            '<span class="course-glimpse__title-text">{0:s}</span>',
         )
 
     @mock.patch(
@@ -407,7 +407,7 @@ def test_templates_category_detail_cms_draft_content_organizations(self):
     def test_templates_category_detail_cms_draft_content_courses(self):
         """Validate how a draft category page is displayed with its related courses."""
         self._extension_cms_draft_content(
-            CourseFactory, '<h2 class="course-glimpse__title" title="{0:s}">{0:s}</h2>'
+            CourseFactory, '<span class="course-glimpse__title-text">{0:s}</span>'
         )
 
     def test_templates_category_detail_cms_draft_content_blogposts(self):

diff --git a/tests/apps/courses/test_templates_organization_detail.py b/tests/apps/courses/test_templates_organization_detail.py
@@ -257,7 +257,7 @@ def test_templates_organization_detail_cms_published_content(self):
         self.assertContains(
             response,
             # pylint: disable=consider-using-f-string
-            '<h2 class="course-glimpse__title" title="{0:s}">{0:s}</h2>'.format(
+            '<span class="course-glimpse__title-text">{0:s}</span>'.format(
                 published_course.public_extension.extended_object.get_title(),
             ),
             html=True,
@@ -400,7 +400,7 @@ def test_templates_organization_detail_cms_draft_content(self):
         # The published course should be on the page in its draft version
         self.assertContains(
             response,
-            '<h2 class="course-glimpse__title" title="modified course">modified course</h2>',
+            '<span class="course-glimpse__title-text">modified course</span>',
             html=True,
         )