qodesmith / datepicker

Get a date with JavaScript! A datepicker with no dependencies.
344 stars 101 forks source link

feat(shadow-dom-support): add option isInShadowDom, making Datepicker listen to events on input's parent instead of document #68

Closed emil-laurell closed 4 years ago

emil-laurell commented 5 years ago

We want to use the datepicker on inputs in shadow DOM.

But since the datepicker listens to events (for all instances) on document, the datepicker couldn't be used for an input within a shadow DOM (because the event's target is set to the input's shadow host)

In this PR an option (isInShadowDom) is added. If set to true event listeners will also be added to the parentElement of the input element. When one of the events are triggered, the same event handler logic will be executed + a property (handledForShadowDom) will be added to event (with value true). When the event bubbles to the global event handler on document, the ordinary logic will not be run. (but other events, like clicks outside calendar, will still be handled in the global event handler.

qodesmith commented 5 years ago

Hey @zumankii. I appreciate the effort you made with this PR. I'm not familiar with using the shadow DOM. Would this whole thing be much easier if there was an option to allow the user to attach the listeners to any element they wanted instead of the document? I'm thinking something like this:

datepicker('.shadow-input', { root: myShadowDom })

This could have the effect of attaching the listeners to myShadowDom and querying for .shadow-input within myShadowDom. If I'm missing something huge here, by all means help me out. Thanks brother!

emil-laurell commented 5 years ago

Hey @qodesmith, Thanks for looking into this! I see what you're thinking - and likes the idea of keeping the selector string initializer intact.

The reason to why we're also adding a listener to document (in the shadow DOM case) is for events triggered on elements outside the parent/root, e.g. hiding the datepicker when user clicks outside.

Maybe a good combination would be, like you wrote, to add the optional option root instead of isInShadowDom. If root is set we will query for selector within it and add listeners to both root element and document. If an event is handled in "root listener", we will set a property on the event marking it as handled in root, so the "document listener" will ignore it. What do you think?

emil-laurell commented 4 years ago

Hello again @qodesmith , Sorry for the delay, but now this thing is on the table again :)

In our case we will use the datepicker in a custom element (extending a native input element). So, we don't know what selector or root parameter to pass as arguments (since the surrounding markup is up to the application using the custom element)

I think this is the best way for our use case (the component case). What do you think? Is it mergable?

qodesmith commented 4 years ago

Trying to absorb this now...

qodesmith commented 4 years ago

@zumankii I just finished up writing the tests and published a new version. 5.4.0 is the latest. I'm going to mess around with customElements to try to wrap my brain around this. I'll follow up once I've done that. It's been hard to find time for it all!

qodesmith commented 4 years ago

@zumankii Can you share some example code of how you're using customElements / shadow dom to implement Datepicker?

qodesmith commented 4 years ago

Also curious how you were able to apply the CSS to the calendar within a web component. Still fooling around with this...

emil-laurell commented 4 years ago

@qodesmith I understand it's hard to find the time, but you are doing a great job! :)

I messed up my github notifications, so I didn't see your comments until now - sorry about that.

Here are a few examples (with the datepicker code from this PR): https://gist.github.com/zumankii/cd14d48e5bac9558385eff61318fae6e

It contains:

Regarding the css, since doing this with vanilla js I moved the css into a javascript file to be able to add it to a script tag. But when using webpack we can import the css as raw text and add it to the script tag instead.

Hope these examples helps!

emil-laurell commented 4 years ago

If you set isInShadowDom to false in date-picker-custom-element.js and native-input-date-picker.js, only the first native-input-date-picker in index.html will work. Same thing if you are using the datepicker code from the master branch.

qodesmith commented 4 years ago

@zumankii I've got a work in progress that seems complete, but I'm writing tests for it it still. You can see it here. I took your approach with adding a property on the event object. But a bunch of other stuff had to be done, such as ensuring the listeners are only applied once per shadow DOM. Let me know your thoughts.

qodesmith commented 4 years ago

Now that I look at it, I have to handle removing the event listeners correctly. My shadow DOM code isn't doing that at all. Ooopf.

emil-laurell commented 4 years ago

@qodesmith Great that you are looking in to this!

One thing that isn't working with this approach is the thing i wrote Jan 16; " In our case we will use the datepicker in a custom element (extending a native input element). So, we don't know what selector or root parameter to pass as arguments (since the surrounding markup is up to the application using the custom element) "

If you, in the gist above, does these changes:

... you will see that the native-input-date-picker will work when loaded from index.html (no shadow dom), but not when loaded via the other-component-using-date-pickers (that has a shadow dom)

I don't how this case should be handled in your approach, when the actual reference to the shadow dom should be passed as an option. Maybe a better name for the parameter in my PR would be shadowDomSupport ?

qodesmith commented 4 years ago

you will see that the native-input-date-picker will work when loaded from index.html (no shadow dom), but not when loaded via the other-component-using-date-pickers (that has a shadow dom)

I just copied all the contents of the files from the gist into a project in a local directory on my computer. All the datepickers worked fine with no error from the other-component-using-date-pickers section. What was the error that you saw?

Also, there were indeed 2 shadow DOM's produced from all the code you gave me in that gist:

qodesmith commented 4 years ago

@zumankii Another thing I'd like to get your thoughts on... in your case it looks like you end up passing an element from inside the shadow DOM to Datepicker. One idea I was thinking was to simply go up the chain of parent elements until you get to the root - which would either be a shadow DOM or document.body. From there we could implement logic that appends an extra listener to the shadow DOM.

In short, the api Datepicker would provide to work with custom elements would be you have to provide it an element, not a string selector. From there it will automatically figure out if the element is inside a shadow DOM or not and handle both scenario's accordingly.

What do you think?

qodesmith commented 4 years ago

If you set isInShadowDom to false...

Looks like I missed this statement. Ok, let me keep looking into things...

qodesmith commented 4 years ago

@zumankii Ya know, the more I look at your original solution, it seems to make the most sense. Can you merge the latest master back into your PR as well as change your code to use ES5 instead? I'll make comments once you've handled that.

emil-laurell commented 4 years ago

@qodesmith , now I've merged from master and made it es5 compatible.

emil-laurell commented 4 years ago

@qodesmith ping ;) Any thoughts about the PR?

qodesmith commented 4 years ago

@zumankii Hey, sorry for the delay. So before merging anything, I really wanted to understand webcomponents and the shadow dom a bit better. I've got a branch that I'm working on and I'd love to get your eyes on that code as well - https://github.com/qodesmith/datepicker/tree/shadow-dom-2.

I think shadom dom / custom support can happen without having to supply anything to the options object. Let me finish the work in the branch, put up a PR, and get your eyes on it before doing anything official. All I'm trying to do is make sure I get what's going on. Sorry it's taking so long, haven't had much brain real estate recently.

qodesmith commented 4 years ago

Hey @zumankii. Could I get your eyes on this PR? The delay in all this was mostly me trying to understand custom elements / shadow DOM before I went fwd with anything. Here's a few differences between our PR's (which I'd like to get your opinion on):

I'm definitely interested in your thoughts - and equally interested to get this shadow DOM work behind me 😃

emil-laurell commented 4 years ago

Great work, @qodesmith ! I really appreciate the effort you've put in to this! And I like that you're thinking outside the box (of my PR) to improve the component! I've reviewed and run some manual tests both in and outside shadow DOM - looks great!

I don't require a flag in the options object, although I keep with your sentiment on having to pass in a node (not a string selector) as the 1st argument.

Great for the API to avoid extra options. Unfortunately, the string selector probably is impossible in the shadow DOM case. So this is as good as it gets, I think.

I apply the listeners to the shadow DOM itself. If the el is a direct child of the shadow DOM, doing el.parentElement will fail because the shadow DOM isn't an element (but interestingly it is a node).

👍

I threw in some checking with a try/catch since IE doesn't support custom elements or the shadow DOM at all - this is still our reality 🙃. While an error is still thrown, a nice message is logged to the console with a link to a polyfill (although I don't think you can polyfill the shadow DOM, just custom elements 🤷‍♂).

IE11 is as good as gone from my reality - lucky me 😁 ShadyDOM ?

  • I refactored which elements are used for calculating position. You'll notice a new positionedEl element being used. Somewhere along the line, deep deep into the shadow DOM world, there was a reason I did this. Probably something to do with the calendar being a direct child of the shadow DOM and needing to go further up the chain to get a reference element for positioning.
  • I'm storing references to the shadow DOM and custom elements on the instance, just as a convenience.

👍

I'm definitely interested in your thoughts - and equally interested to get this shadow DOM work behind me 😃

Again, great work that looks great. Ready to release 😉

qodesmith commented 4 years ago

Closing since this was merged in #84. Thanks @zumankii for all your hard work on this and especially your patience! This was a great addition to Datepicker and I learned a ton along the way 😄