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

Update cancel check to be shadowRoot compatible #2086

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

malarahfelipe
Copy link

This proposal is made to ensure if the event should be canceled if the first element in the composedPath (or the event.target as fallback) matches with the options.cancel selector

This behaviour happens when we're using shadowRoot and we're trying to cancel the mousedown event when it starts from an element within the shadowRoot.

This proposal is made to correctly check if the event should be canceled if the first element in the composedPath (or the event.target) matches with the options.cancel selector
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: malarahfelipe / name: Felipe Mateus Malara (3a1b590)

@fnagel
Copy link
Member

fnagel commented Jun 22, 2022

@mgol What do you think about this?
@malarahfelipe Can you try fixing the tests?

@malarahfelipe
Copy link
Author

@fnagel sure thing 👍

@malarahfelipe
Copy link
Author

Fixed @fnagel.

Surely this is a specific case scenario, where I'm using Sortable with Web Components, but this may come in hand when Web Components get more and more popular

As answered in the question below, the purpose of using event.composedPath() is to get specifically which element triggered the event within the Web-Component (container).

image

@malarahfelipe malarahfelipe force-pushed the patch-1 branch 2 times, most recently from 92bb322 to e272483 Compare June 23, 2022 13:15
@fnagel
Copy link
Member

fnagel commented Jun 24, 2022

@malarahfelipe Tests still fail :-(

@malarahfelipe
Copy link
Author

malarahfelipe commented Jun 25, 2022

Sorry, missed the grunt test workflow
I believe is fine now 👍 @fnagel

@fnagel
Copy link
Member

fnagel commented Jun 25, 2022

Still failing.

When using web-components with shadowRoot, the mouseDown event is triggered with the event.target pointing to the entire web component, not inner nodes that were clicked, thus to cancel the mouseDown based on inner-elements we have to use composedPath instead of event.target
@malarahfelipe
Copy link
Author

Surely, the lint got me again sorry, now I was able to run them locally and seems fine
image

Thanks

@mgol
Copy link
Member

mgol commented Jun 27, 2022

It'd be good to see what case currently doesn't work on a live test case. This PR needs tests anyway, preparing an isolated test case may be a good starting point in creating one.

As it stands, I don't have a specific opinion as I don't understand what exactly this PR is trying to fix.

@fnagel
Copy link
Member

fnagel commented Jul 7, 2022

@mgol Thanks for the feedback! I must confess I'm not really familiar with Web Components so I hoped for your input :-)

Maybe @dmethvin can help us out here?

@malarahfelipe Thanks for fixing the tests! Can you give some some more insights?

@malarahfelipe
Copy link
Author

Thanks for the feedback on this guys

Sure @fnagel
I'll prepare two codepens one using web-components and other not to better explain the differences on this

@malarahfelipe
Copy link
Author

Okay, after long time 😅
I've set up three simple codepens to better explain the proposed solution

  1. JqueryUI.Sortable without ShadowRoot https://codepen.io/malarahfelipe/pen/QWmEoVz
  2. JqueryUI.Sortable with ShadowRoot ❌https://codepen.io/malarahfelipe/pen/XWEjpWo
  3. JqueryUI.Sortable with ShadowRoot ✅ https://codepen.io/malarahfelipe/pen/eYMBYGb

Also I've record a <1min loom video just dragging things around so you can see the output in the html / console

https://www.loom.com/share/a2c4f95d5eae4589967a00041bcb7307

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants