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

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

Open
sparkofreason opened this issue Jun 3, 2015 · 37 comments
Open

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

sparkofreason opened this issue Jun 3, 2015 · 37 comments

Comments

@sparkofreason
Copy link

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
Copy link
Owner

aaronc commented Jun 3, 2015

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
Copy link
Author

Yes, that sounds right.

@aaronc
Copy link
Owner

aaronc commented Jun 3, 2015

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
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 3, 2015

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 [email protected] 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
#45 (comment).

@aaronc
Copy link
Owner

aaronc commented Jun 3, 2015

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 [email protected]
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 [email protected]
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
#45 (comment).

@sparkofreason
Copy link
Author

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

@aaronc
Copy link
Owner

aaronc commented Jun 3, 2015

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 [email protected] 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
#45 (comment).

@sparkofreason
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 3, 2015

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 [email protected] 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
#45 (comment).

@aaronc
Copy link
Owner

aaronc commented Jun 3, 2015

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
Copy link
Author

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

@aaronc
Copy link
Owner

aaronc commented Jun 5, 2015

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
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 5, 2015

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
Copy link
Author

Agreed.

@aaronc
Copy link
Owner

aaronc commented Jun 5, 2015

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 11, 2015

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 [email protected] 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
#45 (comment).

@sparkofreason
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 11, 2015

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 [email protected]
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
#45 (comment).

@sparkofreason
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 11, 2015

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 [email protected]
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
#45 (comment).

@sparkofreason
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 11, 2015

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
Copy link
Owner

aaronc commented Jun 11, 2015

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

@sparkofreason
Copy link
Author

That should do it.

@sparkofreason
Copy link
Author

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

@aaronc
Copy link
Owner

aaronc commented Jun 18, 2015

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 [email protected]
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
#45 (comment).

@aaronc
Copy link
Owner

aaronc commented Jun 19, 2015

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
Copy link
Author

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: 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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

aaronc commented Jun 22, 2015

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
Copy link
Author

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.

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

No branches or pull requests

2 participants