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

Dealing with event handler bubbling #268

Closed
richard-to opened this issue May 17, 2024 · 4 comments · Fixed by #913
Closed

Dealing with event handler bubbling #268

richard-to opened this issue May 17, 2024 · 4 comments · Fixed by #913
Assignees

Comments

@richard-to
Copy link
Collaborator

The scenario that brought this up was the use of a Modal where I wanted to be able to close the modal when clicking on the background.

In this scenario, if I click on the modal content to say copy and paste some text, the Modal will still close since the Modal background click event gets triggered.

This seems like expected behavior. I was wondering how we would prevent this from happening? Would we need to add a noop click event at the modal container/content. Would that prevent the event on the background from being triggered, or would both events get propagated? I can't quite remember what the behavior is but I'll test it out when I get the chance.

@wwwillchen
Copy link
Collaborator

Interesting, I think if we called event.stopPropagation then it would stop the event bubbling which would fix the issue you've described. We'd probably want to be consistent across all of our components.

One tricky part is how the Mesop app developer conveys this intent. For example, the following does not work:

def on_click(e):
  # by the time you call this, it's too late
  e.stop_propagation()

Instead, we'd probably need an API more like

@me.event_handler(stop_propagation=True)
def on_click(e):
  ...

Essentially we need to know when the function is binded at the component call site whether to stop future event propagations or not.

This seems a bit of a complex API and I'm wondering now if we should always call stopPropagation on behalf of users. It seems like the default behavior of always stopping propagation is probably what most developers want and then if people wanted to keep propagation, we could add this as a future feature (using an API like the above).

@richard-to
Copy link
Collaborator Author

Yeah, I guess there's two different issues being discussed here.

I think in my modal example I described the issue wrong since it's not bubbling up the events. Though I agree that the stopPropagation thing could become an issue. I agree also that stop propagation seems like a reasonable default behavior. I'm trying to think if there are more complicated stop propagation edge cases.


Back to the modal issue. The problem is that the click event is being triggered against the child component which I think is reasonable behavior in many cases, such as:

with me.box(on_click=on_click):
    me.text("Click me")

So the modal case, I think in javascript, I think I typically would check the event target. Is the element the modal div or is this the modal container? If modal div, then close. If not, then noop.

@wwwillchen
Copy link
Collaborator

I see what you're saying right now, basically it's the distinction between event.target and event.currentTarget. Right now, for each event, the key is effectively the event.currentTarget (where the event handler is attached), but it sounds like sometimes you want event.target - we could add a second property to MesopEvent like target_key.

I'm also wondering if we should actually have modal/dialog be a first-class component. It's kind of a tricky thing to implement correctly (even aside from this issue). I've filed #278

@wwwillchen
Copy link
Collaborator

Recap today's discussion: a minimal addition to the API would be add a bool property like is_target to ClickEvent which would allow you to distinguish whether the box was clicked or if a descendant of the box was clicked. This is particularly useful for the backdrop use case.

@richard-to richard-to self-assigned this Aug 31, 2024
richard-to added a commit to richard-to/mesop that referenced this issue Aug 31, 2024
Usage of is_target mainly works for me.box.

For me.button / me.content_button it will almost always be false since
the buttons are composed of other sub elements. There may be cases where
it is true.

Updated dialog usages to utilize `is_target`.

Closes google#268
richard-to added a commit that referenced this issue Sep 1, 2024
* Add is_target to Click event

Usage of is_target mainly works for me.box.

For me.button / me.content_button it will almost always be false since
the buttons are composed of other sub elements. There may be cases where
it is true.

Updated dialog usages to utilize `is_target`.

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

Successfully merging a pull request may close this issue.

2 participants