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

Make reorder buttons keyboard accessible #3372

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

AndreaBarbasso
Copy link
Contributor

References

Description

Added keyboard controls for drag and drop functionality on submission forms. The functionality follows the indications found here.

Instructions for Reviewers

This PR can be tested by going in any submission form with repeatable fields.
Tabbing until the first drag element, an Aria assertive message is added to the live region, giving the user some instructions on how to reorder elements.
Elements can be reordered by pressing spacebar (activating the "drag" action), moving the item with the arrow keys, and then dropping the item again with the spacebar.
The Aria assertive message is always updated with info on the element being dragged and its current position.

List of changes in this PR:

  • Added keyboard controls to drag and drop elements;
  • Added Aria messages to the Live region.

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@AndreaBarbasso AndreaBarbasso marked this pull request as ready for review September 30, 2024 16:03
@tdonohue tdonohue added bug accessibility high priority funded Task is funded via the DSpace Development Fund labels Sep 30, 2024
Copy link

github-actions bot commented Nov 1, 2024

Hi @AndreaBarbasso,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@tdonohue
Copy link
Member

@AndreaBarbasso : Could you resolve the merge conflict in this PR, so that we might consider this for inclusion in 8.1 / 7.6.3? Thanks

# Conflicts:
#	src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.html
#	src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts
#	src/assets/i18n/en.json5
@AndreaBarbasso
Copy link
Contributor Author

@tdonohue, conflicts have been resolved!

@tdonohue tdonohue requested review from artlowel and tdonohue January 23, 2025 15:44
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @AndreaBarbasso!

The code looks good, although you are missing some tests according to the coverage check

It works well for the most part. However, if you press escape after selecting something and moving it, I'd expect the selected row to jump back to the place it started. Currently it doesn't. Pressing escape after moving does the same thing as pressing spacebar.

I'd also always show the handles. It's a bit strange now that they pop into existence if you tab to them. It's even stranger that if you press the arrow keys when you have a handle selected without pressing space first, it looks like you're reordering but you're not. Always showing the handles would also solve that issue

In general I think it would be a good idea to make it more obvious when something is selected. I'd give the entire form-row a background color rather than changing the font-style of the text inside the input (I'd leave that untouched)

@AndreaBarbasso
Copy link
Contributor Author

AndreaBarbasso commented Jan 28, 2025

@artlowel, I implemented the changes you requested, the last one (using a background-color instead of italicizing text) has been done by applying such background-color to the input instead of the entire form-row for aesthetic reasons.

@tdonohue tdonohue requested a review from artlowel January 29, 2025 16:03
Copy link

Hi @AndreaBarbasso,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@AndreaBarbasso : This looks great overall & works. Overall, I'm +1

However, along with the spacebar, I'd like us to add the "Enter" key as another way to select the row. That way this reorder feature works the same as the reordering of Bitstreams on the "Edit Item > Bitstreams" screen (see #3464). (I initially tried to test this by pressing "Enter" and was surprised when I couldn't get it to work. I then realized that it only responds to spacebar)

This PR also has a very minor code conflict in en.json5 that needs to be resolved before merger.

@AndreaBarbasso
Copy link
Contributor Author

@tdonohue, I updated the PR! Merge conflicts have been resolved, and now the drag and drop can be toggled with the Enter key (in addition to the spacebar).

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @AndreaBarbasso!

@tdonohue tdonohue self-requested a review January 30, 2025 14:47
@tdonohue tdonohue added port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility bug funded Task is funded via the DSpace Development Fund high priority port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release
Projects
Status: 👍 Reviewer Approved
Development

Successfully merging this pull request may close these issues.

[Deque Analysis] Reorder buttons in Submission forms are not keyboard accessible (Critical Issue)
3 participants