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

Search input keyboard focus enhancements #4282

Merged

Conversation

sporeventexplosion
Copy link
Contributor

@sporeventexplosion sporeventexplosion commented Nov 5, 2023

Search input keyboard focus enhancements

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Partially implements a feature requested in #2138

Description

Select text in search input when using Ctrl-L/Meta-L or Alt-D to focus search input.
This is the behavior of the combined address/search box of both Chrome and Firefox. I think it makes sense here because the search input in the application is essentially both an address box and a search box.

Add new Slash key shortcut to focus input without selecting text.
This does not select the text in the input, which matches the behavior of the YouTube web frontend.

Screenshots

Testing

  • On KDE Plasma, open Klipper settings GUI and enable "Synchronize contents of the clipboard and the selection"
  • Copy an arbitrary string to the clipboard
  • Open FreeTube, enter some text into the search input
  • do various combinations of pressing Slash, Ctrl-L, Alt-D, and also focusing/defocusing of search input by mouse
  • Paste clipboard contents: original string copied to keyboard is unchanged

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

Select text in search input when using Ctrl-L/Meta-L or Alt-D to focus
search input

Add new Slash key shortcut to focus input without selecting text
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 5, 2023 07:03
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 5, 2023
@sporeventexplosion
Copy link
Contributor Author

Sorry, you're right that my commit still exhibits the issue.

It appears that both Chrome and Firefox somehow work around this problem, allowing them to select text without triggering the KDE auto-copy. I have no idea how it's implemented. If it uses some low-level API that's not accessible to Electron I guess we're out of luck.

@sporeventexplosion
Copy link
Contributor Author

(note: the order of the comments is garbled. the above comments were posted during the first DST "repeated hour" for Pacific Time, which seems to have triggered a bug in GitHub's code)

@absidue
Copy link
Member

absidue commented Nov 5, 2023

FreeTube used to have the search focus, but it was removed in #2108 solve #2019, so please don't add it back. The new keyboard shortcut could be considered, but I think it is important to keep in mind that typing the slash character may be easy on a US English keyboard layout, but for many other layouts that is not the case, so maybe a different key would be more suitable.

@sporeventexplosion
Copy link
Contributor Author

After much mucking around, I found a workaround for this behavior in Chromium: make the focus+select asynchronous by wrapping it in a setTimeout(fn, 0). Now the problem is gone.

@sporeventexplosion
Copy link
Contributor Author

sporeventexplosion commented Nov 5, 2023

Would it make sense to make this an option? I don't think it makes much sense to not implement a feature that's standard in browsers just because it affects what I imagine to be a rather rare desktop environment configuration.

Also I think what is being added here is different from what was removed in #2108, as far as I can tell.

auto-merge was automatically disabled November 5, 2023 10:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 5, 2023 10:28
@sporeventexplosion
Copy link
Contributor Author

sporeventexplosion commented Nov 5, 2023

New auto-select behavior no longer causes #2019

@sporeventexplosion
Copy link
Contributor Author

@absidue Is there still any desire to merge this?

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Nov 21, 2023

I can't get Alt-D to work on MacOS. (I tested my keyboard on https://keyboardchecker.com
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/altKey
image

Oh it seems a different issue
image
No idea how to fix at the moment, non blocking for me

Slash & Meta-L work

Update 1:
From fgr-araujo/vue-shortkey#28 (comment)

On Macs, at least those with a non-US keyboard, using alt/option in keyboard shortcuts doesn't work, because Alt combinations are used to type non-ASCII characters.

In general, keyboard shortcuts using Alt only should be avoided on Mac, while shortcuts using Ctrl only should be avoided on Windows – since most combinations are in use there.

Just ignore Alt-anything on MacOS

PikachuEXE
PikachuEXE previously approved these changes Nov 21, 2023
@kommunarr
Copy link
Collaborator

kommunarr commented Nov 21, 2023

Thanks for taking the initiative on this @sporeventexplosion! Works great for Ctrl+L and Alt+D. One main issue I'm seeing: pressing / anywhere in the app will now focus the search bar, which is pretty bad when you're trying to, say, type in an Invidious instance. You should either remove this shortcut or gate it behind Alt or Ctrl/Meta.

@PikachuEXE
Copy link
Collaborator

Agree on removing slash as short cut, I can't type / anywhere in text box (focus always moved to search bar)

@sporeventexplosion
Copy link
Contributor Author

sporeventexplosion commented Nov 21, 2023

I agree, that should be separated from this PR.

auto-merge was automatically disabled November 21, 2023 09:34

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 21, 2023 09:35
auto-merge was automatically disabled November 21, 2023 09:35

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 21, 2023 09:36
@sporeventexplosion
Copy link
Contributor Author

Slash shortcut has been removed

PikachuEXE
PikachuEXE previously approved these changes Nov 21, 2023
Comment on lines 318 to 321
isFocused() {
return document.activeElement === this.$refs.input
},

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this code is now unused.

Suggested change
isFocused() {
return document.activeElement === this.$refs.input
},

auto-merge was automatically disabled November 21, 2023 21:31

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 21, 2023 21:32
Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

@FreeTubeBot FreeTubeBot merged commit bc1757e into FreeTubeApp:development Dec 9, 2023
10 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 9, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Dec 11, 2023
* development:
  Translated using Weblate (Lithuanian)
  Work around context menus in the devtools being displayed behind the window (FreeTubeApp#4264)
  Search input keyboard focus enhancements (FreeTubeApp#4282)
  Translated using Weblate (Turkish)
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.

6 participants