fix: Transfer event listeners to content DIV [#96643092]

[alexanderGugel]

Event listeners for non-bubbeling events are being listened to by adding an
event listener directly to the element corresponding to the renderable. When
setting the content, the listeners need to be transfered to the content DIV.

Fixes #263

[michaelobriena]

@DnMllr

[alexanderGugel]

@DnMllr Can you review this? This is IMO a pretty serious bug.

[jd-carroll]

Can we always guarantee that the content div will be the same size as the
container? (Or at least be the same size on the next frame)

Why is there a second div for content? I know "render" size presents some
challenges, but isn't there a way to accomplish this without nesting div's?
IMO would rather see the nested div go away...

On Monday, June 29, 2015, Alexander Gugel [email protected] wrote:

@DnMllr https://github.com/DnMllr Any update on this? This is IMO a
pretty serious bug.

—
Reply to this email directly or view it on GitHub
#283 (comment).

[alexanderGugel]

@jd-carroll

Can we always guarantee that the content div will be the same size as the
container? (Or at least be the same size on the next frame)

Yes.

• https://github.com/Famous/engine/blob/master/dom-renderers/DOMRenderer.js#L575
• https://github.com/Famous/engine/blob/master/dom-renderers/DOMRenderer.js#L542
• https://github.com/Famous/engine/blob/master/dom-renderers/DOMRenderer.js#L638

All edge cases should be handled properly.

Why is there a second div for content? I know "render" size presents some
challenges, but isn't there a way to accomplish this without nesting div's?
IMO would rather see the nested div go away...

We need a second DIV to preserve the ability to have a nested DOM while not pushing down subsequent children if a non-leaf node has content. Render size is handled properly.

[jd-carroll]

@alexanderGugel based on our conversation in #378, would it be necessary to transfer the listeners? Wouldn't it be possible to just maintain the listeners on multiple DOM elements?

As I'm typing this though, I think that would mean there would have to be some sort of stopImmediatePropagation and/or preventDefault logic somewhere. If so, I can see that both positively and negatively.

[michaelobriena]

@alexanderGugel I think this only solves part of the issue. If you are deeper than the content and you don't bubble we still fail. Curious to think if we should add this even though it doesn't really address the core issue.

[alexanderGugel]

@michaelobriena You are absolutely right. Unfortunately I don't see a way to address this issue without managing the DOMElement's content via some sort of virtual DOM representation (instead of innerHTML). The core issue IMO is that events are added on a per node basis.

[michaelobriena]

I think we can't do just this approach because it breaks our bubbling model. If you put it on the content DIV, then a child in the scene graph won't have the events bubbled up to the parent. This is a pretty big issue and reason enough to not make this change. I think, as we have talked about internally, we need a real templating system instead of strings.

Closing for now.

[alexanderGugel]

@michaelobriena

If you put it on the content DIV, then a child in the scene graph won't have the events bubbled up to the parent.

No. Because we only add the event listeners to the content DIV for events that don't bubble in the first place. Not adding them to the content DIV means they aren't available at all.

This PR fixes this issue.

[michaelobriena]

I know, but we break bubbling events like click if do this. So to solve the non bubbling case and bubbling, we need a bigger solution.

[alexanderGugel]

@michaelobriena Can you elaborate on that? click bubbles, therefore it shouldn't be affected at all by this change. E.g. https://github.com/Famous/engine/pull/283/files#diff-18e5f08a4c73b48f46cdd6ef9ff4fd63R177 is not being executed here, since the listener is not being added in the first place.

[michaelobriena]

Clicking on a child will not trigger a click on the parent if the parent element's listeners are on the content div.

[alexanderGugel]

@michaelobriena It won't trigger the event in either case, since we never add the listener to anything inside the content div.

[michaelobriena]

Closing since this doesn't address the core issue and would have to be refactored out when a full solution was implemented

[alexanderGugel]

@michaelobriena If you mean the fact that inner events can't be captured, that issue should have been addressed by da0f28d.