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

HTML5Backend Event Swallowing Updates #3052

Merged
merged 15 commits into from Feb 22, 2021
Merged

HTML5Backend Event Swallowing Updates #3052

merged 15 commits into from Feb 22, 2021

Conversation

darthtrevino
Copy link
Member

@darthtrevino darthtrevino commented Feb 22, 2021

This PR removes some events swallowing that was occurring in the HTML5Backend. Some of this may cause regressions in older browsers or edge case situtaions. The goal here is to minimize our interference with natural event patterns.

There are a few instances of preventDefault() that were either unavoidable or needed to be selectively disabled depending on the needs of clients. In the latter case we provide a configuration option in the HTML5Backend for unblockNativeTypeEvents

@tommoor related to #2852
Usage:

<DndProvider backend={HTML5Backend} options={{unblockNativeTypeEvents: true}}> 
{/* app *}
</DndProvider>

Fixes #3051

@darthtrevino darthtrevino changed the title Minimize Event Swallowing / Add Config for Blocking Swallowing of Native Event Types HTML5Backend Event Swallowing Updates Feb 22, 2021
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #3052 (7d6b555) into main (253a725) will increase coverage by 0.05%.
The diff coverage is 76.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3052      +/-   ##
==========================================
+ Coverage   45.19%   45.24%   +0.05%     
==========================================
  Files         248      248              
  Lines        3790     3799       +9     
  Branches      770      773       +3     
==========================================
+ Hits         1713     1719       +6     
- Misses       2076     2079       +3     
  Partials        1        1              
Impacted Files Coverage Δ
packages/react-dnd/src/hooks/useDrop.ts 0.00% <0.00%> (ø)
packages/backend-html5/src/HTML5BackendImpl.ts 71.47% <81.81%> (+0.28%) ⬆️
packages/backend-html5/src/OptionsReader.ts 61.11% <100.00%> (+7.77%) ⬆️
packages/backend-html5/src/index.ts 100.00% <100.00%> (ø)

@tommoor
Copy link

tommoor commented Feb 22, 2021

@darthtrevino I'm not familiar with the react-dnd architecture yet, but is it not possible to restrict all of it's event capturing to within the boundaries of a dom element, perhaps by passing a ref to the DNDProvider?

I'd hate to see all of the IE support you've built over the years have to suffer…

@@ -632,6 +610,9 @@ export class HTML5BackendImpl implements Backend {

public handleTopDragLeaveCapture = (e: DragEvent): void => {
if (this.isDraggingNativeItem()) {
if (!this.options.unblockNativeTypeEvents) {
!this.options.unblockNativeTypeEvents
Copy link

Choose a reason for hiding this comment

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

This looks unexpected

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thank you

@darthtrevino
Copy link
Member Author

@tommoor That's not a terrible idea. I've added a rootElement configuration option in this PR that will change the element that's used to subscribe to DnD events. Can you try this option first? (I can always back out the other changes)

@tommoor
Copy link

tommoor commented Feb 22, 2021

I wish I could test but for some reason yarn install in this project won't work on my M1 Mac, it immediately fails with a wasm out of memory error. I'll take a look into it tomorrow…

@darthtrevino
Copy link
Member Author

@tommoor I published this branch as react-dnd-html5-backend@12.0.1-canary.1 if that helps

@tommoor
Copy link

tommoor commented Feb 22, 2021

Good news – it seems to work well passing the rootElement in testing on that canary build, I am seeing this error in the console after completing a drag on the react-dnd element, although it has no clear user-facing effect. Probably one of those removed preventDefault's.

image


https://github.com/outline/outline/pull/1918/files#diff-e49bad73ae3c25e5503e9881b61e3e0bf64b6fab88ffdcc9ebaa4014865ff30cR75-R79

@tommoor
Copy link

tommoor commented Feb 23, 2021

Something in 12.1.0 regressed the hover interaction, I'm not sure if it was this but I'm now seeing green "plus" symbol when moving items in Chrome that wasn't there before. I can file a new issue if you'd like

@darthtrevino
Copy link
Member Author

darthtrevino commented Feb 23, 2021 via email

@tommoor
Copy link

tommoor commented Feb 23, 2021

I wasn't actually, updating to v13 with the v12.1.0 backend just gives more new errors 😬

scratch this, I just realized the API changed.

@tommoor
Copy link

tommoor commented Feb 26, 2021

Cut an issue as you probably saw #3099 – I'm disturbed by the fact that the docs site seems… fine. Maybe you'll spot something looking at the diff?

tommoor added a commit to outline/outline that referenced this pull request Mar 10, 2021
…1918)

* fix: Drag and drop images in editor, conflict with sidebar react-dnd
see: react-dnd/react-dnd#3052

* Bump to non-canary

* Upgrade react-dnd

* react-dnd api changes

* lint

* fix: dnd doesn't work on first render

* remove unneccessary async

* chore: Update react-dnd (API changed again)

* restore fade
darthtrevino added a commit that referenced this pull request Feb 3, 2022
* refactor: remove preventDefaulth in handleTopDragEnter

* refactor: remove preventDefault from handleSelectStart

* feat: add HTML5Backend options to unblock drag

* chore: cut semver doc

* fix: typo

* feat: add rootElement option for attaching event listeners to

* fix: revent some cuts

* fix: revert some more cuts

* fix: revert more cuts

* feat: use localized eventing option in DnDProvider on docsite

* fix: revert unblockNativeTypeEvents config options experiment

* fix: guard endDrag() invocations with isDragging()

* fix: error in dropTarget accept arrays

* chore: update semver impact

* chore: update semver
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.

Cannot call endDrag while not dragging
2 participants