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: adds shift click functionality to datatable #6942

Closed
wants to merge 12 commits into from
Closed

feat: adds shift click functionality to datatable #6942

wants to merge 12 commits into from

Conversation

davidmenendez
Copy link
Contributor

Closes #4084

Adds shift click functionality to the datatable for multiple selecting and deselecting rows.

Changelog

New

  • datatable shift click functionality

Testing / Reviewing

  • normal select the first row of the table and shift select the last row of the table. all rows in the table should selected
  • then shift select the third row in the table. the first two rows should still be selected
  • then normal select the fourth row in the table and then shift select the last. the third row should be the only one unselected
  • then shift select the first row in the table. all the rows should be unselected

@davidmenendez
Copy link
Contributor Author

i didn't include any testing because i wanted to get approval on functionality before committing time to implementing tests.

@netlify
Copy link

netlify bot commented Sep 30, 2020

Deploy preview for carbon-elements ready!

Built with commit 8d9af92

https://deploy-preview-6942--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 8d9af92

https://deploy-preview-6942--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Sep 30, 2020

Deploy preview for carbon-elements ready!

Built with commit 8de51e8

https://deploy-preview-6942--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 30, 2020

Deploy preview for carbon-components-react ready!

Built with commit 8de51e8

https://deploy-preview-6942--carbon-components-react.netlify.app

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

The shift-click functionality doesn't seem to be working on Firefox latest in Windows 10.

Like mentioned in the linked issue we'll need screen reader support for this feature in VoiceOver and Jaws on their respective browsers and OSes.

@davidmenendez
Copy link
Contributor Author

davidmenendez commented Oct 4, 2020

@dakahn

The shift-click functionality doesn't seem to be working on Firefox latest in Windows 10.

i feel stupid for missing this initially because most of my prototypal work leading up to this was trying to figure out why it wasn't working in firefox to begin with. i've added a workaround. to give additional context to this workaround- it seems that shift clicking a checkbox doesn't work in firefox. it's a known bug, so there's no native support for it. it simply doesn't work. so i've removed the onClick event from the input and moved it to the label.

@joshblack
Copy link
Contributor

bump @dakahn @tw15egan when you get a sec today to take a look 👀

@dakahn
Copy link
Contributor

dakahn commented Oct 12, 2020

NVDA checkbox selection functionality seems completely broken after your last update. JAWS works as expected. Neither screen reader informs the user a batch selection option is available and batch selection doesn't work with either JAWS or NVDA in my testing (worth mentioning is that this didn't work in Chrome's History grid either 😞). @tw15egan can you check VoiceOver and see what those test results look like?

Just to reiterate, quoting from #4084 (comment)

I can click on first checkbox, scroll down e.g. to row 150, press Shift and hold, click on this 150 element, and all elements between 1-150 will be selected.

If we're going to design interactions that forego smart/better filtering options and instead rely on the user carefully selecting potentially hundreds of checkboxes in a list of potentially thousands then having this ease of use feature be something exclusive to sighted users is probably unacceptable.

@tw15egan
Copy link
Member

@dakahn I'm seeing the same thing with VoiceOver on MacOS

@davidmenendez
Copy link
Contributor Author

I'm not exactly sure what the solution is here to satisfy accessibility concerns @dakahn @tw15egan do we need to involve design?

@tw15egan
Copy link
Member

tw15egan commented Oct 12, 2020

@davidmenendez Not sure what is needed in terms of adding a11y to the new shift-click functionality, but the old voiceover functionality (selecting a checkbox with VoiceOver) is broken with these changes so that would def need to be addressed.

@dakahn
Copy link
Contributor

dakahn commented Oct 12, 2020

@davidmenendez the accessibility concerns are technical in nature. No need to involve design. To satisfy the accessibility requirements we need screen reader support for VoiceOver in Safari, JAWS 2020 on Windows 10, and NVDA.

As it stands we currently have functionality regressions. We've lost select/unselect keyboard controls in VoiceOver and NVDA.

You may need to roll back your changes until you see what specific change broke Voiceover/NVDA. After that a good place to start would be to investigate how this feature is implemented from an accessibility standpoint in other web based data tables/grids. Then hopefully reverse engineer those solutions.

@davidmenendez
Copy link
Contributor Author

ok. thanks for the feedback. i've never worked with these tools before, so i'll have to check those out. I'm assuming the reason it's breaking those tools is because in order to get the shift click to work in firefox requires changing the onClick handler from the actual input to the label.

i'll continue to work on this and seek additional help here if I have any questions about these tools.

@tw15egan
Copy link
Member

@davidmenendez voiceover on Mac is pretty easy to work with, just need to triple tap the power / touchID button to enable it. Thanks for taking the time to work on this, it's much appreciated! 🙏 Feel free to reach out with any questions

@davidmenendez
Copy link
Contributor Author

still working on addressing accessibility issues

@davidmenendez
Copy link
Contributor Author

The more I continue to work on this the more I'm just not sure that this can be accomplished within the set guidelines. The main problem being how labels and check boxes interact with each other natively in different browsers. The biggest hurdle being that Firefox doesn't support shift clicking a checkbox. If you attempt to shift click any native checkbox it don't fire any kind of event. My original solution to move any click events to a label or outside div has proven to be ineffective because it's not accessible. Using VoiceOver to toggle a checkbox triggers the onClick event of the checkbox, but that doesn't work in Firefox.

I'll try to continue to hack this, but I'm not sure if this is feasible at the moment. I can't even find another major component library that supports this functionality and I suspect it's because they also figured out it's not worth the hassle to attempt to rewrite how check boxes and labels interact with each other. Not even Material UI has this implemented.

If anyone has any ideas I'd love to hear them.

@janhassel
Copy link
Member

janhassel commented Nov 26, 2020

This is an incredibly important feature for many users and we constantly hear from them. It's definitely a tricky topic due to the cross-browser support and accessibility considerations but let's see how we can move forward from here. @tw15egan @dakahn, I'd love your feedback on the following options/paths:

1. Change how Carbon checkboxes are rendered
Right now, the actual input element of a checkbox is made invisible and the styling is done with pseudo-elements on the label element. If instead the input element would receive the styling, shift+clicking on it would be supported by Firefox, just shift+clicking the label would not result in a selection.
Alternative: only do this with checkboxes in data tables

2. Replace <input type="checkbox"> with <div role="checkbox">
HTML elements with a checkbox role work fine with shift+click in Firefox. The selection logic would need to be implemented on component side. This is actually how Google Mail has done it: they support contiguous checkbox selection with shift+click and use divs under the hood.
Alternative: only do this with checkboxes in data tables

3. Implement this feature without Firefox support
Since the normal checkbox behaviour is not affected by this and it is solely an addition, the experience for Firefox users would not be degraded. The other browsers Carbon supports would all work with this change. We could hope that Firefox adresses this 11yr old bug (https://bugzilla.mozilla.org/show_bug.cgi?id=559506) at one point.

@joshblack
Copy link
Contributor

@janhassel just wanted to follow up with this issue, approach 2 seems very intriguing especially with the precedence in GMail. If I remember correctly, Data Table already uses InlineCheckbox instead of the Checkbox component so there is precedence for this approach.

Do you know if there is a minimal example/prototype of this approach that we could take a look at?

@davidmenendez
Copy link
Contributor Author

@joshblack i'll give this a go. I don't think from a technical standpoint that it should be too difficult to offer an option in the existing checkbox component to give a div rendered option or perhaps i'd just start with using this custom checkbox in this component to start and if it works i can open an additional PR to port it over to the standard checkbox component.

My main concern has just been i don't know if it'll be satisfactory from an accessibility point of view. but i'll start working on this locally and see what I can come up with and update my PR accordingly.

@dakahn
Copy link
Contributor

dakahn commented Feb 26, 2021

@davidmenendez did the work @joshblack mentioned get implemented?

Base automatically changed from master to main March 8, 2021 16:35
@davidmenendez
Copy link
Contributor Author

@tw15egan @dakahn @joshblack i was able to switch the input to a div and i was able to use voiceover to control the multi selection. i think i was doing the voiceover controls correctly, though i'm cretainly no expert. the last commit i added removed the checkbox states like readOnly and disabled, but assuming the functionality is acceptable i'll work to add those back in in a follow up commit.

@davidmenendez
Copy link
Contributor Author

so at this point i am looking for confirmation that the current functionality is or is not acceptable in case i wasn't clear about that in my last comment. thanks!

@joshblack
Copy link
Contributor

@davidmenendez sorry for the delay! That's awesome to see 🥳

@dakahn @tw15egan do you have time this week to go over the changes?

@dakahn
Copy link
Contributor

dakahn commented Mar 22, 2021

Circling back on this.

Checked using Firefox latest on Windows 10 with NVDA and JAWS 2020 on Chrome latest on Windows 10

Checkboxs on the the Selection variant of Datatable are broken for both Windows readers. Can't use spacebar to make selection. Worth noting is that keyboard controls without a screen reader work as intended for non-batch selection, but I'm unsure how to operate batch selection with only a keyboard.

For reference for testers (probably stated somewhere above): keyboard controls for batch actions are

  1. with checkbox focused press space to select checkbox
  2. press tab to navigate to last intended selection in batch
  3. press shift + tab to select all checkboxes between first and last intended selection

@joshblack
Copy link
Contributor

@tw15egan was suggesting that we add this to our next sprint (start date April 5th) to build off of what you've worked on @davidmenendez and bring this to GA, does that sound good to everyone? 👀

@emyarod
Copy link
Member

emyarod commented Apr 7, 2021

not sure if I was meant to be added to this issue for sprint 7 but can we confirm that the screen reader regressions aren't caused by the commented code in InlineCheckbox? building @davidmenendez's branch locally and reenabling the commented code along with restoring the input element in the inline checkbox seems to resolve keyboard navigation issues on macOS (including VO) and Windows (excluding screen readers since I do not have access to those)

a good place to start would be to investigate how this feature is implemented from an accessibility standpoint in other web based data tables/grids

do we have any updates on investigations related to this? until we have an established pattern to follow or a set of requirements to implement, this is kind of in limbo. it's also worth noting that keyboard shift selection is not supported in Gmail so it may be a complete nonissue and this PR would be good to go once the commented code is reenabled

@emyarod emyarod requested a review from dakahn April 8, 2021 14:42
@dakahn
Copy link
Contributor

dakahn commented Apr 9, 2021

Looks like it didn't get added to the current sprint -- so I assume it's also not being investigated. I'll comment on our sprint issue and we can get it in the next one 👍🏽

@dakahn dakahn removed their request for review April 9, 2021 20:43
@tw15egan tw15egan removed their request for review August 30, 2021 16:44
@davidmenendez
Copy link
Contributor Author

hey! it's been a while. i completely forgot about this. where are we with this?

@joshblack
Copy link
Contributor

@davidmenendez unfortunately no updates on our end for this 😞 . Looking back, I think the work stalled around figuring out the correct behavior and then was not picked up again.

As of right now, this is not being addressed. The earliest we're going to begin delivering new work into stable will be Q2 2022 after our v11 release which would be the soonest we could revisit this. Sorry that I couldn't offer better news about this, I can't imagine how frustrating trying to get functionality like this landed has been.

@davidmenendez
Copy link
Contributor Author

no problem @joshblack i know you're all very busy. if there's anything i can do to help please let me know.

@jnm2377
Copy link
Contributor

jnm2377 commented Mar 15, 2022

I'm closing this PR for now. We can track any more discussions in this issue: #10995

@jnm2377 jnm2377 closed this Mar 15, 2022
@github-actions
Copy link
Contributor

DCO Assistant Lite bot: Thanks for your submission! We ask that you all sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


I have read the DCO document and I hereby sign the DCO.


1 out of 2 committers have signed the DCO.
@tw15egan
@david Menendez
David Menendez seems not to be a GitHub user. You need a GitHub account to be able to sign the DCO. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

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.

[DataTable] with selection, missing possibility to select multiple subsequent rows with Shift key
7 participants