UIObject.sinkEvents behavior changes with GWT 1.4.10RC

[Sandy McArthur]

I don't have time to do full bug checking and test for this right now
but I noticed that the requirements for UIObject.sinkEvents seems to
have changed.

In GWT 1.3 I could have a Widget that responded to browser events from
elements other than the widget's top level element. Eg: A table Widget
that sinks events on it's <td> elements represented by UIObjects.

In GWT 1.4.10RC it seems I have to sinkEvents on the top level element
of the widget or it won't receive any browser events. eg: sinkEvents
on the <table> element.

Is this an intentional change? Being that sinkEvents is on UIObject
instead of Widget, this seems to not be the intended behavior or an
unfortunate place for the sinkEvents method to be.

--
Sandy McArthur

"He who dares not offend cannot be honest."
- Thomas Paine

[Sandy McArthur]

An issue, example project with GWT 1.3.3 and 1.4.10 compiled output
has been created and attached to 1159:
http://code.google.com/p/google-web-toolkit/issues/detail?id=1159

[Rajeev Dayal]

Hi Sandy,

It turns out that this change in behavior was caused by the fix for issue #734. Here is a link to the code review thread:

http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/b6a816940e43b24f/6ac452ef914dbed7

This fix was also meant to correct the behavior that you are taking advantage of. That is, it would be strange to call sinkEvents on a child <td>, and then receive events on the Table widget,  when sinkEvents was never called on the Table widget to begin with.

However, I do understand what you mean about the negative performance impact of having to call sinkEvents on the entire Table widget when you only care about events on the <td> itself.

One way to get around this problem would be to call DOM.setEventListener(td, tableWidget). That way, the <td> element would pick up events, and the Table widget would act as the listener. However, to prevent memory leaks, you would have to make sure to clear this event listener when the Table widget becomes detached from the DOM.

If possible, can you describe some of the use cases in which you depend on the the existing event behavior in 1.3.3.?


Thanks,
Rajeev

[Sandy McArthur]

On 6/4/07, Rajeev Dayal <rda...@google.com> wrote:
> It turns out that this change in behavior was caused by the fix for issue
> #734. Here is a link to the code review thread:
>
> http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/b6a816940e43b24f/6ac452ef914dbed7
>
> This fix was also meant to correct the behavior that you are taking
> advantage of. That is, it would be strange to call sinkEvents on a child
> <td>, and then receive events on the Table widget, when sinkEvents was
> never called on the Table widget to begin with.

Then it is most unfortunate and counter intuitive that the sinkEvents
method is part of the UIObject class and not the Widget class if that
is the only place it works.

> However, I do understand what you mean about the negative performance impact
> of having to call sinkEvents on the entire Table widget when you only care
> about events on the <td> itself.
>
> One way to get around this problem would be to call DOM.setEventListener(td,
> tableWidget). That way, the <td> element would pick up events, and the Table
> widget would act as the listener. However, to prevent memory leaks, you
> would have to make sure to clear this event listener when the Table widget
> becomes detached from the DOM.

Which is why I had rationalized/accepted the counter intuitiveness of
registering for events you care about on UIObjects (DOM elements) but
processing them on the UIObject's parent Widget.

The "UIObject sends events to a parent Widget" design also makes
certain kind of sense if my suggestion for a future DnD interface was
that any UIObject could be a drag source/target. This way the whole or
just parts of a Widget could be drag grips or drop targets instead of
the whole Widget.

> If possible, can you describe some of the use cases in which you depend on
> the the existing event behavior in 1.3.3.?

This change will break behavior for anyone who uses GWT-Stuff's
ObjectListTable and needs to response to mouse events at the row group
or row level until either the old behavior is restored or I cut a new
GWT-Stuff release without any browser event handling optimizations.

The table widget from GWT-Stuff models the full w3c table model. The
Widget subclass is the table element with internally managed tbody,
thead, tfoot, and tr elements represented by various UIObject
subclasses. (The td and th elements are Panel subclasses.) The
UIObjects that make up the table row groups and rows call sinkEvents
the first time the programmer indicates they want to receive mouse
events for those row groups or rows (Table cells are handled just like
you would with a Panel).

Too see an example of how it can get slow, when I display a lot of
rows in the test app below on an older machine and move the mouse over
some rows the browser can take over a second to re-render the effect
of the mouse moving across the table. (The row group/rows colors are
updated via JavaScript and not CSS:hover.)
http://sandy.mcarthur.org/gwt-stuff/org.mcarthur.sandy.gwt.table.olt.TestObjectListTable/TestObjectListTable.html

I can deal with this behavior change and I already avoid huge complex
UI designs when I can but I'm kind of annoyed that I'll have to
revisit the ~4 days of spare time that I spent trying out event
processing optimizations and then benchmarking them to find an optimal
one again. :-/

[Rajeev Dayal]

Hey Sandy,

Thanks for the information. I'm sorry that this change has caused some breakage in GWT-Stuff. We really try to minimize the number of breaking changes from one release to another, but it looks like this was a major one for you.

> This fix was also meant to correct the behavior that you are taking
> advantage of. That is, it would be strange to call sinkEvents on a child
> <td>, and then receive events on the Table widget,  when sinkEvents was
> never called on the Table widget to begin with.

Then it is most unfortunate and counter intuitive that the sinkEvents
method is part of the UIObject class and not the Widget class if that
is the only place it works.

I agree with you. We should probably move sinkEvents down to the Widget class.  

I can deal with this behavior change and I already avoid huge complex
UI designs when I can but I'm kind of annoyed that I'll have to
revisit the ~4 days of spare time that I spent trying out event
processing optimizations and then benchmarking them to find an optimal
one again. :-/

If you made the change that I suggested (call sinkEvents on the <td> UIObject with the Table widget as the listener), would you not get the desired behavior?



Rajeev

[Sandy McArthur]

On 6/5/07, Rajeev Dayal <rda...@google.com> wrote:
> > > This fix was also meant to correct the behavior that you are taking
> > > advantage of. That is, it would be strange to call sinkEvents on a child
> > > <td>, and then receive events on the Table widget, when sinkEvents was
> > > never called on the Table widget to begin with.
> >
> > Then it is most unfortunate and counter intuitive that the sinkEvents
> > method is part of the UIObject class and not the Widget class if that
> > is the only place it works.
>
> I agree with you. We should probably move sinkEvents down to the Widget
> class.

Definitely move it down.

> > I can deal with this behavior change and I already avoid huge complex
> > UI designs when I can but I'm kind of annoyed that I'll have to
> > revisit the ~4 days of spare time that I spent trying out event
> > processing optimizations and then benchmarking them to find an optimal
> > one again. :-/
>
> If you made the change that I suggested (call sinkEvents on the <td>
> UIObject with the Table widget as the listener), would you not get the
> desired behavior?

Maybe/probably, I haven't looked into it. I'm just going to revert to
a basic implementation to get it working for now and will revisit
performance tuning if/when it becomes an issue and/or I have spare
time again.

[Joel Webber]

All,

I know it's been a long time since we visited this issue, but I'm
going to open it up for discussion once again :)

The original event dispatch code in DOMImpl behaved the following way:
- See if the element has an __listener.
- If so, dispatch the event to it.
- If not, go to its parent.
- Wash, rinse, repeat.

I'd love to say that this was part of some grand scheme, but it was
actually an accidental holdover from an old version of the UI library.
It just turned out that it didn't make much of a difference in
practice, as most elements that had events sunk had a __listener, and
it ends up doing much the same thing as event bubbling, except on
those events that don't bubble.

As we were working on 1.4, I was convinced that no one was taking
advantage of this behavior in any useful way. In fact, after we pulled
it out, we found a couple of bugs in existing widgets that were being
masked by it. So we yanked it, fixed the bugs, and moved on happily.

Now Sandy has pointed out a pretty useful side-effect of this
behavior: You can call sinkEvents() on a sub-element of a Widget, and
receive those events on the Widget without having to explicitly sink
events at the top. My first reaction was to suggest simply sinking
those events on the Widget itself, and letting them bubble up. This
has two problems, though:
- Not all events bubble.
- Sinking an event like mousemove at the top causes the Widget to
receive a *lot* of events that it might not be interested in.

The old behavior allowed you to receive events only from a subset of
elements, which can be quite an optimization. But it can also lead to
some slightly weird behavior. Consider the following case:

public SomeWidget() {
setElement(DOM.createDiv());
Element child = DOM.createDiv());
DOM.appendChild(getElement(), child);

sinkEvents(Event.CLICKEVENT);
DOM.sinkEvents(child, Event.CLICKEVENT);
}

Now onBrowserEvent will be fired *twice* for each click on 'child'.
The first time when child gets the event and the dispatcher crawls up
a notch to the root element. The second time when it bubbles up
naturally to the root element. One could argue that this is perfectly
reasonable, given that we called sinkEvents() twice, but I want to
make sure it makes sense to everyone before we consider changing back.

What does everyone think?

joel.

[Scott Blum]

My totally uninformed take (barrel of NaCl required):

It seems like there should be a better way of handling Sandy's use case without having to do our own event bubbling.  I would hate to penalize the 99% case for the 1% case.  However, I have no idea what that way might be.

Scott

[Fred Sauer]

Joel,

It seems we can do better than walking up the DOM tree to find a __listener in an element's ancestors. I'd like to advocate for the ability to:

1. Subscribe to events on any element (no descendant requirement)
   
2. Registering multiple listeners on an element

We have this DOM method:
  public static void sinkEvents(Element elem, int eventBits)

I'd like to overload it with something like this:
  public static void sinkEvents(Element elem, int eventBits, EventListener listener)

This means expanding the implementation behind __listener to support multiple listeners, each listening according to specific event bit mask.


My use case is drag-and-drop, where I'd like to use a controller to make any Element draggable.

Fred

[Joel Webber]

Fred,

> I'd like to overload it with something like this:
>   public static void sinkEvents(Element elem, int eventBits, EventListener listener) 
 

Ok, here's where things get weird. I've never fully documented this clearly, but I'm going to take a quick stab at it, and use it as a starting point for getting the doc written.

The short answer is "this will almost definitely end up creating unfixable memory leaks". The long answer is below.

joel.

DOM Events, Memory Leaks, and You

 

You may ask yourself, "Why do I have to use bitfields to sink DOM events?", and you may ask yourself, "Why can I not add event listeners directly to elements?". If you find yourself asking these questions, it's probably time to dig into the murky depths of DOM events and memory leaks.

If you're creating a Widget from scratch (using DOM elements directly, as opposed to simply creating a Composite), the setup for event handling is a bit odd. Generally, it looks something like this:

class MyWidget extends Widget {

  public MyWidget() {

    setElement(DOM.createDiv());

    sinkEvents(Event.ONCLICK);

  }

  public void onBrowserEvent(Event evt) {

    switch (DOM.eventGetType(evt)) {

      case Event.ONCLICK:

        // Do something insightful.

        break;

    }

  }

}

 

This may seem a bit obtuse, but there's a good reason for it. To understand this, you may first need to brush up on browser memory leaks. There are some good resources on the web:

- http://www.quirksmode.org/blog/archives/2005/02/javascript_memo.html

- http://www-128.ibm.com/developerworks/web/library/wa-memleak/

The upshot of all this is that in some browsers, any reference cycle that involves a Javascript object and a DOM element (or other native object) has a nasty tendency to never get garbage-collected. The reason this is so insidious is that this is an extremely common pattern to create in Javascript UI libraries. Imagine the following (raw Javascript) example:

 

function makeWidget() {

  var widget = {};

  widget.someVariable = "foo";

  widget.elem = document.createElement ('div');

  widget.elem.onclick = function() {

    alert(widget.someVariable);

  };

}

 

Now, I'm not suggesting that you'd really build a Javascript library quite this way, but it serves to illustrate the point. The reference cycle created here is:

widget -> elem(native) -> closure -> widget

There are many different ways to run into the same problem, but they all tend to form a cycle that looks something like this. This cycle will never get broken unless you do it manually (often by clearing the onclick handler).

There are a number of ways developers try to deal with this issue. One of the more common I've seen is to walk the DOM when window.onunload is fired, clearing out all event listeners. This is problematic for two reasons:

1. It doesn't clear events on elements that are no longer in the DOM.

2. It doesn't deal with long-running applications, which are becoming more and more common.

GWT's Solution

When designing GWT, we decided that leaks were simply unacceptable. You wouldn't tolerate egregious memory leaks in a desktop application, and a browser app should be no different. This raises some interesting problems, though. In order to avoid ever creating leaks, any Widget that *might* need to be get garbage collected must not be involved in a reference cycle with a native element. There's no way to find out "when a widget would have been collected had it not been involved in a reference cycle". So in GWT terms, a Widget must not be involved in a cycle when it is detached from the DOM.

How do we enforce this? Each Widget has a single "root" element. Whenever the Widget becomes attached, we create exactly one "back reference" from the element to the Widget ( i.e. elem.__listener = widget, performed in DOM.setEventListener()). This is set whenever the Widget is attached, and cleared whenever it is detached.

Which brings is back to that odd bitfield used in sinkEvents(). If you look at the implementation of DOM.sinkEvents(), you'll see that it does something like this:

  elem.onclick = (bits & 0x00001) ? $wnd.__dispatchEvent : null;

Each element's events point back to a central dispatch function, which looks for the target element's __listener expando, in order to call onBrowserEvent(). The beauty of this is that it allows us to set and clear a single expando to clean up any potential event leaks.

What this means in practice is that, as long as you don't set up any reference cycles on your own using JSNI, you can't write an app in GWT that will leak. We test carefully with every release to make sure we haven't done anything in the low-level code to introduce new leaks as well.

The downside, of course, is that you can't hook event listeners directly to elements that are children of a Widget's element. Rather, you have to receive the event on the Widget itself, and figure out which child element it came from.

But that's better than leaking gobs of memory on your users' machines, right?

 

[Ian Petersen]

While the structure of sinkEvents is being considered, I'd just like
to ask if the "current" event were somehow more accessible to plain
Java code. TextBox has an instance method named currentEvent(), or
something, that was very useful in my KeyFilter code, found here
http://groups.google.com/group/Google-Web-Toolkit/msg/da43b3af33d2076d?hl=en&.
The relevant point in that message is this:

> I couldn't figure out how to cancel the "current" event
> in a generic fashion--it seems you need to be in the
> onBrowserEvent method to do that--so users of
> KeyFilter on any widget besides TextBox will have to
> implement cancelKey themselves.

Perhaps the use case exemplified in that message is rare, but it was
disappointing to find out that a general KeyboardListener can't do
anything useful with the current Event object because it's not
accessible. If you want to preventDefault() or cancelBubble() you
have to be in onBrowserEvent, which can be a lot of work for little
gain. Unfortunately, I'm not sure how to allow greater access to the
current event and still guarantee no memory leaks.

Ian

--
Tired of pop-ups, security holes, and spyware?
Try Firefox: http://www.getfirefox.com

[Fred Sauer]

Joel,

Thanks for the response. It will be great to have this whole memory leak business be a part of the docs!


I would like to throw in a little what-if scenario. Here goes.

What if, in order to maintain the requirement that:

"...a Widget must not be involved in a cycle when it is detached from the DOM",

and the convenience of:

"...set and clear a single expando to clean up any potential event leaks"

we still maintain a single __listener expando on the element, but rather than have __listener simply be a reference to the widget, we rename it to __listeners and have it contain a list of listeners?

As long as we continue to cleanup __listener when the widget is detached there should be no more back references than we have today.

Is there something that I'm missing?

Thanks
Fred

--
Fred Sauer
fr...@allen-sauer.com

[Joel Webber]

Ian,

Yes, yes, yes! I've never gotten around to submitting an issue for this myself, but it's been floating around in the back of my mind. Nevermind the fact that we should have had Event objects from the beginning (which would have made this problem a lot easier to solve cleanly; but that's a long story...). We should definitely make the 'current' event accessible easily. Hell, it's free on IE, and as long as Javascript's single-threaded, won't pose problems on any browser as long as we build a stack.

Would you do me a favor and enter an issue for this? If you could CC me, I'll go ahead and mark it for 1.4, since it's really not complicated at all.

Thanks,

joel.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Google Web Toolkit Contributors" group.
To post to this group, send email to Google-Web-Toolkit-Contributors...@googlegroups.com
To unsubscribe from this group, send email to Google-Web-Toolkit-Contributor s-unsu...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/Google-Web-Toolkit-Contributors?hl=en
-~----------~----~----~----~-- ----~----~------~--~---

[Joel Webber]

Fred,

__listener could be turned into a list, as long as each object in that list made absolutely certain to remove itself, but I'm not entirely sure how I could use this in practice. I really wouldn't want to encourage people to have more than one Widget listening to events on the same element -- sounds like a recipe for trouble. What would you like to use this for?

joel.

[Ian Petersen]

Joel,

With a response like that, I gladly entered an issue. Unfortunately,
there doesn't seem to be a way for me to CC you, so here's the issue
for you to do it yourself:
http://code.google.com/p/google-web-toolkit/issues/detail?id=1309

[Joel Webber]

Thanks, Ian. I've moved it to 1.4 & pwned it myself.

On 6/26/07, Ian Petersen <ispe...@gmail.com> wrote:

[Fred Sauer]

Joel,

I agree that (in general) I wouldn't want to mouse event listeners or two key event listeners on the same widget. A scenario requiring multiple listeners might include an element that would like to listen for its own key events while a separate drag-and-drop controller would want to listen for mouse events on that same element.

Maybe I should backup a bit: my driving use case is that I want to be able to register a mouse event listener on an arbitrary Element (in a GWT-supported way), for two reasons:

1. Make arbitrary widgets draggable; i.e. widgets that do not implement SourcesMouseEvents
2. Capture mouse events on an Element which does not have a corresponding Widget such as table cells (TD, TH) or table rows (TR); e.g. to allow rearranging of rows or columns in  a table
   

Thanks
Fred

--
Fred Sauer
fr...@allen-sauer.com