Add Event delegation to Elements and groups of elements.

Event delegation
Fix for https://code.google.com/p/dart/issues/detail?id=6270

BUG=
R=blois@google.com, jmesserly@google.com

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

[Emily Fortuna, 2013-08-01 19:43:27]

https://codereview.chromium.org/21607003/diff/3001/sdk/lib/svg/dartium/svg_da...
File sdk/lib/svg/dartium/svg_dartium.dart (left):

https://codereview.chromium.org/21607003/diff/3001/sdk/lib/svg/dartium/svg_da...
sdk/lib/svg/dartium/svg_dartium.dart:3438: return this.getItem(index)(index);
This change is not mine. See https://codereview.chromium.org/20799006

https://codereview.chromium.org/21607003/diff/3001/tools/dom/src/EventStreamP...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/21607003/diff/3001/tools/dom/src/EventStreamP...
tools/dom/src/EventStreamProvider.dart:43: Stream<T> filter(String selector) =>
I'm calling this "filter" because it more clearly describes what it does, but we
can go back to calling it delegate instead if that's the more widely-known term.

[Jennifer Messerly, 2013-08-02 19:31:59]

this is looking great!

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
File tools/dom/src/EventStreamProvider.dart (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:61: class _ElementListEventStreamImpl<T
extends Event> extends Stream<T> with
should this be "implements Stream"? it looks like most (all?) of the methods are
being forwarded.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:67: _controller = new
StreamController(sync: true);
should this be a broadcast stream?

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:69: var stream = new _EventStream(target,
eventType, useCapture);
hmm, it looks like these listeners are never released?

I think you can use onListen and onCancel here, something like:


List _subscriptions;

_ElementListEventStreamImpl(...)
{
  _controller = new StreamController(sync: true, onListen: _onListen, onCancel:
_onCancel);
}

/// Called the first time a listener is added to this object
_onListen() {
  _subscriptions = [];
  for (Element target in targetList) {
    var stream = ...
    _subscriptions.add(stream.listen(...));
  }
}

/// Called when the last subscription for this object is cancelled
_onCancel() {
  for (var sub in _subscriptions) sub.cancel();
  _subscriptions = null;
}

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:82: _controller.stream.listen(onData,
onError: onError, onDone: onDone,
it is probably worth caching "_controller.stream". it appears that it allocates
a new object each time. (from stream_controller.dart)

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/template...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:248:
@DomName('Element.onabort')
curious if we can (or should) generate these? Do we generate the on* methods in
Element?

[blois, 2013-08-02 20:13:02]

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/scripts/...
File tools/dom/scripts/htmleventgenerator.py (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/scripts/...
tools/dom/scripts/htmleventgenerator.py:294: isElement = ('Element' in
interface.id and
Shouldn't all element subclasses get this? Not just element?

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
File tools/dom/src/EventStreamProvider.dart (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:43: Stream<T> filter(String selector) =>
Filter seems a bit generic for Streams (I could easily see it have different
uses elswhere).

How about matchesSelector? or matches (follows Element)?

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:51: class _ElementEventStreamImpl<T
extends Event> extends _EventStream<T> with
Rather than the mixin, how about just declare filter on ElementStream, but put
the implementation here. Generated code should be simpler/smaller.

So:
class _ElementEventStreamImpl<T extends Event> extends _EventStream<T>
implements ElementStream<T> {}

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:61: class _ElementListEventStreamImpl<T
extends Event> extends Stream<T> with
Can this be made more generic- a Stream which combines a bunch of other streams
into one? I know pub has a version which combines 2 streams, not sure if there's
an impl around which combines many.

Then just a subclass for ElementStream which adds the filtering?

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:69: var stream = new _EventStream(target,
eventType, useCapture);
On 2013/08/02 19:31:59, John Messerly wrote:
> hmm, it looks like these listeners are never released?
> 
> I think you can use onListen and onCancel here, something like:
> 
> 
> List _subscriptions;
> 
> _ElementListEventStreamImpl(...)
> {
>   _controller = new StreamController(sync: true, onListen: _onListen,
onCancel:
> _onCancel);
> }
> 
> /// Called the first time a listener is added to this object
> _onListen() {
>   _subscriptions = [];
>   for (Element target in targetList) {
>     var stream = ...
>     _subscriptions.add(stream.listen(...));
>   }
> }
> 
> /// Called when the last subscription for this object is cancelled
> _onCancel() {
>   for (var sub in _subscriptions) sub.cancel();
>   _subscriptions = null;
> }
See
https://codereview.chromium.org/12220003/diff/6/tools/dom/src/EventStreamProv...
for an earlier CL with this.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:89: Stream map(convert(T event)) =>
_controller.stream.map(convert);
Is it necessary to override all of these? Seems like only the listen() method
should need to be overridden.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:236: Stream<T> forTarget(EventTarget e,
{bool useCapture: false}) {
Since the signature is still returning Stream<T>, why have return a sub-type?
Users shouldn't rely on it since it's not guaranteed by the API.

[blois, 2013-08-02 20:21:25]

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
File tools/dom/src/EventStreamProvider.dart (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:44: this.where((event) =>
event.target.matches(selector));
Also, see the discussion in the bug about the ancestor tree- I think this is a
critical aspect of the function.

For example:
  <div class='clickable'>
    <div>something</div>
  </div>

  document.body.onClick.filter('.clickable')

This would yield no click events since the element that the clicks occur on (the
inner div) does not match the selector.

[Emily Fortuna, 2013-08-07 23:38:25]

ptal

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/scripts/...
File tools/dom/scripts/htmleventgenerator.py (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/scripts/...
tools/dom/scripts/htmleventgenerator.py:294: isElement = ('Element' in
interface.id and
On 2013/08/02 20:13:02, blois wrote:
> Shouldn't all element subclasses get this? Not just element?

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
File tools/dom/src/EventStreamProvider.dart (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:43: Stream<T> filter(String selector) =>
On 2013/08/02 20:13:02, blois wrote:
> Filter seems a bit generic for Streams (I could easily see it have different
> uses elswhere).
> 
> How about matchesSelector? or matches (follows Element)?

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:44: this.where((event) =>
event.target.matches(selector));
On 2013/08/02 20:21:25, blois wrote:
> Also, see the discussion in the bug about the ancestor tree- I think this is a
> critical aspect of the function.
> 
> For example:
>   <div class='clickable'>
>     <div>something</div>
>   </div>
> 
>   document.body.onClick.filter('.clickable')
> 
> This would yield no click events since the element that the clicks occur on
(the
> inner div) does not match the selector.

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:51: class _ElementEventStreamImpl<T
extends Event> extends _EventStream<T> with
On 2013/08/02 20:13:02, blois wrote:
> Rather than the mixin, how about just declare filter on ElementStream, but put
> the implementation here. Generated code should be simpler/smaller.
> 
> So:
> class _ElementEventStreamImpl<T extends Event> extends _EventStream<T>
> implements ElementStream<T> {}

True, but then we have to write the identical code in
_ElementListEventStreamImpl (though it's not a lot of code, so not a huge
deal...). Sure, it produces less code, but is the new version actually better on
the maintainable code that developers see?

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:61: class _ElementListEventStreamImpl<T
extends Event> extends Stream<T> with
On 2013/08/02 20:13:02, blois wrote:
> Can this be made more generic- a Stream which combines a bunch of other
streams
> into one? I know pub has a version which combines 2 streams, not sure if
there's
> an impl around which combines many.
> 
> Then just a subclass for ElementStream which adds the filtering?

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:61: class _ElementListEventStreamImpl<T
extends Event> extends Stream<T> with
On 2013/08/02 19:31:59, John Messerly wrote:
> should this be "implements Stream"? it looks like most (all?) of the methods
are
> being forwarded.

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:67: _controller = new
StreamController(sync: true);
On 2013/08/02 19:31:59, John Messerly wrote:
> should this be a broadcast stream?

yes, thank you. done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:69: var stream = new _EventStream(target,
eventType, useCapture);
On 2013/08/02 20:13:02, blois wrote:
> On 2013/08/02 19:31:59, John Messerly wrote:
> > hmm, it looks like these listeners are never released?
> > 
> > I think you can use onListen and onCancel here, something like:
> > 
> > 
> > List _subscriptions;
> > 
> > _ElementListEventStreamImpl(...)
> > {
> >   _controller = new StreamController(sync: true, onListen: _onListen,
> onCancel:
> > _onCancel);
> > }
> > 
> > /// Called the first time a listener is added to this object
> > _onListen() {
> >   _subscriptions = [];
> >   for (Element target in targetList) {
> >     var stream = ...
> >     _subscriptions.add(stream.listen(...));
> >   }
> > }
> > 
> > /// Called when the last subscription for this object is cancelled
> > _onCancel() {
> >   for (var sub in _subscriptions) sub.cancel();
> >   _subscriptions = null;
> > }
> See
>
https://codereview.chromium.org/12220003/diff/6/tools/dom/src/EventStreamProv...
> for an earlier CL with this.

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:82: _controller.stream.listen(onData,
onError: onError, onDone: onDone,
On 2013/08/02 19:31:59, John Messerly wrote:
> it is probably worth caching "_controller.stream". it appears that it
allocates
> a new object each time. (from stream_controller.dart)

Done.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:89: Stream map(convert(T event)) =>
_controller.stream.map(convert);
On 2013/08/02 20:13:02, blois wrote:
> Is it necessary to override all of these? Seems like only the listen() method
> should need to be overridden.

Since it's not actually a Stream and just implements stream.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:236: Stream<T> forTarget(EventTarget e,
{bool useCapture: false}) {
On 2013/08/02 20:13:02, blois wrote:
> Since the signature is still returning Stream<T>, why have return a sub-type?
> Users shouldn't rely on it since it's not guaranteed by the API.

But we should expect Element Event streams to always allow delegation.

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/template...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/21607003/diff/11001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:248:
@DomName('Element.onabort')
On 2013/08/02 19:31:59, John Messerly wrote:
> curious if we can (or should) generate these? Do we generate the on* methods
in
> Element?

I would totally LOVE to, and I looked into this, but it is more trouble than
it's worth... We'd have to add another type of emitter that only emits these
onFoo methods instead of all the members.

[blois, 2013-08-08 00:12:10]

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
File tools/dom/src/EventStreamProvider.dart (right):

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:12: class StreamPool<T> {
Should not be public, can we move it down in the file as well?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:13: StreamController<T> _controller;
final?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:133: class _ElementListEventStreamImpl<T
extends Event>  implements ElementStream<T> {
I'm still unclear why this cannot extend Stream and implement ElementStream,
removing the need to forward all of these APIs.

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:134: //implements Stream<T> {
Comment

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:136: StreamPool _pool;
final.

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:137: Stream<T> _stream;
Is it necessary to cache this?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:312: return new
_ElementEventStreamImpl(e, _eventType, useCapture);
Does this need to be here given that there's already forElementTarget?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/src/Even...
tools/dom/src/EventStreamProvider.dart:332: ElementStream<T>
forElementTarget(Element e, {bool useCapture: false}) {
How about just 'forElement'?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/template...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:250:
@DomName('Element.onbeforecopy')
Is it necessary to do all events? I'm worried about this list getting out of
date- if we want all, is there some way we can keep it in sync with Element?

https://chromiumcodereview.appspot.com/21607003/diff/30001/tools/dom/template...
tools/dom/templates/html/impl/impl_Element.darttemplate:1082: bool
matches(String selectors, [includeAncestors = false]) {
This API should become an inlined native API, I'm a bit hesitant to add the
includeAncestors to this.

[Emily Fortuna, 2013-08-09 00:22:05]

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
File tools/dom/src/EventStreamProvider.dart (right):

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:12: class StreamPool<T> {
On 2013/08/08 00:12:11, blois wrote:
> Should not be public, can we move it down in the file as well?

Done.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:13: StreamController<T> _controller;
On 2013/08/08 00:12:11, blois wrote:
> final?

can't be because _controller has the onCancel method that calls "close", which
is an instance method, and can't be called in the initializer section.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:133: class _ElementListEventStreamImpl<T
extends Event>  implements ElementStream<T> {
On 2013/08/08 00:12:11, blois wrote:
> I'm still unclear why this cannot extend Stream and implement ElementStream,
> removing the need to forward all of these APIs.

How would this extend Stream and not need to call all of the forwarding methods?
If you extended Stream, then you're saying that _ElementListEventStreamImpl IS a
stream. But it's not. It masquerades as a stream holding a StreamController/Pool
that produces a stream, but _ElementListEventStreamImpl itself is not a stream.
If I didn't do all the method forwarding, if you called map [taken from
stream.dart], the following code would execute: return new _MapStream<T,
dynamic>(this, convert); but in the case of _ElementListEventStreamImpl, "this"
is not a Stream. Instead you want to call map on _ElementListEventStreamImpl's
stream.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:134: //implements Stream<T> {
On 2013/08/08 00:12:11, blois wrote:
> Comment

Done.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:136: StreamPool _pool;
On 2013/08/08 00:12:11, blois wrote:
> final.

Done.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:137: Stream<T> _stream;
On 2013/08/08 00:12:11, blois wrote:
> Is it necessary to cache this?

It's helpful, because otherwise get stream in StreamController creates a new
stream every time you access it.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/src/EventStream...
tools/dom/src/EventStreamProvider.dart:312: return new
_ElementEventStreamImpl(e, _eventType, useCapture);
On 2013/08/08 00:12:11, blois wrote:
> Does this need to be here given that there's already forElementTarget?

I want Elements to always have the ability to do event delegation, even if some
user calls this API, which they can since it's public.... but I changed it as
requested.

https://codereview.chromium.org/21607003/diff/30001/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/21607003/diff/30001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:250:
@DomName('Element.onbeforecopy')
On 2013/08/08 00:12:11, blois wrote:
> Is it necessary to do all events? I'm worried about this list getting out of
> date- if we want all, is there some way we can keep it in sync with Element?

Now with autogeneration!

https://codereview.chromium.org/21607003/diff/30001/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Element.darttemplate:1082: bool
matches(String selectors, [includeAncestors = false]) {
On 2013/08/08 00:12:11, blois wrote:
> This API should become an inlined native API, I'm a bit hesitant to add the
> includeAncestors to this.

Where is there information about it becoming an inlined native API? 

W3C doesn't mention it anywhere; nowhere on the web can I find info about
"includeAncestors" with respect to the matchesSelector.

[blois, 2013-08-09 01:02:58]

lgtm

[Jennifer Messerly, 2013-08-09 06:05:59]

lgtm

[Emily Fortuna, 2013-08-09 17:37:40]

Committed patchset #6 manually as r25969 (presubmit successful).