Accessibility settings page #3613
Conversation
…-main # Conflicts: # src/app/core/auth/auth.service.ts # src/app/info/info-routing-paths.ts # src/app/info/info-routing.module.ts # src/app/info/info.module.ts # src/app/shared/live-region/live-region.service.spec.ts # src/app/shared/live-region/live-region.service.ts # src/app/shared/notifications/notifications-board/notifications-board.component.spec.ts # src/app/shared/notifications/notifications-board/notifications-board.component.ts # src/config/app-config.interface.ts # src/config/default-app-config.ts
AAwouters
force-pushed
the
accessibility-settings-main
branch
from
12fa618 to
3422ec7
Compare
|
Hi @AAwouters. Thank you for this PR. Overall, it looks really good. I tested it locally, and I'm having a problem with the Accessibility Settings form, both as an anonymous user and when I'm logged in. It happens on Firefox and Chrome. When I change the "Hide notifications automatically" setting to off and click "Save accessibility settings" the green "Successfully saved settings..." notification doesn't go away until I click on the 'X'. That isn't a big problem, but once I change that setting then I can never change it back to on. Every time I do the setting returns to off, even though the green "Successfully saved settings" notification appears. However, I am able to change the Notification and Live region timeouts successfully. The green "Successfully saved settings..." notification appears and disappears on its own. Also, clicking on "Reset accessibility settings" works well. And the green "Successfully reset settings" notification goes away after a few seconds. So I think the problem is only with the option to hide notifications automatically. 
|
…-main # Conflicts: # src/app/accessibility/accessibility-settings.service.spec.ts
…s-main # Conflicts: # src/app/profile-page/profile-page.component.html # src/app/profile-page/profile-page.component.ts # src/app/shared/notifications/notifications-board/notifications-board.component.spec.ts # src/app/shared/notifications/notifications-board/notifications-board.component.ts # src/themes/custom/app/profile-page/profile-page.component.ts
Yes. When you disable automatic notification hiding, notifications won't hide/'go away' automatically. Maybe I misunderstood you but is that not exactly the expected behavior?
Right. That was a small oversight on my part. It should be fixed now. |
There was a problem hiding this comment.
@AAwouters : Thanks for this PR. Overall, the feature works, but I've run into a few bugs and areas for improvement.
- If I open the "Accessibility settings", leave the defaults and click "Save accessibility settings" button, I see an error in my Browser's DevTools console. This same error also appears if you try to change the time outs while anonymous.
TypeError: Cannot read properties of undefined (reading 'cookieExpirationDuration')
- I'm running the UI in production mode:
npm run build:prod; npm run serve:ssr - Do not login (stay anonymous)
- Go to the "Accessibility Settings" page
- Click the "Save Accessibility settings" button immediately. Or try to change one of the two timeouts (without touching the toggle).
- In your browser's DevTools console, you'll see the error.
-
Both of the time-outs can be set to a negative number, which doesn't make sense. I'd recommend we only allow a minimum of 1 or zero.
-
If you open the Accessibility Settings and immediately click the up or down arrow next to the default time outs, you'll jump to -1 or 1. This is odd, especially for the "Live region time out", which starts at 30, but if you click the "up" arrow on the field, it goes to 1.
-
I'm not a fan of the current display. I think we should change the labels to say "Notification time out (in seconds)" and "ARIA live region time out (in seconds)" as those are more accessible. Having the word "Seconds" appear after the input field will not make sense to screen readers. I also have a recommendation for renaming the "Hide notifications automatically" label (see comments inline below)
I also have some general suggestions inline below. Most of it is just text change suggestions.
src/app/info/accessibility-settings/accessibility-settings.component.html
Show resolved
Hide resolved
src/app/info/accessibility-settings/accessibility-settings.component.html
Show resolved
Hide resolved
|
@tdonohue Heads up: while I agree with your feedback. I should warn you that we're already quite a ways above the 100 hours that were the combined estimate on the original tickets (#1191, #1270 and #3154). Among other things because a separate accessibility settings page wasn't planned when we started. We're currently at 172 hours, and still have to start on the Orejime integration |
…-main # Conflicts: # src/app/accessibility/accessibility-settings.service.spec.ts # src/app/accessibility/accessibility-settings.service.ts # src/app/info/accessibility-settings/accessibility-settings.component.spec.ts # src/app/info/accessibility-settings/accessibility-settings.component.ts # src/app/shared/cookies/klaro-configuration.ts # src/assets/i18n/en.json5
|
@artlowel : Thanks for letting us know. Please also try to remember to ping @hostle83 on any updates of estimates. Could we have a fresh estimate then for how much time it anticipated for this entire effort? I know @hostle83 is trying to keep track of how much money is left in our fund for fixing accessibility issues. So, a new total estimate would be helpful for fund tracking purposes. |
|
I've processed your feedback @tdonohue .
I was not able to reproduce this issue.
I initially wanted to avoid validation as it's a fairly simple form, but I agree that being able to enter negative values does not make sense.
The reason for this is that those numbers were placeholders based on the default values and not actual values in the form. The reason I used those placeholders instead of filling the form with the default values is because the behavior is slightly different for notifications.
This means that when the default is pre-filled in the form and the user saves the form without making changes, the user might have changed the behavior without realising. With the validation, empty fields are no longer possible so now the fields are filled with the default values (if there are no user-configured values yet). e2e testsWith the addition of a new (optional) cookie, there are some issues with the e2e tests. For the Keep in mind that when logged in a different cookie is used based on the userId so three cookies are necessary: one for anonymous users, one for the admin user and one for the submitter user. |
There was a problem hiding this comment.
@AAwouters : Thanks for the updates. I think this looks much better and is working well. I just have a few minor suggestions inline on how to cleanup the e2e tests (and fix those for 7.x and 8.x) and a few other minor things.
UPDATE: I've also noticed I'm getting an SSR error when I login now which seems related to the new Orejime Accessibility settings. Here's how to reproduce it:
- First, I'm running in production mode:
npm run build:prod; npm run serve:ssr. Keep an eye on the commandline where you runnpm run serve:ssras this is where the SSR error will apper. - Now, if you login, you'll see an SSR error similar to this on the commandline:
GET /reload/1740776418071?redirect=%2Finfo%2Faccessibility 200 465.058 ms - -
ERROR Error [NullInjectorError]: R3InjectorError(Standalone[RootComponent2])[LiveRegionService2 -> LiveRegionService2 -> AccessibilitySettingsService2 -> OrejimeService2 -> OrejimeService2]:
NullInjectorError: No provider for OrejimeService2!
Once these minor things are cleaned up, I'd be +1 this PR. Thanks!
| @@ -1,6 +1,9 @@ | |||
| beforeEach(() => { | |||
| cy.visit('/collections/create?parent='.concat(Cypress.env('DSPACE_TEST_COMMUNITY'))); | |||
| cy.loginViaForm(Cypress.env('DSPACE_TEST_ADMIN_USER'), Cypress.env('DSPACE_TEST_ADMIN_PASSWORD')); | |||
|
|
|||
| // Accept all cookies to make sure cookie window does not block other elements | |||
| cy.get('.orejime-Button--save').click(); | |||
There was a problem hiding this comment.
You should be able to remove all these clicks that you've added to Orejime if you just update the default orejime-anonymous cookie defined here: https://github.com/DSpace/dspace-angular/blob/main/cypress/support/e2e.ts#L59
The purpose of adding that Cookie before every test is to ensure the Orejime window doesn't popup at all. If that cookie is set with all the proper settings, then the Orejime window shouldn't appear.
My guess is that since you added a new accessibility section to Orejime, you may need to add it to the Orejime cookie as well.
This same approach should also work for 7.x and 8.x branches, except they use Klaro instead and therefore define a default Klaro cookie in that same e2e.ts file for Cypress.
|
|
||
| "info.accessibility-settings.liveRegionTimeOut.label": "ARIA Live region time out (in seconds)", | ||
|
|
||
| "info.accessibility-settings.liveRegionTimeOut.hint": "The duration after which a message in the ARIA live region disappears. ARIA live regions are not visible on the page, but proivde announcements of notifications (or other actions) to screen readers.", |
There was a problem hiding this comment.
Small typo here that I just noticed. In the second sentence "proivde" should be "provide".
| @@ -80,6 +80,10 @@ <h5 class="text-uppercase">Footer Content</h5> | |||
| <a class="btn text-white" | |||
| routerLink="info/feedback">{{ 'footer.link.feedback' | translate}}</a> | |||
| </li> | |||
| <li> | |||
| <a class="text-white" | |||
| routerLink="info/accessibility">{{ 'footer.link.accessibility' | translate }}</a> | |||
There was a problem hiding this comment.
I think we should move this link next to the "Cookie Settings" in order to keep the two settings together. So, I'd propose the footer list these links in this ordering...
"Cookie settings" "Accessibility settings" "Privacy Policy" "End User Agreement", "Send Feedback"
References
Fixes point 3 of #1191: "Content visually appears and disappears with no ability to adjust timing."
Description
This PR adds the 'Accessibility settings' page where a user can configure accessibility settings. This page can be found either through the 'Accessibility settings' link in the footer, or through the link on the profile page. The settings are stored in a cookie when the user is not authorized and are stored in a metadata field (see backend PR) when the user is authenticated.
Instructions for Reviewers
Access the page through one of the mentioned links, or directly at
/info/accessibility. Currently only two settings are available: notification time-out and live region time-out. Functionality of the notification time-out is the easiest to verify. Set a new value and click the 'Save accessibility settings` button. A notification will appear mentioning whether the changes are saved locally (cookie) or on the user profile (metadata) and this notification will stay as long as configured.Live region time-out can be verified by making the live region visible. (see #3337)
List of changes in this PR:
accessibility-settingscomponent and servicenotifications-boardwill have the user-configured time-out (if set by the user).Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.