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

A blocked redraw from an event handler lower in the tree cascades upwards. #1850

Closed
dwaltrip opened this Issue May 10, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@dwaltrip
Copy link

dwaltrip commented May 10, 2017

If two nested divs both have onclick handlers, and the inner div blocks the redraw with e.redraw = false, then neither of the onclick handlers will result in a redraw.

It seems that the browser passes the same event object to all handlers up the tree, as part of the bubbling process. Thus, both handlers will have an event object with redraw set to false, although it was only set to false in the inner handler.

This cascading of blocked redraws only occurs in one direction, up the tree. If, instead, we set e.redraw = false in the outer handler, then a click event on the inner div will trigger a redraw after the inner handler, but not again after the outer handler.

This surprised me at first. In some situations, it creates the unfortunate requirement of needing to know whether event handlers lower down in the tree are blocking the redraw.

I'm not sure what the ideal behavior would be, but wanted to at least document the issue. Adding a note to the official documentation might be all that is needed.

Perhaps the more important lesson is to generally avoid designs or logic that depends on redraws being blocked, and use event.redraw = false only if absolutely needed.

Here's a codepen to illustrate: https://codepen.io/anon/pen/xdYdXM?editors=0011

@pygy pygy self-assigned this May 10, 2017

@pygy pygy added the bug label May 10, 2017

@pygy pygy removed their assignment May 10, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 10, 2017

Thanks for the report, this is at the very least a gotcha, and IMO a bug that should be fixed.

We could set event.redraw to true before passing it to the handler. That way, setting it to false in a handler will not contaminate other ones "uptree".

@gyandeeps

This comment has been minimized.

Copy link
Contributor

gyandeeps commented May 10, 2017

I can do the work if nobody is working on this.

@dwaltrip

This comment has been minimized.

Copy link

dwaltrip commented May 11, 2017

I've linked the lines below I identified as relevant when I was looking into it =) I think the fix recommended by @pygy might be a one-liner? Of course then we have to make sure everything else works as expected. I'm guessing this doesn't affect the rest of the render/render.js code, but then again I don't understand the nitty gritty details of the core renderer very well.

https://github.com/MithrilJS/mithril.js/blob/next/api/redraw.js#L28-L30
https://github.com/MithrilJS/mithril.js/blob/next/render/render.js#L15
https://github.com/MithrilJS/mithril.js/blob/next/render/render.js#L566

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 11, 2017

The fix is a one liner, yes. I'd reset e.redraw to undefined in api/redraw.js, in order to keep render/render.js redraw-unaware.

We'll also need a test. I don't know if the DOM mock code implements event bubbling. If it doesn't checking that e.redraw has been reset after a timeout is better than nothing.

Edit: @gyandeeps thanks for the offer, PR most welcome!

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 11, 2017

@pygy

I don't know if the DOM mock code implements event bubbling.

It currently does not. But once we get sync redraws, this'll be much easier to test.

@pygy pygy added the needs test case label Jul 5, 2017

pygy added a commit to pygy/mithril.js that referenced this issue Jul 6, 2017

@pygy pygy closed this in 198f9ca Jul 6, 2017

pygy added a commit that referenced this issue Jul 6, 2017

Merge pull request #1890 from pygy/fix-1850
Reset e.redraw when it was set to `false`. Fixes #1850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment