fix(DnD): fix drag and drop in Shadow DOM #11975
Conversation
TeodorTaushanov
changed the title
TeodorTaushanov
changed the title
dimovpetar
force-pushed
the
drag_drop_shadow_dom
branch
from
f18cca0 to
ce1dc89
Compare
| @@ -404,7 +404,7 @@ class Table extends UI5Element { | |||
| @i18n("@ui5/webcomponents") | |||
| static i18nBundle: I18nBundle; | |||
|
|
|||
| _events = ["keydown", "keyup", "click", "focusin", "focusout", "dragenter", "dragleave", "dragover", "drop"]; | |||
| _events = ["keydown", "keyup", "click", "focusin", "focusout", "dragstart", "dragenter", "dragleave", "dragover", "drop", "dragend"]; | |||
| DragRegistry.subscribe(this._table); // TODO: Where unsubscribe? | ||
| } | ||
|
|
||
| _ondragstart(e: DragEvent) { |
There was a problem hiding this comment.
I'm not sure if there's a technical reason for it, but it feels really wrong: does every component that might become movable have to handle those things itself
There was a problem hiding this comment.
Yes, now interested components need to call setDraggedElement/clearDraggedElement. The alternative approach to overcome shadow DOMs is to keep the global document handler in the DragRegistry and apply heuristic search using the event's composedPath(). This approach was discussed and rejected.
Another pro of setDraggedElement/clearDraggedElement is that it is more obvious to component developers as no logic happens behind the scenes. In the future, we might improve it even further by introducing a DragDropDelegate, similar to the ItemNavigation
There was a problem hiding this comment.
I'm not sure I understand the reasoning here. Why can't these lines be implemented within the DragRegistry? Since the table elements are in the light DOM, there should be no difference between registering this event on document.body or handling it directly in the Table.
You mentioned that this approach was discussed and rejected, but the reasons for that decision have not been shared. Also, the "pro" you mentioned is from my perspective a "contra".
There was a problem hiding this comment.
The only functional difference I can see between handling dragstart/dragend in the Table versus the DragRegistry would be in scenarios where the Table is used inside the shadow root of another Web Component with movable rows enabled( lets name it a SmartTable)
In your current approach, the source parameter of the dragstart event would be the TableRow inside the Table's shadow DOM (if this is desired from the SmartTable perspective is discussable). If handled via DragRegistry, the source would instead be the SmartTable. I don’t see this as a problem, this is consistent with how Web Components behave. The target parameter of any event never points to an element inside a shadow DOM. If you've discussed this internally and disagree, I'd recommend inspecting composedPath() to find the first UI5 element that is movable.
PS: In the current implementation, because the table elements reside in the light DOM, dragging a link or text within a row results in the dragged element being registered in the DragRegistry instead of the row itself. I believe this is incorrect. This suggests there may need to be adjustments in the TableRow implementation.
Jira: BGSOFUIRODOPI-3442
fixes #11185