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

[24.0] Fix bugs relating to history keyboard navigation #17587

Merged

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Mar 4, 2024

Adds cross platform Ctrl/Cmd(Metakey) compatibility for features added in #17502

Also adds a minor Shift+Click select feature (on top of the existing Shift+Arrowkey feature):

shift_click_select.mp4

Here, the user first selects individual items using Ctrl+Click, and selects batches of items using Shift+Ctrl+Click, to be able to select batch items while maintaining prior selections.
Then, they select items using only Shift+Click, which restricts the selection to the initial item and the range.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. Select an initial item using Ctrl/Shift + Click
    2. Then Shift+Click another item above or below the initial one to select the ones in the range
    3. Also, try out Shift+Ctrl+Click to select items in multiple batches

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Users can use arrows keys to go to the next/prev item.
Tab key will instead help navigate through the `ContentOptions` (and other item actions if expanded)
Users can select a batch of items starting from one focused item, until the next `Shift+Click` item.
If the user uses a combination of `Shift+Ctrl+Click`, the remainder of the selection persists and is not reset.
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review March 4, 2024 20:35
@ahmedhamidawan ahmedhamidawan changed the base branch from dev to release_24.0 March 4, 2024 22:42
@ahmedhamidawan ahmedhamidawan changed the title Fix bugs relating to history keyboard navigation [24.0] Fix bugs relating to history keyboard navigation Mar 4, 2024
Copy link
Member

@martenson martenson left a comment

Choose a reason for hiding this comment

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

It works well, thanks @ahmedhamidawan

As a fairly needed followup to this feature I think we should take care to enable click-to-select on a larger area of the history items when tags are present. See below what portion of the item is clickable, the rest is not and it feels weird.

Screenshot 2024-03-04 at 5 14 43 PM

client/src/components/History/Content/ContentItem.vue Outdated Show resolved Hide resolved
client/src/components/History/Content/ContentItem.vue Outdated Show resolved Hide resolved
@ahmedhamidawan
Copy link
Member Author

Yes, very good point, doing this as part of the same PR

@dannon
Copy link
Member

dannon commented Mar 5, 2024

Haven't done any tinkering to actually try it, but it might be worth looking into https://vueuse.org/core/useMagicKeys/#usemagickeys w/ https://vueuse.org/core/useKeyModifier/#usekeymodifier to simplify this in a standard non-platform-specific way. And we already use vueuse.

@@ -12,7 +12,6 @@ const props = defineProps({
isVisible: { type: Boolean, default: true },
state: { type: String, default: "" },
itemUrls: { type: Object, required: true },
keyboardSelectable: { type: Boolean, default: true },
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ContentOptions buttons are always Keyboard selectable now, with the Tab key. To go from one item to another, you can use the arrow keys, the tab key will take you from ContentItem -> ContentOptions -> ContentItem ....

@martenson
Copy link
Member

@dannon Likely a decent standard for the key detection. However the logic here goes beyond different keyboards, because the standard macos behavior for selecting is command+click despite having the ctrl key. I did not see that being handled in the lib you linked. Did I miss it?

@dannon
Copy link
Member

dannon commented Mar 5, 2024

Good point -- might not be worth caring about OS, and just have a simpler modifierPressed be the cross-os union of ctrl/cmd/meta/whatever?

Is there a situation where that would actually be problematic? Would it just be something like I guess on a windows machine you get win+c copying as well, which is fine?

@ahmedhamidawan
Copy link
Member Author

Good point -- might not be worth caring about OS, and just have a simpler modifierPressed be the union of ctrl/cmd/meta/whatever?

Is there a situation where that would actually be problematic? Would it just be something like I guess on a windows machine you get win+c copying as well, which is fine?

Only thing is, you wouldn't want to prevent default behavior for the Windows key

For Mac, it'd probably work out fine for Control+Click or Cmd-Click doing the same thing. It would feel weird if on Windows, Ctrl-Click and Windows-Click were the same though

@dannon
Copy link
Member

dannon commented Mar 5, 2024

Sure -- does windows+click have a predefined behavior? Ctrl+click opens in a new tab, so we're already kind of overwriting that I guess.

@ahmedhamidawan
Copy link
Member Author

Sure -- does windows+click have a predefined behavior? Ctrl+click opens in a new tab, so we're already kind of overwriting that I guess.

This would only be the behavior when you ctrl click on the area which typically expands the dataset/ opens the collection, but I see your point. Maybe then that's what I'll do here, treat the Meta and Ctrl/Control key the same way (for ContentItems)

@martenson
Copy link
Member

ctrl+click on macos is simulating the right click 🫣

@ahmedhamidawan
Copy link
Member Author

So maybe then

function isSelectKey(event: KeyboardEvent) {
    const isMac = navigator.userAgent.indexOf("Mac") >= 0;
    return isMac ? event.metaKey : event.ctrlKey;
}

is an alright way of using the appropriate key for this? Since, for the purposes here, Ctrl-Click or Ctrl-A for Windows, and Cmd-Click or Cmd-A for Mac are default behaviors that users are used to across apps?

@dannon
Copy link
Member

dannon commented Mar 5, 2024

@ahmedhamidawan Yeah, I think that's fine. I didn't realize control+click on mac was also a right click; we shouldn't mask that.

@martenson martenson merged commit bc78b6f into galaxyproject:release_24.0 Mar 6, 2024
26 of 27 checks passed
@martenson
Copy link
Member

thanks @ahmedhamidawan!
I extracted the followup to #17620

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

Successfully merging this pull request may close these issues.

4 participants