feat(DataList): add draggable feature for basic list

[kmcfaul]

What: Closes #4708

Basic implementation of draggable. DataLists with nested state dependencies (like expandable) I am having trouble getting to work, because the whole list needs to be saved in the state in order to update, but you cannot access state variables inside of itself.

@mcoker The draggable ghost defaults to a copy of the whole row, and I haven't found a great way to change this yet, but it at least seems to preserve the spacing. The entire row is also draggable by default instead of just the chip button (with mouse), but it doesn't interfere with other actions so I'm not sure this is an issue.

For keyboard, hitting space or enter on a drag chip button begins the drag. Use the up arrow or down arrow to move the row (showing the ghost placeholder) and enter to confirm/finish the drag. Escape will cancel the drag, or hitting any other key (like Tab to continue tabbing through the DOM).

[patternfly-build]

PF4 preview: https://patternfly-react-pr-4852.surge.sh
A11y report: https://patternfly-react-pr-4852-coverage.surge.sh
A11y report: https://patternfly-react-pr-4852-a11y.surge.sh

[kmcfaul]

Re: changes in DataListCheck.

The pf-c-data-list__item-control div was built into the DataListCheck component, but now with the drag button, multiple components may reside under the control div. To prevent this being a breaking change, I edited DataListCheck to have a new property flag which will prevent the control div from wrapping the check, and added a new DataListItemControl component.

So it's not breaking currently, but a flag will have to be passed to DataListCheck to get the right HTML structure. Later on we can remove the flag and remove the control div from the check component.

[mcarrano]

@kmcfaul This is looking good. Just 2 problems that I'm seeing. On Firefox, there seems to be some problem with dropping the item. It sometimes is staying in the disabled state after I release the mouse. Have you tested on Firefox? Also, the cursor behavior is a bit flaky. As I drag the item, the cursor may update as I'm moving over other items below it.

[kmcfaul]

@mcarrano Firefox just wanted the onDrop handler in addition to or instead of dragEnd, so it should work properly once the build finishes.

As far as the pointer, I would guess it is because the dragging doesn't interfere with css below it. I'm not sure if there's anything I can do about that, but I'll look into it. Any thoughts @mcoker ?

[jessiehuff]

Nice! This is looking great so far! The keyboard interaction works pretty well (though I wonder if we should include enter as one of the activation key presses like this demo), but I think there's some a11y updates that we should add to make it accessible for screen readers. Jenn and I hashed out all of the details in the core PR starting here, but essentially I think a structure like the core example would be great, potentially including the keyboard interaction instructions above the example as they did as well:

Some notable points from it:

• More a11y on the button: aria-label with "Reorder" telling users what they can do, aria-pressed with true/false depending on whether the button is selected, aria-labelledby the item, aria-describedby with the description text of how to use the reorder button.
• aria-live="assertive" as a div on the page that announces what is happening for users with the screen reader class.
• I also notice that the checkboxes are labelled by an id that doesn't exist? It looks like the id of the list item is "simple-item1" instead of "check-item1".
  (I think making these changes should also fix the axe violations happening. 🙂)

[mcoker]

@kmcfaul when I test on Firefox, the row I'm dragging is really small. Are you seeing that?

I'm also seeing the cursor issue. I can't see what's happening with the row in the inspector when I drag it. Can you describe what happens to the row that follows the cursor as you drag it over the data list? Is it cloned and absolutely positioned? I think we should add a class to the row that's being dragged and assign pointer styles to it. I would also like to assign a z-index so we can assure it is positioned on top of other things in the data list that might have their own z-index

[kmcfaul]

@mcoker As far as I can tell, the drag image is a literal image of the row, positioned on the cursor. I don't think the cursor sees it at all? Everything it is dragging over would have to have that pointer styling (or maybe it's possible to programmatically set the styling on the body while it's dragging, and remove it when the drag ends/is cancelled). The drag image is part of HTML 5 handler events so browsers may handle it slightly differently.

Setting a custom drag image is possible, but it's expecting an image/simple element, not a React Element / clone of something.

[kmcfaul]

@jessiehuff The check aria-labelledby I think I just misunderstood when I was editing examples, it should probably be labelled by the items' ids.

I'm having trouble understanding how aria-live="assertive" should be applied to the example. Does every item need a div with this? And what should it say? The core demo has it at the end of the list but if it's meant to describe dragging I'm not sure how.

edit: Updating with what I've added currently minus the live tag.

[mcoker]

@kmcfaul ah, gotcha! in that case we should be able to do something like this? https://codepen.io/mcoker/pen/oNxRRbe

[kmcfaul]

That might work. Do we need to add a new class for it or can it be detected via the ghostrow modifier we already apply when dragging?

[mcoker]

The ghost row class is added to the clone that we move around in the data-list as you drag right? This class would be added to the thing the browser creates when you drag. You can test it by adding el.style.cursor = "grabbing"; to see where the style belongs.

[codecov-commenter]

Codecov Report

Merging #4852 into master will decrease coverage by 0.34%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##           master    #4852      +/-   ##
==========================================
- Coverage   52.49%   52.14%   -0.35%     
==========================================
  Files         527      529       +2     
  Lines        9603     9701      +98     
  Branches     3552     3572      +20     
==========================================
+ Hits         5041     5059      +18     
- Misses       3916     3993      +77     
- Partials      646      649       +3     

Flag	Coverage Δ
#patternfly4	52.14% <29.41%> (-0.35%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...es/react-core/src/components/DataList/DataList.tsx	25.51% <24.74%> (-60.21%)	⬇️
...ore/src/components/DataList/DataListDragButton.tsx	28.57% <28.57%> (ø)
...eact-core/src/components/DataList/DataListItem.tsx	34.78% <34.78%> (-6.89%)	⬇️
...t-core/src/components/DataList/DataListControl.tsx	50.00% <50.00%> (ø)
...act-core/src/components/DataList/DataListCheck.tsx	46.66% <80.00%> (+10.30%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17763f0...7da248a. Read the comment docs.

[kmcfaul]

Hmm that doesn't seem to work (e.target.style.cursor = 'grabbing' at least, applied on dragStart and removed on dragEnd). It applies to the grabbed row but I the cursor still changes so I don't think it's getting transferred to the drag image. Maybe if it's a global css it's different?

From what I've looked up so far, it could be the text selection in Chrome just overrides the pointer? FF doesn't seem to have this issue either.

[nicolethoen]

Just popping in here to say, @kmcfaul this is really cool! I'll take my time walking through to review it shortly, but playing with it is so fun!

[jessiehuff]

I completely understand what you mean about the aria-live, it can be confusing. 🙂 When you want to be able to give screen reader users live updates about what is happening (something a sighted user can usually get visually), you can create a container with aria-live on it so that any information you want to give them gets sent to that container to be announced. This container has to live on the page prior to any of the actions being announced. So, for example:

In the demo example, they have a div set to aria-live and then as you click the drag button, it announces "{item name} grabbed" or "{item name} dropped". This text gets inserted into the aria-live container. In this demo, this text gets removed after a certain amount of time although that may not always be the case depending on the use case. Our toast alerts look something like this:

<div class="pf-c-alert pf-m-success" aria-label="Success Alert" data-ouia-component-type="PF4/Alert" data-ouia-safe="true" data-ouia-component-id="OUIA-Generated-Alert-success-2" aria-live="polite" aria-atomic="false"> When users initiate the alert, that information gets put into the aria-live container and then is announced by screen readers. When the toast alert is closed, then it gets removed from the container. I think it'd be preferrable to handle it the way this demo does and have it removed from the container after it's announced or some nominal amount of time, but I think that part is somewhat flexible.
Does that all make sense? Also, there's a part of me that keeps thinking to use enter to initiate the drag as well as space. Would that be something easy to add? I was talking to Jenn about what her expectations between the two would be, and she was thinking we should try to hook into whatever the typical onClick event would be. So if a browser uses enter to select buttons, enter would work, and if the browser supported/used space to select buttons, space would work.

I agree with Nicole though, this looks awesome!!! 😄

[kmcfaul]

@jessiehuff Added the aria-live div and support for updating its text, let me know if I approached this the right way!

I also added logic for Enter to begin the dragging operation, in addition to space.

[jessiehuff]

Small nit in terms of the string used in dragging the items, but otherwise looking great!!! (It currently uses the index- i.e. "Dragging index 0 to 1", but I'm not sure it's completely ideal since it dynamically changes during drag? Perhaps we could just be more generalized and say "Dragging Item 1". Other than that, the keyboard interaction is awesome, and I think it works in VO (it's slightly tough to test because of the previous role that was added ("listbox") which is interfering with testing (removing in #4913). VO seems to really not like nesting form elements inside of other form elements like buttons inside of list boxes. I can pull it down though and check locally.

[mcoker]

LGTM! We can work on the cursor styles in the next sprint.

[nicolethoen]

in the draggable example, should I be able to check a checkbox by hitting enter when focusing on the box? I dont think that's working.

[kmcfaul]

Rebased

[redallen]

It still works!

[mcarrano]

Looks great! Thanks for all the work on this @kmcfaul .

[jessiehuff]

• The biggest issue I see is that the Esc key no longer works in stopping the action. This makes it not a great user experience and rather confusing for keyboard users. You can tab out of it so it's not a keyboard trap from what I can tell (which is good), but it has some odd behavior and styling in this scenario:

• Some minor things that could be fixed are that because the ids on the page are not unique, when testing with screen reader, it thinks it's labelled by previous examples so the VO output is odd. Not urgent though as it reads fine when isolated by itself.

• We can open another issue as I don't want to hold anything up. However, the Esc key is a pretty big issue that we would need to prioritize.

[patternfly-build]

Your changes have been released in:

• @patternfly/react-catalog-view-extension@4.8.57
• @patternfly/react-core@4.63.0
• @patternfly/react-datetime@4.2.2
• @patternfly/react-docs@5.10.7
• @patternfly/react-inline-edit-extension@4.5.114
• demo-app-ts@4.50.0
• @patternfly/react-integration@4.50.0
• @patternfly/react-table@4.18.11
• @patternfly/react-topology@4.6.22
• @patternfly/react-virtualized-extension@4.5.102

Thanks for your contribution! 🎉