Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♿ [#2379] Search feedback notification for screenreaders #1280

Merged
merged 3 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/open_inwoner/cms/cases/tests/test_htmx.py
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ def mock_upload(request, context):
upload_form = page.locator("#document-upload")
file_input = upload_form.get_by_label("Sleep of selecteer bestanden")
submit_button = upload_form.get_by_role("button", name=_("Upload documenten"))
notification_list = page.get_by_role("alert").get_by_role("list")
notification_list = page.locator(".notification").get_by_role("list")
notification_list_items = notification_list.get_by_role("listitem")
file_list = page.get_by_role("list").last
file_list_items = file_list.get_by_role("listitem")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<div class="notifications notifications__errors">
<div class="notification notification--warning">
{% icon icon="warning_amber" icon_position="before" outlined=True %}
<div class="notification__content">
<div class="notification__content" role="alert" tabindex="-1">
<p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-compact">{{ message.message }}</p>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
{% load i18n utils button_tags icon_tags button_tags icon_tags %}
<div class="notification{% if contents %} notification--contents{% endif %}{% if type %} notification--{{ type }}{% endif %}{% if compact %} notification--compact{% endif %} {% if ctx %}notification--{{ ctx }}{% endif %}" role="alert">
<div class="notification{% if contents %} notification--contents{% endif %}{% if type %} notification--{{ type }}{% endif %}{% if compact %} notification--compact{% endif %} {% if ctx %}notification--{{ ctx }}{% endif %}">
{% if not icon == False %}
<div class="notification__icon">
{% icon icon outlined=True %}
</div>
{% endif %}

<div class="notification__content">
<div class="notification__content"
{% if type == "error" %}
role="alert"
{% elif type == "warning" %}
role="alert"
{% else %}
role="status"
{% endif %} tabindex="-1">
{% if title %}<h2 class="utrecht-heading-2">{{ title }}</h2>{% endif %}
{% if notification %}<p class="utrecht-paragraph">{{ notification }}</p>{% endif %}
{% if action %}{% button href=action text=action_text %}{% endif %}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% load notification_tags string_tags %}

<div class="notifications">
<section class="notifications">
{% for message in messages %}
{% with as_markdown=message.extra_tags|is_substring:"as_markdown" %}
{% with local_message=message.extra_tags|is_substring:"local_message" %}
Expand All @@ -14,4 +14,4 @@
{% endwith %}
{% endwith %}
{% endfor %}
</div>
</section>
4 changes: 2 additions & 2 deletions src/open_inwoner/conf/locale/nl/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -6552,9 +6552,9 @@ msgstr "Zoekindex opnieuw opbouwen"
#: open_inwoner/search/tests/test_feedback.py:192
#: open_inwoner/search/views.py:158
msgid ""
"Thank you for your feedback. It will help us to improve our search engine"
"Thank you for your feedback, it will help us improve our search engine."
msgstr ""
"Dank u voor uw feedback, hiermee kunnen wij de omgeving verder verbeteren"
"Dank u voor uw feedback, hiermee kunnen wij de omgeving verder verbeteren."

#: open_inwoner/search/tests/test_logging.py:33 open_inwoner/search/views.py:63
#, python-brace-format
Expand Down
2 changes: 2 additions & 0 deletions src/open_inwoner/js/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import './header'
import './map'
import './message-file'
import { Notification } from './notifications'
import { NotificationsList } from './notifications/NotificationsList'
import './plans'
import './plan-preview'
import './questionnaire'
Expand Down Expand Up @@ -55,6 +56,7 @@ const elementWrappers = [
(elt) => new DisableContactFormButton(elt),
],
[Notification.selector, (elt) => new Notification(elt)],
[NotificationsList.selector, (elt) => new NotificationsList(elt)],
[AnchorMobile.selector, (elt) => new AnchorMobile(elt)],
[StatusAccordion.selector, (elt) => new StatusAccordion(elt)],
[FileInput.selector, (elt) => new FileInput(elt)],
Expand Down
39 changes: 39 additions & 0 deletions src/open_inwoner/js/components/notifications/NotificationsList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* Surrounding Notifications class.
* @class
*/
export class NotificationsList {
static selector = '.notifications'
constructor(notificationContents) {
this.notificationContents = notificationContents
}

scrollToFirstNotification() {
if (this.notificationContents.length > 0) {
// Scroll to the first notification, since there could be multiple
this.notificationContents[0].scrollIntoView({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notificationContents behaves like a stack, so this will srcoll into the last notification (because it was the last one added). If you want to start with the first notification, you need to loop in reverse (so -1 instead of 0).

I don't know what's the intended (or best) behavior, though. If you want to focus on the last notification, you can ignore this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pi-sigma Actually no. If you would manually add multiple templates for notifications by hand (after doing Git pull and rebuild NPM), you can see the reverse is not needed.

The code correctly targets the first notification content by using this.notificationContents[0].
There is no need to loop in reverse or use a different index. The code already achieves the intended behavior of scrolling to and focusing on the first notification, because I am "re-indexing the Nodelist" here.

If you want to visually see which 1 element truly has focus after PageReload, you can adjust/remove the _Notification.SCSS in this PR where I removed the browsers default focus-visible style.

You can test this manually this way: paste in multiple types of notifications in the wrong order, after the last line of the Notification.html template, so you can see the re-ordering and focus is happening correctly and generate these notifications by using the "Feedback versturen" button after doing a Search query:

<div class="notification notification--success ">
   <div class="notification__icon">
      <span aria-hidden="true" class="material-icons-outlined ">check_circle</span>
   </div>
   <div class="notification__content" role="status" tabindex="-1">
      <p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-compact">Success success</p>
   </div>
   <button class="button notification__close button--textless button--icon button--transparent" type="" name="" value="" title="Sluit deze melding" aria-label="Sluit deze melding">
   <span aria-hidden="true" class="material-icons ">close</span>
   </button>
</div>
<div class="notification notification--info ">
   <div class="notification__icon">
      <span aria-hidden="true" class="material-icons-outlined ">check_circle</span>
   </div>
   <div class="notification__content" role="status" tabindex="-1">
      <p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-compact">Info blue info</p>
   </div>
   <button class="button notification__close button--textless button--icon button--transparent" type="" name="" value="" title="Sluit deze melding" aria-label="Sluit deze melding">
   <span aria-hidden="true" class="material-icons ">close</span>
   </button>
</div>
<div class="notification notification--error ">
   <div class="notification__icon">
      <span aria-hidden="true" class="material-icons-outlined ">check_circle</span>
   </div>
   <div class="notification__content" role="status" tabindex="-1">
      <p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-compact">Error error</p>
   </div>
   <button class="button notification__close button--textless button--icon button--transparent" type="" name="" value="" title="Sluit deze melding" aria-label="Sluit deze melding">
   <span aria-hidden="true" class="material-icons ">close</span>
   </button>
</div>
<div class="notification notification--warning ">
   <div class="notification__icon">
      <span aria-hidden="true" class="material-icons-outlined ">check_circle</span>
   </div>
   <div class="notification__content" role="status" tabindex="-1">
      <p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-compact">Warning warning</p>
   </div>
   <button class="button notification__close button--textless button--icon button--transparent" type="" name="" value="" title="Sluit deze melding" aria-label="Sluit deze melding">
   <span aria-hidden="true" class="material-icons ">close</span>
   </button>
</div>

block: 'center',
behavior: 'smooth',
})

// Add a delay before setting focus for screen readers
setTimeout(() => {
// Set focus on the first notification content
this.notificationContents[0].focus()
jiromaykin marked this conversation as resolved.
Show resolved Hide resolved
}, 100)
}
}
}

// Start!

const notificationContents = document.querySelectorAll('.notification__content')

// Instantiate notifications section
document
.querySelectorAll(NotificationsList.selector)
.forEach((notifications) => new NotificationsList(notifications))

// Focus only the first notification content
const scrollManager = new NotificationsList(notificationContents)
scrollManager.scrollToFirstNotification()
87 changes: 77 additions & 10 deletions src/open_inwoner/js/components/notifications/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
const typeOrder = ['error', 'warning', 'success', 'info']

/**
* Helper function to determine the order index of a notification type.
* @param {HTMLElement} notification - The notification element.
* @returns {number} - Order index of the notification type.
*/
const getTypeOrderIndex = (notification) => {
const type = getTypeFromNotification(notification)
return typeOrder.indexOf(type)
}

/**
* Notification class.
* Helper function to get the type of a notification.
* @param {HTMLElement} notification - The notification element.
* @returns {string} - Type of the notification.
*/
const getTypeFromNotification = (notification) => {
let notificationType = ''
notification.classList.forEach((cls) => {
if (cls.startsWith('notification--')) {
notificationType = cls.replace('notification--', '')
}
})
return notificationType
}

/**
* Single Notification class.
* @class
*/
export class Notification {
Expand All @@ -14,6 +41,7 @@ export class Notification {
this.node = node

this.bindEvents()
this.reorderNotifications()
}

/**
Expand All @@ -25,16 +53,25 @@ export class Notification {
}

/**
* Scrolls to the notification content.
* Scrolls to the notification content and sets focus.
*/
scrollToNotification() {
const notificationContent = document.querySelector('.notification__content')
const notificationContents = Array.from(
this.node.querySelectorAll('.notification__content')
)

if (notificationContents) {
notificationContents.forEach((content) => {
// If errors are present, scroll and trigger the opened state
jiromaykin marked this conversation as resolved.
Show resolved Hide resolved
content.scrollIntoView({
block: 'center',
behavior: 'smooth',
})

if (notificationContent) {
// If errors are present, scroll and trigger the opened state
notificationContent.scrollIntoView({
block: 'center',
behavior: 'smooth',
// Add a pause before setting focus for screen readers after DOM load
setTimeout(() => {
content.focus()
}, 100)
})
}
}
Expand All @@ -47,8 +84,6 @@ export class Notification {
e.preventDefault()
this.close()
})

this.scrollToNotification()
}

/**
Expand All @@ -57,6 +92,29 @@ export class Notification {
close() {
this.node.parentElement.removeChild(this.node)
}

/**
* Reorders notifications based on type.
*/
reorderNotifications() {
// Get all notifications in the parent container
const notificationsContainer = document.querySelector('.notifications')
const notifications = Array.from(
notificationsContainer.querySelectorAll(Notification.selector)
)

// Sort notifications based on type order
notifications.sort((a, b) => {
const typeA = getTypeOrderIndex(a)
const typeB = getTypeOrderIndex(b)
return typeA - typeB
})

// Re-append sorted notifications to parent container
notifications.forEach((notification) =>
notificationsContainer.appendChild(notification)
)
}
}

// Start!
Expand All @@ -65,3 +123,12 @@ export class Notification {
document
.querySelectorAll(Notification.selector)
.forEach((notification) => new Notification(notification))

// Scroll to the notifications after reordering
setTimeout(() => {
const firstNotification = document.querySelector(Notification.selector)
if (firstNotification) {
const instance = new Notification(firstNotification)
instance.scrollToNotification()
}
}, 0)
6 changes: 6 additions & 0 deletions src/open_inwoner/scss/components/Grid/Grid.scss
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@
}
}
}

/// Search grid
&--search {
display: flex;
flex-direction: column;
}
}

&--limit &__sidebar,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@
& &__content {
margin-top: var(--spacing-tiny);

&:focus,
&:focus-visible {
outline: none;
border: none;
}

& * {
margin: 0;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
top: var(--spacing-extra-large);
display: flex;
flex-direction: column;
gap: var(--spacing-extra-large);
gap: var(--spacing-medium);
z-index: 1002;

/// Multiple errors.
Expand Down
2 changes: 1 addition & 1 deletion src/open_inwoner/search/tests/test_feedback.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,6 @@ def test_feedback_form_not_displayed_after_submit(self):
self.assertEqual(
message.message,
_(
"Thank you for your feedback. It will help us to improve our search engine"
"Thank you for your feedback, it will help us improve our search engine."
),
)
2 changes: 1 addition & 1 deletion src/open_inwoner/search/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def form_valid(self, form):
self.request,
messages.SUCCESS,
_(
"Thank you for your feedback. It will help us to improve our search engine"
"Thank you for your feedback, it will help us improve our search engine."
),
)
redirect = furl(reverse("search:search"))
Expand Down
2 changes: 1 addition & 1 deletion src/open_inwoner/templates/pages/search.html
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ <h2 class="utrecht-heading-2">{% trans "Zoekfilters" %}</h2>
{# end search filters #}
</aside>

<div class="grid__main">
<div class="grid__main grid__main--search">
{% if paginator.count %}
<div class="search-results">
<h2 class="utrecht-heading-2 search-results__title">
Expand Down
Loading