support appendChild() and insertBefore() #52

Closed
wkeese opened this Issue Mar 5, 2014 · 31 comments

Comments

Projects
None yet
5 participants
Owner

wkeese commented Mar 5, 2014

ViewStack sets up a listener on DOMNodeInserted, to support native calls to create ViewStack children such as Element.appendChild() , setting innerHTML , or calling insertAdjacentHTML(), rather than just supporting our custom Container.addChild() method.

Supporting Element.appendChild() is a nice API, especially since delite widgets are DOM nodes, so users may expect the standard Element methods to work.

However, we intentionally avoided listening to DOMNodeInserted in register.parse() due to performance concerns. The issue would be if one of the ViewStack's panes contained, for example:

  • a dgrid object that asynchronously gets populated with 2000 DOM nodes
  • a chart that gets populated with thousands of SVG nodes

If we do want to support appendChild() across the board then it may require changes in other code like Container and KeyNav.

@wkeese wkeese added the question label Mar 5, 2014

Owner

wkeese commented Mar 5, 2014

I'll create a benchmark to see how slow this is particularly on mobile devices.

cc @dmandrioli, @pruzand, @cjolif

Owner

cjolif commented Mar 5, 2014

Also if we don't support it across the board I would tend to say we should not support it at all to be consistent.

Contributor

asudoh commented Mar 6, 2014

DOM Mutation Events has performance concerns as Bill pointed out. As such it's an deprecated API, and (much) faster, asynchronous DOM Mutation Observer API took it over.

However, DOM Mutation Observer is not supported by IE10-. Looking at shims/polyfills is one way to address that, but unless we see a fast shim/polyfill we may want to bite the bullet and rely on explicit call to something like Container#addChild()?

Owner

cjolif commented Mar 6, 2014

We might want to support appendChild on platforms that support DOM Mutation Observer? And keep addChild. All code/sample would use addChild but people deploying only on modern platforms might be able to drop it if they want to? The day we would drop IE10 support we would then have everything in place?

Contributor

asudoh commented Mar 6, 2014

Touché. If I'm not misunderstanding what ViewStack does, we may:

  • For modern browsers, use DOM Mutation Observer to run _setChildrenVisibility() upon child node insertion, and ViewStack#addChild() just calls (DOM) appendChild()
  • For IE10-, put the responsibility of calling _setChildrenVisibility() on the shoulder of ViewStack#addChild()

This approach may bring an anciliary thought that we can auto-upgrade widgets using DOM Mutation Observer for modern browsers, but guide users to use register.parse() and register.upgrade() in our documents.

Owner

wkeese commented Mar 6, 2014

I like DOM Mutation Observer... IIUC you can watch changes to children but not deeper descendants? Besides IE9-10 though, I think some mobile devices may not support it... MDN and it has a question mark for Android.

One option is to use MutationObserver where available and DOMNodeInserted otherwise. The question is really performance on Android where MutationObserver is not supported.

I wrote a trivial benchmark for DOMNodeInserted. My results for inserting 2000 elements are:

  • Chrome 2ms ➡ 17ms
  • IE10: 25ms ➡ 30ms
  • My android phone: 36ms ➡ 188ms
  • iPad 2: 16ms ➡ 68ms
Owner

wkeese commented Mar 6, 2014

PS: I just noticed Akira wrote that the new API is "asynchronous" (something like Object.observe?). That might throw a wrench into things.

Owner

wkeese commented Mar 7, 2014

I updated my benchmark to be more realistic, and added tests for MutationObserver. The benchmark now has a container (like ViewStack) with a pane inside of it, and the DOM insertions happen on the pane.

On my Android phone, creating the DOMNodeInserted listener makes the time go from 450ms ➡ 860ms. That's not good. I also confirmed that my phone doesn't have MutationObserver. The France team though should check newer versions of Android.

On my iPad2, MutationObserver is supported. A MutationObserver on the container has no performance overhead, because it doesn't respond to DOM insertions on the pane inside the container. If I setup a MutationObserver on the pane, performance goes from 25ms ➡ 35ms.

Contributor

asudoh commented Mar 7, 2014

Bill is correct that one of the biggest performance issues with DOM Mutation Events is event propagation. The performance impact to ViewStack using DOM Mutation Events would depend on how complex DOM structure panes in ViewStack have.

I just noticed Akira wrote that the new API is "asynchronous" (something like Object.observe?).

Exactly. DOM Mutation Observer as well as Object.observe() runs callbacks as soon as microtask ends. This allows them to reduce running callbacks by feeding multiple change records to a callback, and therefore application can "squash" those changes to reduce the impact of DOM Mutation.

(For example, AFAICT from the code, running ViewStack#_setChildrenVisibility() seems needed only once even if code detects multiple child pane additions)

caniuse.com says that Android started supporting DOM Mutation Observer in 4.4. IIRC 4.4 is the first Android making Chrome its default browser?

Owner

wkeese commented Mar 7, 2014

caniuse.com says that Android started supporting DOM Mutation Observer in 4.4. IIRC 4.4 is the first Android making Chrome its default browser?

OK thanks... so as you said, if we support arbitrary methods of adding children to Container widgets (appendChild, etc.), we should use MutationObserver when available, and fallback to DOMNodeInserted otherwise.

That means that it will be slow on IE9-10 and old versions of Android. Android is the more worrisome case, because phones are much slower than desktop machines, and also because most IE users will be on IE11.

I guess I don't mind old Android being slow.

Owner

cjolif commented Mar 7, 2014

I guess I don't mind old Android being slow.

The problem is that Android 4.3 is not old? And apparently does not support Mutation Observer?

Owner

cjolif commented Mar 7, 2014

caniuse.com says that Android started supporting DOM Mutation Observer in 4.4. IIRC 4.4 is the first Android making Chrome its default browser?

Yes.

Owner

wkeese commented Mar 7, 2014

The problem is that Android 4.3 is not old?

By "old" I meant "not the newest version". The tradeoff is between supporting natural API's vs. slowing down IE<11 and Android<4.4.

Contributor

asudoh commented Mar 10, 2014

Enterprise customers tend to keep old IE and prevent Windows Update from upgrading it, to support their business applications that are designed for specific version(s) of IE. Though I have never used Android myself, what I have heard about Android is users have much less chance to get their Android versions updated? So I'd be timid about deliberately slowing down old IE/Android, and I see (as Bill has been doing) carefully seeing different shim/polyfill approaches and how costful they are wrt performance would be the best way to think about this topic.

Another idea that has come to my mind is having a public version of _setChildrenVisibility()... For old IE/Android that would call _setChildrenVisibility() and no-op of other browsers.

Owner

cjolif commented Mar 10, 2014

Yes, in particular 4.4 is only available as far as I know, at least for now, on Google devices. Meaning the rest of the world does not have access to it. We can't just ignore them.

Member

seb-pereira commented Mar 10, 2014

Right, even with the ongoing 4.4 deployment over the constructor's "flagships", most devices will never receive this upgrade from hardware manufacturers.

http://developer.android.com/about/dashboards/index.html shows 4.0 <= release < 4.4 represents more than 77%. Adoption of newest release of Android is slow.

Member

dmandrioli commented Mar 10, 2014

For methods (appendChild and insertAdjacentHTML) we may avoid to rely on events by overriding them:

            appendChild: dcl.superCall(function (sup) {
                return function (child) {
                    sup.apply(this, [child]);
                    this._setChildrenVisibility();
                };
            })

I tried with appendChild, it works at least on IE11, FF and Chrome. Need to be tested with insertAdjacentHTML and supported mobiles.

For the innerHTML case:

  1. Maybe there is a way to detect innerHTML assignments?
  2. If none and if overriding methods works on all platforms, maybe we can drop event-based solutions and say in Container documentation that the behaviour of setting innerHTML on a Container is not specified? This could be motivated by the fact that using innerHTML is known to be the worst performance way to add children to a node.
Contributor

asudoh commented Mar 11, 2014

Maybe there is a way to detect innerHTML assignments?

Though I haven't tried it myself, one idea that comes on top of my head is creating ECMAScript getter/setter for ViewStack#innerHTML.

Owner

wkeese commented Mar 24, 2014

Note that insertAdjacentHTML() might also be called on a child widget
with the beforeBegin or afterEnd options. But it's unlikely to be used
much at all.

Probably just supporting appendChild and insertBefore is sufficient.

@wkeese wkeese assigned wkeese and dmandrioli and unassigned wkeese Apr 8, 2014

Owner

wkeese commented Apr 8, 2014

From the 2014-4-8 meeting, we like the idea of supporting appendChild() and insertBefore() [in addition to our custom addChild() method, but not other less common ways of adding children.

So, we'll ask Damien if he can look into using MutationObserver where available, and overriding appendChild() and insertBefore() on other platforms (like Android < 4.4).

I guess that code would eventually end up back in delite/Container, if only for the part that calls startup() on the added children.

@wkeese wkeese changed the title from support Element.appendChild() etc. or just Container.addChild() to support Element.appendChild() and Element.insertBefore() Apr 8, 2014

@wkeese wkeese changed the title from support Element.appendChild() and Element.insertBefore() to support appendChild() and insertBefore() Apr 8, 2014

dmandrioli added a commit to dmandrioli/deliteful that referenced this issue May 6, 2014

ViewStack: appendChild and insertBefore now supported through Mutatio…
…nObserver or overriding methods instead of DomNodeInserted. Close #52.

@dmandrioli dmandrioli closed this in 0dfc373 May 6, 2014

Owner

cjolif commented May 6, 2014

@dmandrioli per decision above can you see if this can be moved up to delite/Container level so everyone can benefit from this?

Member

dmandrioli commented May 7, 2014

I'm not sure how to promote this code to Container. The ViewStack is a simple case because it just calls the same method for any DOM tree modification.
But a container developer may need:

  • to override and bypass the native super call to appendChild/insertBefore: in this case, the developer would necessarily override the methods
  • to be notified of DOM tree changes, in this case we could emit one event for both methods or one event per method. But the support would be incomplete on platforms without MutationObserver available.

If we just want to support appendChild/insertBefore, ignoring the benefit of using MutationObserver in the case of ViewStack, we may just override appendChild and insertBefore on Container. These implementations would just call the native methods and hold some documentation to be clear on supported APIs.

But I think we should postpone that question and re-ask again when a second Container will need this kind of service.

Owner

wkeese commented May 7, 2014

I think we should postpone that question and re-ask again when a second Container will need this kind of service.

I agree w/your philosophy that we should have (at least) two clients that want a feature before extracting it into a parent class. But in this case, Container itself already needs this functionality (in addition to Viewstack): whenever a new child is added to Container, Container needs to call startup() on that child.

dmandrioli added a commit to dmandrioli/deliteful that referenced this issue May 7, 2014

Member

dmandrioli commented May 7, 2014

Reopen because this could impact #109

@dmandrioli dmandrioli reopened this May 7, 2014

Owner

cjolif commented May 19, 2014

@dmandrioli what is the status on this, I'm not sure to exactly understand why this is re-opened? Does it introduce a regression? If yes maybe we should look into it now?

Member

dmandrioli commented May 19, 2014

The new approach (MutationObserver/method overriding when unavailable) has introduced a regression in dapp (#109). Since discussions are still ongoing about promoting some appendChild() insertBefore() support to DisplayContainer, a quick fix for this regression has been provided but we need to decide how to support appendChild() insertBefore() at DisplayContainer level to move forward on this. This issue is the place for this discussion.
I continue to think we could just rely on method overriding instead of MutationObserver.

Owner

wkeese commented May 19, 2014

@dmandrioli tells me that the "problem" with MutationObserver is that it notifies asynchronously (much like Object.observe()?), and dapp wasn't expecting that.

About implementing observation purely by overriding appendChild() and insertBefore(), we can try, but I'm skeptical that will work on all browsers. We tried a similar approach declaring custom setters for property names like tabIndex, and that didn't work, hence we ended up with code like https://github.com/ibm-js/delite/blob/master/Widget.js#L124.

Owner

wkeese commented May 19, 2014

PS: I do like the idea of using method overriding on all browsers, because it makes the behavior consistent. If it works on webkit then that's great. If it doesn't work, you might try setting the methods on the instance rather than in the prototype.

Member

dmandrioli commented May 19, 2014

Ok I will test methods overriding on all supported browsers. I would also
prefer a consistent behavior.

2014-05-19 15:14 GMT+02:00 Bill Keese notifications@github.com:

PS: I do like the idea of using method overriding on all browsers, because
it makes the behavior consistent. If it works on webkit then that's great.
If it doesn't work, you might try setting the methods on the instance
rather than in the prototype.


Reply to this email directly or view it on GitHubhttps://github.com/ibm-js/deliteful/issues/52#issuecomment-43500506
.

Member

dmandrioli commented Jul 22, 2014

Will be implemented at Container level: ibm-js/delite#168

@dmandrioli dmandrioli closed this Jul 22, 2014

Owner

wkeese commented Sep 3, 2014

I fixed ibm-js/delite#168 in ibm-js/delite@8d40e4f, using Adrian's suggestion to do a dcl override

appendChild: dcl.superCall(function (sup) {
    return function (child) {
        var res = sup.call(this, child);
        this._setChildrenVisibility();
        return res;
    };
}),

I filed #264 for the corresponding updates to ViewStack.

dmandrioli added a commit that referenced this issue Feb 6, 2015

ViewStack: appendChild and insertBefore now supported through Mutatio…
…nObserver or overriding methods instead of DomNodeInserted. Close #52.

wkeese pushed a commit to wkeese/deliteful that referenced this issue Mar 12, 2015

ViewStack: appendChild and insertBefore now supported through Mutatio…
…nObserver or overriding methods instead of DomNodeInserted. Close #52.

wkeese pushed a commit to wkeese/deliteful that referenced this issue Mar 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment