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

fix: fix and test focus issues in vaadin upload #7643

Merged
merged 8 commits into from
Aug 14, 2024

Conversation

FrediWa
Copy link
Contributor

@FrediWa FrediWa commented Aug 12, 2024

Description

Keyboard navigation in the upload list was broken. This PR fixes that by:

  • focus next when a file is removed
  • focus previous when the last file is removed
  • focus the upload button when the last remaining file is removed
  • focus self on retry
  • focus upload button after upload (or really, just don't move it elsewhere)

These choices for navigation have been discussed internally. This PR focuses on keyboard navigation, possible ARIA issues are not taken into account.

I noticed that some of the tests were false positives because null === null so I added to each test that compares two objects an additional check for null. Of course, as these aren't supposed to be null, I also changed the way some of the elements are query selected.

Updated dev/upload.html to have proper files, this allows retrying upload for failed files.

Fixes #6292

Type of change

  • Bugfix

@FrediWa FrediWa changed the title Fix: fix and test focus issues in vaadin upload fix: fix and test focus issues in vaadin upload Aug 13, 2024
@FrediWa FrediWa requested a review from tomivirkki August 14, 2024 06:29
@FrediWa FrediWa marked this pull request as ready for review August 14, 2024 06:29
@FrediWa FrediWa removed the request for review from tomivirkki August 14, 2024 06:29
@FrediWa FrediWa requested review from tomivirkki and web-padawan and removed request for tomivirkki August 14, 2024 07:02
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Tested this and moving focus seems to work fine. Added a few comments about the test.

packages/upload/test/keyboard-navigation.common.js Outdated Show resolved Hide resolved
packages/upload/test/keyboard-navigation.common.js Outdated Show resolved Hide resolved
Comment on lines +45 to +46
expect(document.activeElement).to.not.equal(null);
expect(document.activeElement).to.equal(uploadButton);
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to check for null, the second check is enough.

Copy link
Member

Choose a reason for hiding this comment

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

UPD: the previous test was incorrect due to upload-button part being removed in #4857.
So we discussed that the explicit null check might be OK to keep for now.

packages/upload/test/keyboard-navigation.common.js Outdated Show resolved Hide resolved
packages/upload/test/keyboard-navigation.common.js Outdated Show resolved Hide resolved
packages/upload/test/keyboard-navigation.common.js Outdated Show resolved Hide resolved
Copy link

@web-padawan web-padawan merged commit 80ec293 into main Aug 14, 2024
9 checks passed
@web-padawan web-padawan deleted the fix/upload-list-focus branch August 14, 2024 08:25
@vaadin-bot
Copy link
Collaborator

Hi @FrediWa and @web-padawan, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 80ec293
error: could not apply 80ec293... fix: prevent losing focus on file remove or retry (#7643)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

Hi @FrediWa and @web-padawan, when i performed cherry-pick to this commit to 24.3, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 80ec293
error: could not apply 80ec293... fix: prevent losing focus on file remove or retry (#7643)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus issues in Upload
3 participants