aaronc / freactive

High-performance, pure Clojurescript, declarative DOM library
http://documentup.com/aaronc/freactive
Eclipse Public License 1.0
387 stars 27 forks source link

appendChild won't work with Polymer 1.0 "shady DOM" #45

Open sparkofreason opened 9 years ago

sparkofreason commented 9 years ago

See this discussion.

Bottom line: for performance reasons, Polymer 1.0 gives the option of not polyfilling the full shadow DOM spec by overwriting DOM API's like appendChild. For at least some Polymer elements, we need to execute slightly different code to ensure that things render correctly, allowing the element to be notified when its content has changed. I played with this a bit in my local copy of freactive, and was able to make it work by adding a register-append-child function in freactive.dom:

(def ^:private append-child-fns
  (atom {:default (fn [parent node] (.appendChild parent node))}))

(defn register-append-child
  [tag append-child-fn]
  (swap! append-child-fns assoc (.toUpperCase (name tag)) append-child-fn))

(defn- do-append-or-insert-child [parent node before]
  (if before
    (.insertBefore parent node before)
    (if-let [append-child-fn (@append-child-fns (.-tagName parent))]
      (append-child-fn parent node)
      ((@append-child-fns :default) parent node))))

And then calling it like this:

(dom/register-append-child :paper-button 
                           (fn [parent node] 
                             (.appendChild (js/Polymer.dom parent) node)))

The design maybe needs some more thought. Seems onerous to have to register for every element, maybe makes more sense to provide a predicate function. Probably could streamline performance a bit as well. I assume insertBefore has the same issue, but haven't tested.

aaronc commented 9 years ago

Okay, well it seems like a matter of changing all calls to DOM fn's to use Polymer.dom(parent) instead right? We just need to provide a wrapper method for all native DOM calls and then a plugin fn enable-polymer-dom! that replaces them with Polymer's version. Would that work?

sparkofreason commented 9 years ago

Yes, that sounds right.

aaronc commented 9 years ago

So, in general I've tried to design this system to be as pluggable as possible. If all native DOM calls need to be replaced, we just isolate them by default and then "patch" them if needed. Also, check out the new DOM implementation a little - everything is now based on IVirtualElement. If some element needs to do some wildly custom stuff it can do it as long as other elements see it as IVirtualElement. as-velem provides the syntatic sugar by converting all "hiccup vectors" to DOMElement and text to DOMTextNode - but maybe Polymer stuff would benefit from other conversions. The core of the library is really ReactiveElement and ReactiveElementCollection in freactive.ui-common plus ReactiveAttribute in freactive.core - none of which care at all which renderer they're using. Just some food for thought...

sparkofreason commented 9 years ago

A potential issue with patching DOM calls might occur if you were using more than one Web Components-ish library. I don't know that anyone else is doing something like Polymer shady-DOM right now, but it seems likely to pop up, and then we'd need a way to dispatch based on the library used to create the element. Does IVirtualElement solve this? Could we perhaps render the correct IVirtualElement implementation based on a tag namespace or prefix?

aaronc commented 9 years ago

Yes, see: https://github.com/aaronc/freactive/blob/develop/src/freactive/dom.cljs#L506-L513. register-node-prefix! will add a handler fn for a ns prefix: https://github.com/aaronc/freactive/blob/develop/src/freactive/dom.cljs#L95-L96 .

On Wed, Jun 3, 2015 at 1:28 PM, Dave Dixon notifications@github.com wrote:

A potential issue with patching DOM calls might occur if you were using more than one Web Components-ish library. I don't know that anyone else is doing something like Polymer shady-DOM right now, but it seems likely to pop up, and then we'd need a way to dispatch based on the library used to create the element. Does IVirtualElement solve this? Could we perhaps render the correct IVirtualElement implementation based on a tag namespace or prefix?

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-108536203.

aaronc commented 9 years ago

Do you see any need to dispatch on non-prefixed tags? dom-element could have custom dispatch potentially... - https://github.com/aaronc/freactive/blob/develop/src/freactive/dom.cljs#L486

On Wed, Jun 3, 2015 at 1:31 PM, Aaron Craelius aaroncraelius@gmail.com wrote:

Yes, see: https://github.com/aaronc/freactive/blob/develop/src/freactive/dom.cljs#L506-L513. register-node-prefix! will add a handler fn for a ns prefix: https://github.com/aaronc/freactive/blob/develop/src/freactive/dom.cljs#L95-L96 .

On Wed, Jun 3, 2015 at 1:28 PM, Dave Dixon notifications@github.com wrote:

A potential issue with patching DOM calls might occur if you were using more than one Web Components-ish library. I don't know that anyone else is doing something like Polymer shady-DOM right now, but it seems likely to pop up, and then we'd need a way to dispatch based on the library used to create the element. Does IVirtualElement solve this? Could we perhaps render the correct IVirtualElement implementation based on a tag namespace or prefix?

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-108536203.

sparkofreason commented 9 years ago

Let me play with it. My first impression is that register-node-prefix! and IVirtualElement covers it.

aaronc commented 9 years ago

Okay. Probably because a custom element can use it's own "as-velem" for children that may cover it.

On Wed, Jun 3, 2015 at 2:16 PM, Dave Dixon notifications@github.com wrote:

Let me play with it. My first impression is that register-node-prefix! and IVirtualElement covers it.

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-108554365.

sparkofreason commented 9 years ago

Looking at the implementation of DOMElement as a starting point. It seems that PolymerDOMElement would share most of that implementation. Could we instead make the DOM API an argument to DOMElement? Or maybe that's what you meant by patching DOM calls.

aaronc commented 9 years ago

Yeah, we could take either approach. What do you think is better? I don't know enough about Polymer/custom element internals personally.

FYI, added some doc strings here: https://github.com/aaronc/freactive/blob/develop/src/freactive/ui_common.cljs#L5-L34

On Wed, Jun 3, 2015 at 2:30 PM, Dave Dixon notifications@github.com wrote:

Looking at the implementation of DOMElement as a starting point. It seems that PolymerDOMElement would share most of that implementation. Could we instead make the DOM API an argument to DOMElement? Or maybe that's what you meant by patching DOM calls.

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-108564063.

aaronc commented 9 years ago

Due to other issues that I am currently working through, I will probably decide to pass a "native dom wrapper" through the whole tree so this may provide a way to deal with this.

sparkofreason commented 9 years ago

From what I can tell from their docs and code, that seems like it will be the key piece for Polymer 1.0.

aaronc commented 9 years ago

So regarding Polymer's manipulation of DOM elements. If freactive uses Polymer's shady DOM API, wouldn't the DOM appear unchanged to freactive? I think everything would work fine if this were the case. The only real issue I can see would potentially be with using freactive's element sequences - in that if other DOM elements are inserted, then the ordering of elements could maybe be messed up. But again, if shady DOM hides all this manipulation, there probably wouldn't be an issue...

sparkofreason commented 9 years ago

I guess the key question is whether freactive is going to expect things to maintain a specific place in the DOM, i.e. in the <paper-button> example, are you going to care that the <span> is not actually the immediate child of <paper-button> in the final version of the DOM? If you're always holding references to DOM elements and working directly with them (which I think has been the case so far), it seems like it should be fine. Same for lists, under the assumption that you don't care that maybe the whole list of elements has been moved in the DOM, as long as the items are kept in the same order.

When I get there with Cereus, I'll use the freactive interfaces for moving nodes, and that should keep things consistent.

aaronc commented 9 years ago

This implementation always holds its own references to nodes and keeps track of where it has placed nodes in the DOM. It would care if a list of children were moved if the reactive collection binding is being used because now things won't get inserted into the right places. But my understanding of Polymer's DOM API is that it would make it appear that things are the same place freactive put them, right? If that's the case then it wouldn't care what's going on in the final DOM.

sparkofreason commented 9 years ago

Agreed.

aaronc commented 9 years ago

The implementation for moving around nodes is here: https://github.com/aaronc/freactive/blob/develop/src/freactive/ui_common.cljs#L210-L234. This stuff is still sort of a work in progress, but the main idea is that this API would be used for managing movement: https://github.com/aaronc/freactive.core/blob/master/src/clojure/freactive/core.cljc#L877-L912 and would more or less expect that one projection source is managing a single projection target. Yes, it would be possible to move nodes between projection sources (for say drag and drop) if they knew how to do that. ReactiveElementCollections can be nested - no need for wrapper nodes.

sparkofreason commented 9 years ago

Looks good. Presumably, when I want to do something similar as Polymer, moving things as rendered in "logical" DOM to another place in the actual DOM, I would implement IVirtualElement to ensure that the parent gets notified when the logical child DOM changes.

BTW, I verified freactive works find doing mount!/unmount! on the native shadow root. So you're future-proof ;-)

sparkofreason commented 9 years ago

So I think I have a relevant case for projection. The Polymer <paper-dialog> element uses z-order to ensure that the dialog appears on top of everything else. However, if <paper-dialog> is a child of another element with z-order set then it doesn't work. Their recommendation is to make the <paper-dialog> a child of <body> if possible. I could make a custom element to do this directly via DOM API, but from what you described it sounds like I could rig it so I could put <paper-dialog> in a natural location in the logical DOM, but have it render as a child of <body>. Is that correct?

aaronc commented 9 years ago

You could make your own custom virtual element that inserts it where you like. Projection is more for binding collections. On Wed, Jun 10, 2015 at 8:40 PM Dave Dixon notifications@github.com wrote:

So I think I have a relevant case for projection. The Polymer

element uses z-order to ensure that the dialog appears on top of everything else. However, if is a child of another element with z-order set then it doesn't work. Their recommendation is to make the a child of if possible. I could make a custom element to do this directly via DOM API, but from what you described it sounds like I could rig it so I could put in a natural location in the logical DOM, but have it render as a child of . Is that correct? — Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-110955658.
sparkofreason commented 9 years ago

I guess this case is actually covered if we have the ability to specify the DOM API implementation.

Maybe a nice-to-have for this would be the ability to register the DOM API implementation by tag name.

aaronc commented 9 years ago

Well, I'm not sure it would be covered by the DOM API - does Polymer take care of appending it to the body? If not, an IVirtualElement could do that. Once we abstract the DOM API, we can figure out different ways to swap it in and out. I'm thinking that for Polymer we could just have a mount-polymer! fn which causes all child elements to use its DOM API. Would that work?

On Thu, Jun 11, 2015 at 9:48 AM, Dave Dixon notifications@github.com wrote:

I guess this case is actually covered if we have the ability to specify the DOM API implementation.

Maybe a nice-to-have for this would be the ability to register the DOM API implementation by tag name.

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-111141625.

sparkofreason commented 9 years ago

I was going to provide my own implementation of the DOM API which did the magic for the insert/append cases, otherwise called through to Polymer's DOM API. But thinking about it, the semantics might get sticky on how you associate a particular DOM API implementation with a particular tag, e.g. does it only apply to operations involving the element itself, or the element and it's children? So IVirtualElement is probably clearer.

Would mount-polymer! mean that the Polymer API's are used on all descendants? I'm guessing some other component frameworks play similar API tricks - Cereus certainly will, I think document-register-element does (at least for IE8 polyfills), maybe famo.us, etc. Would virtual elements provide more mix/match flexibility, for example a Polymer element could have Cereus elements as children?

For the Polymer case, I was thinking register-node-prefix! for the polymer namespace, and having the handler figure out which IVirtualElement impl to use based on the tag name. Similarly, Cereus could provide it's own namespace/handler/virtual elements, etc.

aaronc commented 9 years ago

Yeah, there are many different ways it could be done. One way is to have DOMElement, DOMTextNode, etc. take a DOM API instance as an argument. Each DOM API could have its own as-velem fn. Or you can create your own DOMElement, etc. implementations that use Polymer's API without changing the way DOMElement, etc. are now and a special as-velem fn and mount fn for polymer... Or you can use node prefix plugins. Once you're within a plugin, it gets to decide how all its children are handled - so you could have a [:polymer/polymer-root ...] node that inside of that uses polymer's virtual elements and DOM API and as-velem fn... It's flexible. I just tried to make ReactiveAttribute, ReactiveElement and ReactiveElementSequence as generic as possible... I could make DOMElement more generic too or maybe not... The main thing to keep in mind is that it's as-velem that decides how a given syntax (i.e. hiccup vector, etc.) gets converted to a virtual element - you can have different as-velem implementations and all the types in ui-common take it as a parameter (velem-fn).

On Thu, Jun 11, 2015 at 12:41 PM, Dave Dixon notifications@github.com wrote:

I was going to provide my own implementation of the DOM API which did the magic for the insert/append cases, otherwise called through to Polymer's DOM API. But thinking about it, the semantics might get sticky on how you associate a particular DOM API implementation with a particular tag, e.g. does it only apply to operations involving the element itself, or the element and it's children? So IVirtualElement is probably clearer.

Would mount-polymer! mean that the Polymer API's are used on all descendants? I'm guessing some other component frameworks play similar API tricks - Cereus certainly will, I think document-register-element https://github.com/WebReflection/document-register-element does (at least for IE8 polyfills), maybe famo.us, etc. Would virtual elements provide more mix/match flexibility, for example a Polymer element could have Cereus elements as children?

For the Polymer case, I was thinking register-node-prefix! for the polymer namespace, and having the handler figure out which IVirtualElement impl to use based on the tag name. Similarly, Cereus could provide it's own namespace/handler/virtual elements, etc.

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-111201639.

sparkofreason commented 9 years ago

Ok, I think I'm starting to get it. Seems like implementing IVirtualElement is for the case where you want to completely take over the DOM management from that point forward, maybe if you were using something like React Material UI.

The case of HTML custom elements is more light weight. A Polymer element needs to push in the DOM API, but otherwise could be handled by the as-velem, as it's really just an HTMLElement, and its children should be processed the same as any other "normal" DOM. Cereus is covered in a similar manner. Could we have as-velem take the DOM API as an argument, and the tag-handler for a namespace just pass as-velem? Seems pretty simple then. An element like [:polymer/paper-button] is processed by the polymer tag handler, strips the namespace from the tag, and calls as-velem with the updated element spec and the appropriate DOM API.

aaronc commented 9 years ago

Yeah implementing IVirtualElement is for when you want to basically make "anything" look like a logical child - could be one real element, several real elements, an element somewhere else - you could even make your own shadow DOM if you wanted.

We could have:

(defn as-velem* [native-api]
 (fn as-velem [x]
   ...))

(def as-velem (velem* native-dom))
(def as-velem-polymer (velem* polymer-dom))
aaronc commented 9 years ago

With this approach all child elements would use the same as-velem unless some plugin handler changed that...

sparkofreason commented 9 years ago

That should do it.

sparkofreason commented 9 years ago

I'm at a good spot in my project where I could tackle this, if you want some help.

aaronc commented 9 years ago

That would be great. I will try to send a few notes later today on a suggested DOM API protocol. On Thu, Jun 18, 2015 at 11:37 AM Dave Dixon notifications@github.com wrote:

I'm at a good spot in my project where I could tackle this, if you want some help.

— Reply to this email directly or view it on GitHub https://github.com/aaronc/freactive/issues/45#issuecomment-113195612.

aaronc commented 9 years ago

So this is what I've had in mind for a native API:

(defprotocol INativeAPI
 (native-queue [this f] "Queues the no-arg fn f to be run in the application thread of this UI platform")
 (native-create-element [this elem-ns elem-name])
 (native-create-text-node [this text])
 (native-bind-attrs! [this elem attr-map]
  "Binds a map of attributes to the element and returns an fn (or something that implements IFn) that when called with a single argument representing the new map of attributes will rebind the attributes on this element. This fn object map also have a plain javascript dispose member fn that will be called when the element is disposed to perform any clean-up.")
 (native-insert [this parent elem before?])
 (native-replace [this new-elem old-elem])
 (native-remove [this elem])
 (native-element? [this x])
 (native-text-node? [this x])
 (native-set-text-node! [this node text]))

The most complex thing here may be attribute binding. The code for this now is actually designed to be as generic as possible and it should be pretty straight-forward to adapt it - if it's not evident how this works let me know.

With this sort of API, it may actually be possible to work with other UI frameworks (for instance javascript bindings for iOS or Android native components). In that case DOMElement would become just Element as it would be pretty generic.

One other consideration for Polymer is the .flush fn - we can create a hook for this in the render loop if it makes sense.

sparkofreason commented 9 years ago

Good start, I like how this is shaping up. I think we probably also need functions to get (and set, where applicable) DOM properties like parentNode.

Is there a reason to do native-bind-attrs! like this, or could we just have native-get-attribute and native-set-attribute?

Do we want pre and post render hooks? I have a couple of ideas for the future that I think would use this, particularly using cassowary.js for constraint-based layout.

I did a quick and dirty attempt earlier today, diff here: https://github.com/aaronc/freactive/compare/develop...allgress:develop. To avoid passing the DOM API through all of the relevant functions, I followed Polymer's example and stuck it on the element as a property, and registered element create APIs which in turn would place the appropriate DOM API on the element. There's a little subtlety here, because in order to get parentNode right I also had to hijack appendChild. Incomplete implementation for Polymer below.

(defn- polymer-dom-api
  [node]
  (if-let [dom-api (.-__domApi node)]
    (let [append-child (.-appendChild dom-api)]
      (set! (.-appendChild dom-api)
            (fn [child]
              (set! (.-freactive-dom-api child) polymer-dom-api)
              (.call append-child dom-api child)))
      dom-api)
    node))

(def create-polymer-element
  (reify
   ui/ICreateElement
   (createElement [this tag]
     (let [el (.createElement js/document tag)]
       (set! (.-freactive-dom-api el) polymer-dom-api)
       el))
    (createElementNS [this tag ns-uri]
      (let [el (.createElementNS js/document tag ns-uri)]
        (set! (.-freactive-dom-api el) polymer-dom-api)))))

Registration was then done via

(register-create-element! :paper-button util/create-polymer-element)

Did some testing and verified the works correctly with <paper-button>. Seems like this approach would work with INativeAPI implementations as well. parentNode remains the tricky bit, need to think more if this is something which can be abstracted away or if all INativeAPI implementations need to be responsible for correctly wiring up parentNode on children.

sparkofreason commented 9 years ago

Clarifying my question on native-bind-attrs!: since the current implementation is pretty generic, does it suffice to supply the native get/set methods?

Also, I think the parentNode API needs to separated. The function used to look up the parent node really belongs to the parent node API. The child could have a different API for managing it's children. Example:

[:paper-button
  [:span "I'm a button"]]

The paper-button element would use the Polymer API to manage its children, and span would use the native browser API. But to get the right parent of the span, it needs to use the Polymer API.

sparkofreason commented 9 years ago

After digging through the code, I'm going to guess at the answer to the native-bind-attrs! question: it's to handle special cases like :style and :events.

sparkofreason commented 9 years ago

First try using INativeAPI now pushed to the Allgress fork of freactive. Works as far as I can tell, including our reasonably non-trivial app running Polymer 1.0. Still some details to be ironed out, I think, particularly around attribute binding. I've only implement get/set/remove attribute functions on INativeAPI, and am using your existing attribute binding code. For normal attributes, I would think this code works for all browser-based DOM APIs. Style and event attributes are currently unwrapped, and Polymer at least doesn't seem to care about these.

aaronc commented 9 years ago

So, I'm not quite sure why the parentNode needs a separate API. Are you saying that some nodes would use Polymer's DOM API and some would use the browser's API within the same sub-tree?

sparkofreason commented 9 years ago

Currently I have it so each element gets a reference to its native API, e.g. in the example above the [:span] uses the regular DOM API, while the [:paper-button] uses the Polymer DOM API. The only difference is that when getting the span's parent, it uses the Polymer API. Wiring this up is now handled by freactive.dom, so API implementers don't need to know about it.

Theoretically, I think Polymer's DOM API could be used everywhere, but there will be some performance penalty, which is why they kept it separate in 1.0 rather than just overwriting the native definitions as was done in Polymer 0.5.