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 arrow keys not working in the Create New Playlist prompt #5243

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

absidue
Copy link
Member

@absidue absidue commented Jun 9, 2024

Fix arrow keys not working in the Create New Playlist prompt

Pull Request Type

  • Bugfix

Related issue

closes #5232

Description

This pull request addresses two issues with using the arrow keys in the Create New Playlist prompt, firstly that you weren't able to move the cursor around in the text box and secondly that you weren't able to move the focus between the create and cancel buttons.
The first issue was solved by making sure we only detect arrow keys for navigating between the buttons, when the focus is actually on a button.
The second issue was solved by changing how the prompt decides which buttons are eligible for the arrow key treatment, previously it only detected the buttons if they were direct descendents of an ft-flex-box and had an id that started with prompt, now it will detect all ft-buttons inside of the prompt.

Testing

Text box

  1. Enter some text into the text box
  2. Try moving the cursor with the left and right arrow keys

Buttons

  1. Focus the create or cancel buttons by pressing Tab
  2. Try moving the focus between the two buttons by using the left and right arrow keys

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: bf3304d

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 9, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 9, 2024 21:12
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

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

Arrow keys not working on create or cancel buttons on the 2nd add to playlist prompt showing in the vid. There is also an error thats popping up.

VirtualBoxVM_YOwuYAcf80.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 10, 2024
@absidue
Copy link
Member Author

absidue commented Jun 10, 2024

Fixed it, I had to tell the portal that multiple prompts are allowed to get teleported there at the same time, without that extra attribute it broke the rendering of the second prompt.

@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jun 10, 2024
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

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

PR reintroduces behavior that was addressed before by @jasonhenriquez in a previous PR. I can now use tab to go outside the add to playlist prompt

before:

FreeTube_IKvAY2alUs.mp4

This PR:

VirtualBoxVM_uYeomZv3vU.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jun 10, 2024
@absidue
Copy link
Member Author

absidue commented Jun 11, 2024

None of FreeTube's code was written to deal with more than one prompt being open at the same time, as that only happens in that one place in the app and only since playlists were added. So all the bugs you are noticing are caused by that.

This pull request didn't actually break Jason's changes, as it never disabled interaction on the prompt in the background. It was just a lucky side-effect of the broken rendering (yes broken, it made the prompt component forget that it was connected to it's HTML elements), that was caused by trying to teleport two items to a portal only meant to receive one item.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 11, 2024

If I look at the before recording I see that the Add to playlist prompt is removed when entering the Create new playlist prompt and that makes me believe that its the reason for being unable to navigate outside the Create new playlist prompt

Edit: re-read your comment. So you're saying that this behavior im describing was a lucky side-effect?

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 13, 2024

@jasonhenriquez do you have suggestions on solving this issue?

@kommunarr
Copy link
Collaborator

You can either make the other prompt inert when the new prompt is opened, or have it such that it's one prompt replacing its content rather than having two open. The first option seems far easier

@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jun 18, 2024
@FreeTubeBot FreeTubeBot merged commit 877a644 into FreeTubeApp:development Jun 19, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 19, 2024
@absidue absidue deleted the prompt-arrow-keys branch June 19, 2024 04:07
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jun 20, 2024
* development: (120 commits)
  Update version number to v0.21.0
  Remove limited donation methods (FreeTubeApp#5290)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese (Portugal))
  Avoid cloning all profiles when subscribing and unsubscribing (FreeTubeApp#5289)
  Fix arrow keys not working in the Create New Playlist prompt (FreeTubeApp#5243)
  Bump ws from 8.16.0 to 8.17.1 (FreeTubeApp#5291)
  Remove a few bits of unused code (FreeTubeApp#5287)
  Update About page to display correct Freetube logo based on currently set theme (FreeTubeApp#5126)
  Translated using Weblate (Croatian)
  * Update playlist page titles (FreeTubeApp#5271)
  Update Invidious instances list (FreeTubeApp#5288)
  Translated using Weblate (Polish)
  Bump the eslint group with 2 updates (FreeTubeApp#5275)
  Bump marked from 12.0.2 to 13.0.0 (FreeTubeApp#5276)
  Bump sass from 1.77.4 to 1.77.5 (FreeTubeApp#5277)
  Bump lefthook from 1.6.15 to 1.6.16 (FreeTubeApp#5279)
  Bump webpack from 5.91.0 to 5.92.0 (FreeTubeApp#5278)
  Update Flatpak PR Workflow to work with updated module (FreeTubeApp#5270)
  Translated using Weblate (Portuguese)
  ...
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jun 20, 2024
* development: (102 commits)
  Update version number to v0.21.0
  Remove limited donation methods (FreeTubeApp#5290)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese (Portugal))
  Avoid cloning all profiles when subscribing and unsubscribing (FreeTubeApp#5289)
  Fix arrow keys not working in the Create New Playlist prompt (FreeTubeApp#5243)
  Bump ws from 8.16.0 to 8.17.1 (FreeTubeApp#5291)
  Remove a few bits of unused code (FreeTubeApp#5287)
  Update About page to display correct Freetube logo based on currently set theme (FreeTubeApp#5126)
  Translated using Weblate (Croatian)
  * Update playlist page titles (FreeTubeApp#5271)
  Update Invidious instances list (FreeTubeApp#5288)
  Translated using Weblate (Polish)
  Bump the eslint group with 2 updates (FreeTubeApp#5275)
  Bump marked from 12.0.2 to 13.0.0 (FreeTubeApp#5276)
  Bump sass from 1.77.4 to 1.77.5 (FreeTubeApp#5277)
  Bump lefthook from 1.6.15 to 1.6.16 (FreeTubeApp#5279)
  Bump webpack from 5.91.0 to 5.92.0 (FreeTubeApp#5278)
  Update Flatpak PR Workflow to work with updated module (FreeTubeApp#5270)
  Translated using Weblate (Portuguese)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Unable to navigate using the arrow keys in the New Playlist Name prompt
5 participants