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

feat(NcActionInput): allow to prevent open on hover #5014

Closed
wants to merge 1 commit into from

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Dec 31, 2023

☑️ Resolves

  • Allow to prevent opening the datepicker and select components in NcActionInput on hover.

While working on nextcloud/tasks#2427 I realized that opening the NcSelect in NcActionInput on hover is quite disturbing, as it is unexpected and in many cases also undesired. E.g. when clicking a NcActionButton below the NcActionInput, the open NcActionInput blocks the view. Also, the select is open by default if it is the first element in the NcActions menu. This also looks weird. Furthermore, all other usages of NcDateTimePicker and NcSelect do not open on hover.

This PR adds a prop openOnHover that allows to disable this behaviour. By default it is still enabled to not introduce a breaking change.

🖼️ Screenshots

🏚️ Before:

Bildschirmaufzeichnung.vom.2024-01-01.10-38-37.webm

🏡 After:

Bildschirmaufzeichnung.vom.2024-01-01.10-33-30.webm

@raimund-schluessler raimund-schluessler added enhancement New feature or request 3. to review Waiting for reviews feature: datepicker Related to the date/time picker component feature: actions Related to the actions components feature: select Related to the NcSelect* components labels Dec 31, 2023
@raimund-schluessler raimund-schluessler added this to the 8.5.0 milestone Dec 31, 2023
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

sure!

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

What's your feedback regarding accessibility @JuliaKirschenheuter @ShGKme? I would imagine the fix is good, but just to be sure. :)

@ShGKme
Copy link
Contributor

ShGKme commented Jan 3, 2024

What's your feedback regarding accessibility @JuliaKirschenheuter @ShGKme? I would imagine the fix is good, but just to be sure. :)

I'd say that should be the default and the only behavior.

It is especially not comfortable for people with mobility disabilities. A shaking hand, cerebral palsy or just a broken wrist. Or you a using a laptop in a car while driving on a bad road.

Example scenario. User:

  1. Opens a menu
  2. Moves the cursor to an input element
  3. Starts typing or only want to start typing and leaves the mouse to use keyboard
  4. Due to some mobility disability, the cursor is moved
  5. Focus is lost, catch it again. If a user doesn't look at the screen while typing, they may even not notice that during the typing.

Also, changing focus on hover triggers more actions on the screen reader (it is pronounced). And it could be more complex cognitively.

Example scenario. User:

  1. Opens a menu
  2. Moves to the first item "Label text"
  3. The label is read in loud: Label text
  4. Focus moves automatically: Label text edit Entered text Label text Label text
  5. Cursor moves on the input
  6. Input id read again: Label text

Many things are pronounced. UI state changes just on hover. Focus moves unexpectedly (there was no interaction, no clicks, no keyboard).


From my personal user perspective, I also don't see the benefits of focusing on input elements or opening selects/pickers on mouse move. This is not an expected behavior from my user experience.

@raimund-schluessler
Copy link
Contributor Author

@ShGKme I agree with your analysis. But what is the conclusion? Should we remove focusing by mouse completely then? This is something the NcActions component does, see here:

// MENU KEYS & FOCUS MANAGEMENT
// focus nearest focusable item on mouse move
// DO NOT change the focus if the target is already focused
// this will prevent issues with input being unfocused
// on mouse move
onMouseFocusAction(event) {
if (document.activeElement === event.target) {
return
}
const menuItem = event.target.closest('li')
if (menuItem && this.$refs.menu.contains(menuItem)) {
const focusableItem = menuItem.querySelector(focusableSelector)
if (focusableItem) {
const focusList = this.$refs.menu.querySelectorAll(focusableSelector)
const focusIndex = [...focusList].indexOf(focusableItem)
if (focusIndex > -1) {
this.focusIndex = focusIndex
this.focusAction()
}
}
}
},

Hovering over an action item, be it a button, input, link etc. focuses it, pressing enter then executes the action.

@ShGKme
Copy link
Contributor

ShGKme commented Jan 4, 2024

But what is the conclusion? Should we remove focusing by mouse completely then?

From my PoV - yes, focusing any input elements should be removed.

But we cannot just remove this method, because it is required to activate elements on hover...

And we cannot remove .focusable from input-s to save keyboard navigation with arrows...

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Jan 5, 2024

This sounds like we should not focus the input, but should also not not focus the input 🙈

Btw. Keyboard navigation with NcActionInput of type multiselect or date is anyway completely broken at the moment.

@ShGKme
Copy link
Contributor

ShGKme commented Jan 5, 2024

This sounds like we should not focus the input, but should also not not focus the input 🙈

Focus on keyboard navigation, not focus on mouse

Btw. Keyboard navigation with NcActionInput of type multiselect or date is anyway completely broken at the moment.

*crying*

Thanks for noticing, it should also be fixed...

@raimund-schluessler raimund-schluessler added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Jan 5, 2024
@raimund-schluessler raimund-schluessler marked this pull request as draft January 5, 2024 21:47
@raimund-schluessler raimund-schluessler added bug Something isn't working and removed enhancement New feature or request labels Jan 5, 2024
@Antreesy Antreesy modified the milestones: 8.15.0, 8.15.1 Jul 23, 2024
@susnux susnux modified the milestones: 8.15.1, 8.15.2, 8.16.0 Jul 29, 2024
@Antreesy Antreesy modified the milestones: 8.16.0, 8.16.1 Aug 5, 2024
@susnux susnux modified the milestones: 8.16.1, 8.17.0, 8.17.1 Aug 20, 2024
@susnux susnux modified the milestones: 8.17.1, 8.18.0 Aug 30, 2024
@Antreesy Antreesy modified the milestones: 8.18.0, 8.18.1 Sep 12, 2024
@susnux susnux modified the milestones: 8.18.1, 8.19.0, 8.20.0, 8.19.1 Sep 17, 2024
@Antreesy Antreesy modified the milestones: 8.19.1, 8.20.1 Oct 29, 2024
@susnux susnux modified the milestones: 8.21.1, 8.23.0 Jan 15, 2025
@raimund-schluessler
Copy link
Contributor Author

Superseded by #6475.

@raimund-schluessler raimund-schluessler deleted the feat/noid/prevent-focus-on-hover branch January 31, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of bug Something isn't working feature: actions Related to the actions components feature: datepicker Related to the date/time picker component feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants