joeattardi / picmo

JavaScript emoji picker. Any app, any framework.
https://picmojs.com
MIT License
1.19k stars 118 forks source link

Custom opening functionality bound to `click` event causes the popup picker to close instantly #228

Closed ahukkanen closed 2 years ago

ahukkanen commented 2 years ago

With the following HTML:

<div id="emoji-wrapper">
  <button id="emoji-button">Toggle emojis</button>
</div>

The following initialization will cause the emoji picker popup to close instantly after clicking the "emoji-button":

import { createPopup } from "@picmo/popup-picker";

const wrapper = document.getElementById("emoji-wrapper");
const picker = createPopup({
  rootElement: wrapper
}, {
  position: { top: 10, left: 10 }
});
wrapper.querySelector("#emoji-button").addEventListener("click", () => {
  picker.toggle();
});

This is because the "click" event propagation is not prevented in the event listener which causes it to propagate to the document object which is listened here: https://github.com/joeattardi/picmo/blob/9948a812ba13e99cb66722c8ec5c60812c643a44/packages/popup-picker/src/popupPicker.ts#L68

A workaround is to stop the event propagation at the listener but I just thought it might be a good thing if the document listening was omitted during the opening of the picker.

Example: https://jsfiddle.net/5m4p0yw7/

joeattardi commented 2 years ago

You have two issues with the code:

(1) When creating a popup picker, you need to specify the triggerElement. Otherwise, when you click the button, PicMo thinks you clicked outside of it and closes, which is what you are seeing here. triggerElement should reference the button you click to toggle the picker, and should be passed with the popup options (second parameter to createPopup).

(2) Popup pickers should not have a rootElement defined - the popup picker creates a root element of its own to show in a popup.

See https://picmojs.com/docs/usage/creating-pickers#popup-elements for more details.

ahukkanen commented 2 years ago

(1) When creating a popup picker, you need to specify the triggerElement. Otherwise, when you click the button, PicMo thinks you clicked outside of it and closes, which is what you are seeing here. triggerElement should reference the button you click to toggle the picker, and should be passed with the popup options (second parameter to createPopup).

Great thanks!

I did try triggerElement but I just thought it didn't work because I thought that passing the button element to it, it would automatically handle opening the picker when clicking it. Now that I pass it in the triggerElement it works fine.

(2) Popup pickers should not have a rootElement defined - the popup picker creates a root element of its own to show in a popup.

Well, that's the only way that I found to:

I am basically using the "fixed" positioning but passing it { position: "absolute", right: 0, bottom: 0 } and it works perfectly for our use case and matches how we used to position emoji-picker.

Also, I found this the only way to force the picker element inside a specific container as this line is using this option: https://github.com/joeattardi/picmo/blob/c435ece52fc4a05c2412b334a8dcccbb099b0673/packages/popup-picker/src/popupPicker.ts#L142

This is very much needed in our use case to be able to:

  1. Force the element inside a specific container (with relative positioning)
  2. Force the element at the bottom right corner of the container (using position: { position: "absolute", right: 0, bottom: 0 })

Otherwise it was not working how we want it to work.

joeattardi commented 2 years ago

Did you try using relative positioning and using your textarea as the referenceElement? That should work, if there is a bug with relative positioning and the element jumping around, we can open a new bug issue for that. Thanks!

ahukkanen commented 2 years ago

There's a wrapper around the <textarea> that has relative positioning. I believe the <textarea> itself is not relatively positioned. It lives at the very top corner of the wrapper.

We'd rather have the auto-positioning skipped because it seems unnecessary + utilizes unnecessary extra CPU cycles at the client-side where as the relative+absolute positioning works totally fine.

I can try to re-create the situation at codepen later.

joeattardi commented 2 years ago

I don't think relative positioning is going to cost you anything appreciable in CPU time. It should be a one-time position calculation.

Here's an adaptation of your fiddle with relative positioning around the textarea: https://jsfiddle.net/oavzb5gn/17/

ahukkanen commented 2 years ago

OK thanks, I guess the rootElement option was what was messing the positioning up. We want bottom-end but without the rootElement option it is now working fine without the "hacks".

But I believe there are still extra CPU cycles that I don't feel are necessary, as this line: https://github.com/joeattardi/picmo/blob/c435ece52fc4a05c2412b334a8dcccbb099b0673/packages/popup-picker/src/positioning.ts#L44

Is calling all of this: https://github.com/floating-ui/floating-ui/blob/fa9ae9acab663c124bb856144da6b5405fb5aea9/packages/dom/src/autoUpdate.ts

Which is not really necessary in case we just want the element to stick to the bottom corner of the element.

There's one more issue I need to figure out with the mobile styling compared to the old EmojiPicker we are trying to replace. The old version used to pop-up a dialog with a dimmed out background but the new version with the current configuration spans outside the screen with 375px wide screen (iPhone SE in Chrome developer tools) and about 40px gutters on both sides of the comment area.

But I'll continue investigation regarding this issue. Thank you for your outstanding support!

ahukkanen commented 2 years ago

I just tried the suggestions and here is a situation close to what I get in the actual application: https://jsfiddle.net/b32mLozh/

It may be better to look it at the full screen mode here: https://jsfiddle.net/b32mLozh/show

There are two problems with this approach:

  1. We would want the picker to be on top of the text area, at the very bottom-right corner
  2. We would want the picker to stay at the bottom corner and not jump around when you scroll up and down

This was the initial problem that I wanted to solve by forcing the absolute positioning what I ended up using.

I figured the mobile view in the previous version is something that is not implemented in the current popup picker and we probably need to extend the popup picker to fix the mobile positioning. The current "jump around" functionality actually works quite well for mobile but it's not what is desired on desktop.