react-beautiful-dnd icon indicating copy to clipboard operation
react-beautiful-dnd copied to clipboard

Support for nested scroll containers

Open alexreardon opened this issue 7 years ago • 222 comments

Adding support for n-level deep scroll containers. Currently, only a single level is supported

Current plan

This plan will allow for nested scroll containers, and also improve the performance of scroll updates

Collection (drag start)

  • Grab all of the Droppable elements
  • Take the first one and walk up the DOM tree. If the element is a scroll container then add a data-* attribute to it. (eg data-react-beautiful-dnd-scroll-container=${index}). Cache the element and its result during the collection
  • When the node.parentElement is null then move onto the next Droppable. If an element is found that has previously been investigated then skip the rest of the upwards search. Use any previously found scroll parent ancestry.
  • When visiting an element an we also need to check to see if the element is position:fixed for our position:fixed support

Storage while dragging

  • When storing a Droppable we will keep a map (or linked list) that registers a Droppables scroll containers.
  • A droppable's frame will need to be updated when any of the ancestry changes
  • When calculating droppable displacement internally, all of the ancestry will need to be taken into account

Updates (scroll events)

A single scroll listener is added to the window as a capture:true listener. This will capture all scroll events.

  • If the source of a scroll event is a scroll container that has a data-react-beautiful-dnd-scroll-container attribute then trigger an action to update all relevant Droppables. One redux update for all Droppables (fixes #244). The internal algorithms will need to be updated to account for 0 <-> many scroll containers
  • If a scroll is on the window then trigger a window scroll action. Currently, this is handled by the drag handles. Drag handles will no longer handle this
  • If the source of a scroll event is a scroll container that is NOT a data-react-beautiful-dnd-scroll-container then it can be ignored

Auto scrolling

  • There will need to be some investigation into how this will work.
  • Need to investigate how our push scroll holds up when moving with a keyboard

Clean up (drag end)

  • Remove all of the data-react-beautiful-dnd-scroll-container attributes
  • Remove single window event listener

Bonus

  • We could keep the scroll listeners active while a drop animation is occurring and flush any drop animations

alexreardon avatar Oct 02 '17 19:10 alexreardon

Given the complexity in supporting a single level, I think this is out of scope for now!

alexreardon avatar Feb 11 '18 22:02 alexreardon

@alexreardon Does this issue affect Core boards / Jira Software boards? I'm trying to improve our board experience in Dovetail and running into a lot of issues that seem to be caused by this one. I would love to know a little bit about how the Core / Software teams have tackled this.

humphreybc avatar Jun 01 '18 01:06 humphreybc

Hi @alexreardon, We just bumped to 8.0.1 🎉 and are noticing this warning. We have a horizontal scrollable board with vertical columns. It is no problem for us that we can't drag, then horizontal sroll, then drop at the right place, but we would like to clear this console warning, even in development mode. Is that possible ?

Thanks

goldo avatar Jul 04 '18 15:07 goldo

I am running into this issue too (scrollable columns + horizontal scroll on the board). I think it would be very helpful if you could tackle this in the near future. Thank you!

mtsc-rrapiteanu avatar Jul 23 '18 06:07 mtsc-rrapiteanu

Doesn't quite understand this issue, the long lists in a short container seems to work fine

Also, agree to @goldo, spamming too much console.warn() in the codebase isn't healthy

alxtz avatar Jul 24 '18 10:07 alxtz

@alxtz Nested scroll containers would mean that you can vertically scroll through individual columns whilst having another parent scroll container as right now you can't.

sis avatar Jul 29 '18 11:07 sis

@goldo perhaps we could add an option to opt out of all warnings - even in development? 🤔

alexreardon avatar Aug 03 '18 05:08 alexreardon

Hmm might be dangerous wouldnt it ? Id like to know if there is a warning in dev mode, but if i understand it and I accept it, I would like to get rid of it. Maybe disabling only this warning would be best ? On 3 Aug 2018 at 07:05 +0200, Alex Reardon [email protected], wrote:

@goldo perhaps we could add an option to opt out of all warnings? 🤔 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

goldo avatar Aug 03 '18 23:08 goldo

@goldo would making the disableWarning option to be true by default solve your concern?

alxtz avatar Aug 07 '18 03:08 alxtz

@alxtz like I said, I think warnings are useful, specially in case of bumps, so I would prefer not to remove all the warnings. In this case, we are totally OK with this issue, it's not a problem at all, in our application. That's why we would like to remove only this specific warning, and keep all the others. If it's too complex to do, I think we will keep all the warnings, just in case of problems (at least in dev mode)

goldo avatar Aug 07 '18 14:08 goldo

when to support dragging items from the parent list into a child list?

FEliuyg avatar Aug 29 '18 02:08 FEliuyg

This issue is a real problem for Kanban/Trello like apps (a big use case for D&D apps I guess?). The warning is very nice, but it is pretty unclear before making an app, you realize it when it is too late. I hope this will be fixed in the near future though 🙏

jebarjonet avatar Sep 30 '18 17:09 jebarjonet

an option to opt out of all warnings will be good. Too much warning while the functionality work as per normal.

wesleywong avatar Oct 01 '18 05:10 wesleywong

@jebarjonet brings up a great point. I'd echo others' points that the option to opt out would be highly appreciated!

IsenrichO avatar Oct 01 '18 05:10 IsenrichO

Would it be okay for me to make a PR implementing this opt-out functionality? Should I open a new issue for it?

dylmye avatar Oct 24 '18 09:10 dylmye

Hi @alexreardon,

are you planning to implement this feature in the near future?

bmz1 avatar Oct 24 '18 14:10 bmz1

We have a dev warning opt out coming in v10 On Thu, 25 Oct 2018 at 1:49 am, Boér Máté [email protected] wrote:

Hi @alexreardon https://github.com/alexreardon,

are you planning to implement this feature in the near future?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atlassian/react-beautiful-dnd/issues/131#issuecomment-432689102, or mute the thread https://github.com/notifications/unsubscribe-auth/ACFN7RipwmL8IBPWNBqOOuCr8kUSykoeks5uoH3dgaJpZM4PrKeZ .

alexreardon avatar Oct 24 '18 18:10 alexreardon

Just to clarify the core of this issue (not the dev warning complaints)... it is currently impossible to create a Trello clone (for example) with this library because you cannot have scrolling columns and a horizontally scrolling board container.

kole avatar Nov 19 '18 16:11 kole

We will be starting work on this feature soon On Tue, 20 Nov 2018 at 3:37 am, Nick Johnson [email protected] wrote:

Just to clarify the core of this issue (not the dev warning complaints)... it is currently impossible to create a Trello clone (for example) with this library because you cannot have scrolling columns and a horizontally scrolling board container.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atlassian/react-beautiful-dnd/issues/131#issuecomment-439957615, or mute the thread https://github.com/notifications/unsubscribe-auth/ACFN7cJPPXA1D5XdUzin9FJpX-PeyvMJks5uwt5OgaJpZM4PrKeZ .

alexreardon avatar Nov 19 '18 18:11 alexreardon

@alexreardon That's great news! Do you anticipate this being a v11 improvement because of the complexity or might this fit into a v10 minor release? Just trying to get a very loose timeline picture. Thx

kole avatar Nov 19 '18 18:11 kole

It will be a big piece of work, but it will probably be a minor sem ver release On Tue, 20 Nov 2018 at 5:28 am, Nick Johnson [email protected] wrote:

@alexreardon https://github.com/alexreardon That's great news! Do you anticipate this being a v11 improvement because of the complexity or might this fit into a v10 minor release? Just trying to get a very loose timeline picture. Thx

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atlassian/react-beautiful-dnd/issues/131#issuecomment-439994317, or mute the thread https://github.com/notifications/unsubscribe-auth/ACFN7dGNe0RDDArVCTkun2z2j1wCzkGHks5uwvhYgaJpZM4PrKeZ .

alexreardon avatar Nov 19 '18 18:11 alexreardon

@alexreardon scrollable columns and horizontal scroll on the board

sunran357 avatar Nov 28 '18 06:11 sunran357

Correct me if I'm wrong, but this example from the react-beautiful-dnd example page has nested scroll containers does it not?

How does this example work without giving the error message "Droppable: unsupported nested scroll container detected" ?

daviddworsky avatar Nov 28 '18 23:11 daviddworsky

I am starting to think about algorithms for solving this problem

Collection (drag start)

  • Grab all of the Droppable elements
  • Take the first one and walk up the DOM tree. If the element is a scroll container then add a data-* attribute to it. data-react-beautiful-dnd-scroll-container=${index}. Cache the element and its result during the collection
  • When the node.parentElement is null then move onto the next Droppable. If an element is found that has previously been investigated then skip the rest of the upwards search. Use any previously found scroll parent ancestry.
  • When visiting element an we also need to check to see if the element is position:fixed for our position:fixed support

At the end of the collection, a Droppable will have 0 <-> many associated scroll container indexes which match the indexes applied to data-react-beautiful-dnd-scroll-container=${index}.

Updates (scroll events)

A single scroll listener is added to the window as a capture:true listener. This will capture all scroll events.

  • If the source of a scroll event is a scroll container that has a data-react-beautiful-dnd-scroll-container attribute then trigger an action to update all relevant Droppables. One redux update for all Droppables (fixes #244). The internal algorithms will need to be updated to account for 0 <-> many scroll containers
  • If a scroll is on the window then trigger a window scroll action. Currently, this is handled by the drag handles.
  • If the source of a scroll event is a scroll container that is NOT a data-react-beautiful-dnd-scroll-container then it can be ignored

Clean up (drag end)

  • Remove all of the data-react-beautiful-dnd-scroll-container attributes
  • Remove single window event listener

alexreardon avatar Dec 03 '18 00:12 alexreardon

Hi,How long will it take to get online?

sunran357 avatar Dec 03 '18 01:12 sunran357

@daviddworsky the example is mounted in an iframe, so it only has one scroll container inside of a window

alexreardon avatar Dec 03 '18 01:12 alexreardon

@sunran357 not sure. You can follow here for updates: https://github.com/atlassian/react-beautiful-dnd/milestone/2

alexreardon avatar Dec 03 '18 01:12 alexreardon

@alexreardon Awesome! Do you know which browsers support window scroll event with capture?

TrySound avatar Dec 03 '18 11:12 TrySound

All it looks like On Mon, 3 Dec 2018 at 10:42 pm, Bogdan Chadkin [email protected] wrote:

@alexreardon https://github.com/alexreardon Awesome! Do you know which browsers supports window scroll event with capture?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atlassian/react-beautiful-dnd/issues/131#issuecomment-443681901, or mute the thread https://github.com/notifications/unsubscribe-auth/ACFN7d8qjsx6glb_lFYAmEw8Q6a3LFB5ks5u1Q4hgaJpZM4PrKeZ .

alexreardon avatar Dec 03 '18 18:12 alexreardon

Any updates on when this will be done and released?

Teippo2020 avatar Jan 09 '19 23:01 Teippo2020