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

Rendering in shadow root - clicks don't register #536

Closed
Rendez opened this issue Aug 8, 2018 · 10 comments
Closed

Rendering in shadow root - clicks don't register #536

Rendez opened this issue Aug 8, 2018 · 10 comments

Comments

@Rendez
Copy link
Contributor

Rendez commented Aug 8, 2018

  • downshift version: v2.0.20

What you did:

Render Downshift within a shadow root using the web components API.

What happened:

Clicking on items do not select or register at all.

Reproduction repository:

https://codesandbox.io/s/kk674yvm0o

Problem description:

This is totally related to #200, and it has been fixed. However to make it work with a shadowRoot (document fragment) is currently very cumbersome. Again, see the example: https://codesandbox.io/s/kk674yvm0o

Suggested solution:

Current, addEventListener and removeEventListener rely on environment.document to be present, which works within an iframe. For shadowRoots, we could try environment.document || environment.ownerDocument.

@kentcdodds
Copy link
Member

Hi @Rendez!

Thanks for the issue. If that fixes the issue then you're welcome to make a pull request for it. I would like to have a test for this though. It looks like jsdom does not yet support web components, so we'll probably need to make it part of the E2E test suite with Cypress and storybook.

Thanks!

@Rendez
Copy link
Contributor Author

Rendez commented Aug 9, 2018

Thanks for the suggestion on js-dom. I will make it happen though might take a couple of days, cheers!

@Rendez
Copy link
Contributor Author

Rendez commented Sep 29, 2018

Alright, so I started looking at the source and couldn't find a strong-enough reason to do this elegantly.
I wouldn't modify the current approach, since the environment has to be a window-like object, and ShadowRoot simply isn't one. In other words, the current implementation covers iframe and window usages perfectly, but it's also normal to want to adapt it for web components.

GIven the API surface we need, add(remove)EventListener and document, there are two possible ways to approach it:

  1. We define a getter for environment.document internally that "assumes" ownerDocument is what we need when document isn't found within the environment object.
  2. We provide a synthetic environment object from the outside, which can be done in a custom way depending on what we need, thus, not being bound to ShadowRoot and its particular object's prototype.

IMO, the first approach would offer a targeted solution, but offer less guarantees. Therefore, I've come up with a more refined and elegant way to approach it from the outside (as in 2.):

https://codesandbox.io/s/j42nwojwzy

const isProxySupported =
  typeof Proxy === 'function' &&
  // https://github.com/facebook/react/issues/12011
  !Object.isSealed(new Proxy({}, {}))

/**
 * @param environment ShadowRoot|Window|Object
 */
const createEnvironment = environment => {
  // assume first a non-ShadowRoot object
  if ('document' in environment) {
    return environment
  }
  // es6-way (support lacking on IE and older Safari versions)
  if (isProxySupported) {
    // this is the most elegant way
    return new Proxy(environment, {
      get: function(obj, prop) {
        if (prop === 'document') {
          // ownerDocument would be the closest choice in relation to an iframe 'window'
          return obj.ownerDocument 
        }
        // is there in which Reflect.apply would be helpful here?
        return obj.ownerDocument[prop].bind(obj)
      },
    })
  }
  // es5-way
  return Object.create(Object.prototype, {
    document: {
      configurable: false,
      get() {
        return environment.ownerDocument
      },
    },
    addEventListener: {
      configurable: false,
      get() {
        return environment.ownerDocument.addEventListener.bind(environment)
      },
    },
    removeEventListener: {
      configurable: false,
      get() {
        return environment.ownerDocument.removeEventListener.bind(environment)
      },
    },
  })
}

Let me know your opinion on this, and if looks like a good use-case solution we could document it somehow for the future (perhaps on how to use environment with an iframe vs shadow-root).

@kentcdodds
Copy link
Member

Hmmm.... I think that rather than build-in support for this I'd rather document how to make this work with the existing API. The reason being that it's not too difficult to do it yourself and 99% of people don't have this use case.

Would you like to make a pull request to document how to do this?

@Rendez
Copy link
Contributor Author

Rendez commented Sep 30, 2018

That is exactly what I was trying to say. I'll open the PR :)

@Rendez Rendez closed this as completed Sep 30, 2018
Rendez added a commit to Rendez/downshift that referenced this issue Oct 2, 2018
* Add link to gist example featuring a shadow-root as environment
Rendez added a commit to Rendez/downshift that referenced this issue Oct 2, 2018
* Add link to gist example featuring a shadow-root as environment
kentcdodds pushed a commit that referenced this issue Oct 2, 2018
* Add link to gist example featuring a shadow-root as environment
@TazmanianD
Copy link

TazmanianD commented May 6, 2019

@Rendez I have a question about this. A link to your solution is provided on the main doc page but I'm confused as to why it has to be as complicated as it is. Why do you need to use a Proxy for this? Why can't you just return an object that has the three properties that downshift expects that do what you need. Why doesn't this work?:

function createDownshiftEnvironment(shadowRoot) {
  return {
    document: shadowRoot,
    addEventListener: shadowRoot.addEventListener.bind(shadowRoot),
    removeEventListener: shadowRoot.removeEventListener.bind(shadowRoot)
  };
}

@Rendez
Copy link
Contributor Author

Rendez commented May 7, 2019

@TazmanianD that works too, I was being extra safe and also playful in the example above.

@TazmanianD
Copy link

TazmanianD commented May 7, 2019

@Rendez I've been playing around with this some more and I think there's a bug in your proxy and by extension mine as well. You're assigning environment.document to shadowRoot.ownerDocument but I think that's a mistake. The downshift library uses environment.document to get at the active element as well as call getElementById but those properties aren't on ownerDocument, they're on shadowRoot itself. I think where you've got

document: ownerDocument,

It really should be

document: shadowRoot,

Does that seem right to you?

And actually I think this may be true for the addEventListener calls as well. Wouldn't it be more appropriate to add those listeners to the shadow root directly?

@Rendez
Copy link
Contributor Author

Rendez commented May 8, 2019

About the add/removeEventListener, yes, it would be more correct to assign them to the shadowRoot that's right, more similar to window. However I do have all of the prototype properties you mention within ownerDocument. I do not however have body in the shadowRoot prototype. Also, do you know if the activeElement of shadowRoot is ever anything but null?

To be honest I haven't tested what I wrote above outside my one example in a project, and for that it worked perfectly and continues to work (even after body and activeElement became necessary)

Can you show me how you're creating your shadowRoot, in which your shadowRoot's ownerDocument doesn't have getElementById etc? (I might be wrong about the assumption for your case)

@TazmanianD
Copy link

I found that your version did not work correctly although the only case where I could see that was that the dropdown wasn't scrolling automatically when you use the arrow keys to navigate items. The ownerDocument does have getElementById but it was always returning null for some reason but calling it on the shadow root does work.

And yes, the activeElement on the shadow root did seem to be the actual element that has focus while the activeElement on the ownerDocument was the I think the shadow root itself or the host element..I forget exactly which.

I went ahead and updated my comment above to show what I ended up using it does seem to work for me.

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

No branches or pull requests

3 participants