WickyNilliams / react-simple-colorpicker

Simple, composable, lightweight colorpicker for react
http://wicky.nillia.ms/react-simple-colorpicker
MIT License
46 stars 19 forks source link

Does not work when rendered into an iframe from parent document #13

Closed dsblv closed 8 years ago

dsblv commented 8 years ago

Because of subscription to document events (likely) the colorpicker breaks when used within an iframe.

Is there any particular reason to use native events? I guess this was just inherited from react-colorpicker.

I don't mind digging into this and submiting a PR if you approve.

WickyNilliams commented 8 years ago

Do you have an example where it breaks?

There's no easy way to hook into react's event system here AFAIK. We listen for mousedown on the actual element, but we need to listen to mousemove and mouseup on document so that we can continue to get events for mouse movement even if the user accidentally moves the cursor outside the element. Also if the user then lifts the mouse outside of the element, we know they have stopped adjusting the values. Otherwise the behaviour gets confusing to the user.

If you have a case where things are genuinely breaking I guess we could accept the document as an optional prop. That would be a simple solution

dsblv commented 8 years ago

I keep my app inside it's own iframe (it's a chrome extension) to prevent style/events clashing.

Can be reproduced with something like this:

const frame = document.getElementById('#frame-of-the-app');
const root = frame.contentWindow.document.getElementById('#root');

function handleColor(color) {
    console.log(color);
}

ReactDOM.render(
    <ColorPicker onChange={handleColor} />,
    root
);

As you can see, React lives in different environments to an actual DOM it renders, but colorpickes keeps subscribing to the parent document.

If you have a case where things are genuinely breaking

Well, it basically works until you start dragging around.

I guess we could accept the document as an optional prop

That'll work, but will be a bit inconvenient when an app has a complex structure with colorpicker buried deep inside it. I wanted to encapsulate the fact I even use iframe. :disappointed:

The best way in my mind is to implement alternative drag'n'drop using react's events. It'd have worse UX, but at least some UX.

WickyNilliams commented 8 years ago

OK, yea that's what you might would be doing (rendering into an iframe's document from outer document).

I could potentially get a reference on the correct document by doing the following in componentDidMount: ReactDOM.findDOMNode(this).ownerDocument. That should mean we always listen on the correct document, right? And then it's transparent whether in an iframe 10 levels deep or in the root document.

I'm not prepared to sacrifice UX for architectural purity, so hopefully that would suffice.

dsblv commented 8 years ago

I could potentially get a reference on the correct document by doing the following in componentDidMount: ReactDOM.findDOMNode(this).ownerDocument.

I'll be happy if it works, sounds possible :+1:

WickyNilliams commented 8 years ago

Out of curiosity, does it work if you create the iframe in react and render into it's body, with something like https://github.com/ryanseddon/react-frame-component?

dsblv commented 8 years ago

Just checked — nope.

Thank you for the link by the way!

WickyNilliams commented 8 years ago

Fixed in 9835c93769ef81601bf3ee9ce4998853a423da35

dsblv commented 8 years ago

Yeah, I confirm that it works!

Thank you! :tada:

WickyNilliams commented 8 years ago

That's great to hear! Thanks for reporting. Some slight perf improvements in latest release too, enjoy :)