Add support for custom elements to MDV

Add support for custom elements to MDV
1. Custom elements should be able to override Node.bind
2. Custom elements should be able to intercept attributes set by the binding system

R=blois@google.com, sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=22650

[Jennifer Messerly, 2013-05-12 02:52:41]

I need this to make the nice web_ui <custom-element foo="{{bar}}"> syntax work.

As you can see from the tests, it's kind of boilerplate-y to use by itself. I'm
planning to have a more convenient impl on the Web UI side.

[Siggi Cherem (dart-lang), 2013-05-13 16:42:13]

lgtm!

Nice! For web UI, I assume everything under bind/unbind/unbindAll in
MyCustomElement would be autogenerated?

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
sdk/lib/html/dart2js/html_dart2js.dart:26511: * and [Node.unbindAll] like
like ... ?

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
File tests/html/custom_element_bindings_test.dart (right):

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:240: final
AttributeMapWrapper<String, String> attributes;
consider adding a comment here reinforcing what you said in the class comment
(that this [attributes] overrides the one in Element)

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
tools/dom/src/TemplateBindings.dart:549: _nodeOrCustom(node) => node is Element
? node.xtag : node;
interesting, what about a custom element that extends from <template>? Seems
like 'bind' in the custom element takes priority, but I wonder if it should be
applied to both?

[blois, 2013-05-13 17:45:13]

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
sdk/lib/html/dart2js/html_dart2js.dart:26514: _nodeOrCustom(node) => node is
Element ? node.xtag : node;
Can this have a return type?

Also, possibly make it a static method somewhere? Since this is in a ginormous
library it's nice to avoid library functions when possible- they tend to get
lost.

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
File tests/html/custom_element_bindings_test.dart (right):

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:25: document.body.nodes.add(testDiv
= new DivElement());
Better to use:
document.body.append()

(not important here, but good to get in the habit).

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:124: for (var e in
fragment.queryAll('my-custom-element')) {
What will happen when custom elements are registered after they have been bound?
Will this be supported in JS?

[blois, 2013-05-13 17:45:29]

lgtm

[Jennifer Messerly, 2013-05-13 19:42:41]

Thanks guys. Here are some thoughts. Let me know if this sounds good.

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
sdk/lib/html/dart2js/html_dart2js.dart:26511: * and [Node.unbindAll] like
On 2013/05/13 16:42:13, Siggi Cherem (dart-lang) wrote:
> like ... ?

hah, not sure what happened. Fixed :)

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
sdk/lib/html/dart2js/html_dart2js.dart:26514: _nodeOrCustom(node) => node is
Element ? node.xtag : node;
On 2013/05/13 17:45:13, blois wrote:
> Can this have a return type?

It cannot, unless the type is Object :). I had "Node" there but sometimes
"CustomBinding" is used. :(

One thing I'd like to fix (at some point) is to either not use CustomBinding
like that, or at least create an interface with bind/unbind/unbindAll.

> Also, possibly make it a static method somewhere? Since this is in a ginormous
> library it's nice to avoid library functions when possible- they tend to get
> lost.

All methods in this file are top-level, so I presume you mean convert the entire
file? :)

Although it technically violates rule #1 of the Dart style guide :), I'm
sympathetic that dart:html has special constraints. Here's a proposal: after
this CL lands I move all top-levels in TemplateBindings to be static methods on
some class ("_Bindings"?). Sound good?

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
File tests/html/custom_element_bindings_test.dart (right):

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:25: document.body.nodes.add(testDiv
= new DivElement());
On 2013/05/13 17:45:13, blois wrote:
> Better to use:
> document.body.append()
> 
> (not important here, but good to get in the habit).

agreed. I didn't actually write this one though :), was a copy+paste from
another test.

I looked at fixing all of tests/html but it's used in many places. Perhaps a
follow up CL with search+replace?

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:124: for (var e in
fragment.queryAll('my-custom-element')) {
On 2013/05/13 17:45:13, blois wrote:
> What will happen when custom elements are registered after they have been
bound?
> Will this be supported in JS?

good question, I'm not sure offhand. I'm more focused here on testing the case
where the element is already registered :). That's the nice thing about
"instanceCreated" -- it can upgrade elements before the bindings run.

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
tools/dom/src/TemplateBindings.dart:549: _nodeOrCustom(node) => node is Element
? node.xtag : node;
On 2013/05/13 16:42:13, Siggi Cherem (dart-lang) wrote:
> interesting, what about a custom element that extends from <template>? Seems
> like 'bind' in the custom element takes priority, but I wonder if it should be
> applied to both?

Not sure I understand...

What this does is try and simulate inheriting Element. It needs to be good
enough that we can implement binding to an element's fields. It's definitely not
perfect. You can show the leakiness by simply calling .bind yourself on the
underlying Element; that will skill the custom impl. That's why we need real
inheritance.

It's quite possible something doesn't work if you extend <template> element. I
haven't yet seen a compelling use case for extending template, so I'm inclined
not to put much effort into polyfilling it.

[Jennifer Messerly, 2013-05-13 19:43:45]

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14795005/diff/1/tools/dom/src/TemplateBinding...
tools/dom/src/TemplateBindings.dart:549: _nodeOrCustom(node) => node is Element
? node.xtag : node;
On 2013/05/13 19:42:41, John Messerly wrote:
> On 2013/05/13 16:42:13, Siggi Cherem (dart-lang) wrote:
> > interesting, what about a custom element that extends from <template>? Seems
> > like 'bind' in the custom element takes priority, but I wonder if it should
be
> > applied to both?
> 
> Not sure I understand...
> 
> What this does is try and simulate inheriting Element. It needs to be good
> enough that we can implement binding to an element's fields. It's definitely
not
> perfect. You can show the leakiness by simply calling .bind yourself on the
> underlying Element; that will skill the custom impl. That's why we need real
> inheritance.
> 
> It's quite possible something doesn't work if you extend <template> element. I
> haven't yet seen a compelling use case for extending template, so I'm inclined
> not to put much effort into polyfilling it.

oops, "skill the custom impl" should be "skip the custom impl".

[Jennifer Messerly, 2013-05-13 20:02:40]

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
File tests/html/custom_element_bindings_test.dart (right):

https://codereview.chromium.org/14795005/diff/1/tests/html/custom_element_bin...
tests/html/custom_element_bindings_test.dart:25: document.body.nodes.add(testDiv
= new DivElement());
On 2013/05/13 19:42:41, John Messerly wrote:
> On 2013/05/13 17:45:13, blois wrote:
> > Better to use:
> > document.body.append()
> > 
> > (not important here, but good to get in the habit).
> 
> agreed. I didn't actually write this one though :), was a copy+paste from
> another test.
> 
> I looked at fixing all of tests/html but it's used in many places. Perhaps a
> follow up CL with search+replace?

update. I fixed this one manually, also sent out:
https://codereview.chromium.org/14883008/

[Jennifer Messerly, 2013-05-13 20:24:36]

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/14795005/diff/1/sdk/lib/html/dart2js/html_dar...
sdk/lib/html/dart2js/html_dart2js.dart:26514: _nodeOrCustom(node) => node is
Element ? node.xtag : node;
On 2013/05/13 19:42:41, John Messerly wrote:
> On 2013/05/13 17:45:13, blois wrote:
> > Can this have a return type?
> 
> It cannot, unless the type is Object :). I had "Node" there but sometimes
> "CustomBinding" is used. :(
> 
> One thing I'd like to fix (at some point) is to either not use CustomBinding
> like that, or at least create an interface with bind/unbind/unbindAll.
> 
> > Also, possibly make it a static method somewhere? Since this is in a
ginormous
> > library it's nice to avoid library functions when possible- they tend to get
> > lost.
> 
> All methods in this file are top-level, so I presume you mean convert the
entire
> file? :)
> 
> Although it technically violates rule #1 of the Dart style guide :), I'm
> sympathetic that dart:html has special constraints. Here's a proposal: after
> this CL lands I move all top-levels in TemplateBindings to be static methods
on
> some class ("_Bindings"?). Sound good?

follow up on static methods:
https://codereview.chromium.org/14634011/

[Jennifer Messerly, 2013-05-13 20:27:36]

Committed patchset #3 manually as r22650 (presubmit successful).