1.4.10RC -- add(widget) in onLoad() throws IllegalStateException

[Fred Sauer]

Hi,

The following code works in 1.3.3, but throws an exception in 1.4.10 RC.

public class Test implements EntryPoint {

  class TestPanel extends SimplePanel {

    protected void onLoad() {
      super.onLoad();
      add(new Label("label"));
    }
  }

  public void onModuleLoad() {
    RootPanel.get().add(new TestPanel()); //exception
  }
}

The stack trace is:

[ERROR] Unable to load module entry point class com.allen_sauer.gwt.test.client.Test (see associated exception for details)
java.lang.IllegalStateException: Should only call onAttach when the widget is detached from the browser's document
    at com.google.gwt.user.client.ui.Widget.onAttach(Widget.java:89)
    at com.google.gwt.user.client.ui.Panel.onAttach (Panel.java:86)
    at com.google.gwt.user.client.ui.Widget.setParent(Widget.java:216)
    at com.google.gwt.user.client.ui.Panel.adopt(Panel.java:59)
    at com.google.gwt.user.client.ui.ComplexPanel.insert(ComplexPanel.java:107)
    at com.google.gwt.user.client.ui.ComplexPanel.add (ComplexPanel.java:70)
    at com.google.gwt.user.client.ui.AbsolutePanel.add(AbsolutePanel.java:68)
    at com.allen_sauer.gwt.test.client.Test.onModuleLoad(Test.java:22)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:25)

If the sample code is supposed to work, I'll gladly file an issue.

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

[Scott Blum]

I can't think of any reason that code shouldn't work as stated.  Please put the word "Regression" in the issue's subject line so it will catch our attention.

[Fred Sauer]

Created Issue #1121

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

[Sandy McArthur]

This is another example of how the onLoad() method is flawed. See
comment 5 of issue 896 for another example:
http://code.google.com/p/google-web-toolkit/issues/detail?id=896#c5

The problem is that the onLoad() method is called in the middle of the
onAttach process. This means the widget is in a weird, possibly
inconsistent state. If you were to override onAttach() and put your
onLoad() logic there it would work fine.

The flow is Panel.onAttach() calls Widget.onAttach() which sets
attached to true and then calls onLoad(). After that is done then
Panel.onAttach() finishes by calling onAttach for all the children of
the panel.

In your case the onLoad() method added the new Label and the label was
"attached" as part of that add. When the Panel.onAttach() finished up
it "attached" the label again.

There are two fixed for this. 1. Don't throw the exception and allow a
double attach behavior or 2. move Widget.onAttach()'s call to onLoad()
to before the widget sets attached to true. The former allows a weak
widget lifecycle and the latter means that children are "attached"
before the panel is.

IMO onLoad() should be @deprecated and the onUnload should be removed
before it's too late.

On 5/30/07, Fred Sauer <fr...@allen-sauer.com> wrote:

--
Sandy McArthur

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

[Scott Blum]

On 5/30/07, Sandy McArthur <sand...@gmail.com> wrote:

The flow is Panel.onAttach() calls Widget.onAttach() which sets
attached to true and then calls onLoad(). After that is done then
Panel.onAttach() finishes by calling onAttach for all the children of
the panel.

I'm going to go out on a limb and say that this could be where the flaw is, that Panel (through its super call) calls its onLoad() before it's really through attaching; where I define " really through attaching" to mean that it has already called onAttach on its children.  The contract for onLoad() would seem to be that it shouldn't be called until onAttach is completely done, and in Panel's case, it simply isn't.

Not sure what the right formulation is, though.

Scott

[Miroslav Pokorny]

Im only bringing this up as an idea, because we have already had a few posts about onLoad and onAttach. What do people think of allowing each widget to have a collection of listeners with methods like

void onLoad( Widget widget )
void onAttach( Widget widget )
void onDetach( Widget widget ) with all these methods fired when the parent widget is loaded/attached/detach...

?

--
mP

[Sandy McArthur]

On 5/30/07, Scott Blum <sco...@google.com> wrote:

Short of special case compiler magic intercepting the first call to
this.onAttach() and then calling this.onLoad() or Widget.onAttach()
queuing a DeferredCommand (an unacceptable solution because of
latency) I don't think onLoad() can be "fixed". The onUnload() is
susceptible to the same kinds of problems, just in reverse.

[Ray Cromwell]

Perhaps we need something other than onLoad, like "onAllChildrenAttached" called when everything is in a consistent state. Successfully calling super.onAttach() can guarantee it if attachment is a synchronous process, but it isn't always. In my charting framework, I have to deal with asynchronous attachment. With the CANVAS tag, it's not an issue,  because after a DOM.appendChild(), you know its attached.

However, for stuff like Applets, Flash and Silverlight, it's a different story. If you attempt to wrap a Silverlight canvas, for example, in a GWT widget, you can't just create a DIV/OBJECT tag, drop it in the DOM, and then proceed to run onAttach logic, because if any methods are invoked on those objects before they are fully initialized, you can arrive at a bad place very quickly (segmentation fault usually)

So I ended up having to use continuations to attach all of the Silverlight or Flash canvases without bad stuff happening. To fix this, with SIlverlight and Flash, onAttach simply kicks off an asynchronous JS call to initialize those plugins, and when the native plugin code
is done doing whatever it needs to, it calls a ready function, and the ready function calls back into the Canvas Widget. The Canvas
widget then knows that the underlying native voodoo stuff is up and running, and proceeds to invoke the continuation.

At the very end of the continuation chain is a call to "allCanvasesAttached()", which subclasses of the Chart container can use to know
that it is safe to initialize the chart and render. (in double buffered method, atleast 2 canvases are used, if zoomed-out overviews or used,
than 3 canvases are used, etc)

My suggestion is that rather than rely on super.onAttach() to guarantee parent, and all children, attached from a synchronous point of view, or using onLoad() which appears to have an issue, institute a method like "onAllChildrenAttached" or "onReady"  which is called, if and only if, the whole tree has been successfully attached. The kicker is, onAllChildrenAttached *may* be called asynchronously,
so it would be clearer that super.onAttach() doesn't mean "it's now safe to do stuff", only that everything has been dropped into
the DOM, however, you MAY have to wait an unspecified period of the time for the browser to digest this, and tell you when it is
ready.

If this was the original intent of onLoad, then perhaps all that needs to be done is fix onLoad and make the document clearer
about best practices, what can and can't be done inside an onAttach() method.

My requirements may be in the minority, since I am experimenting with plugin integration, but I have also encountered
times when certain DOM functions don't work unless you defer them. For example, document.defaultView.getComputedStyled ()
doesn't always immediately reflect new CSS values on DOM modification. I have encountered some rare strange cases where
modifying the DOM, and then asking for the computed element style returns wrong values, unless the getComputedStyle()
is deferred. I haven't been able to narrow it down though...

Just my 2 cents,
-Ray

[Reinier Zwitserloot]

Is it perhaps possible to just add an event system for this?

someWidget.addAttachListener?

[Joel Webber]

All,

Sorry I've been so slow to chime in on this thread. Let me back up and give a little historical perspective, and hopefully we can reach a reasonable conclusion here.

The original purpose of onLoad() was simply to give Widgets an indication of when they were actually attached to the DOM (whether it was a good name or not is another question). As Ray points out, there are certain types of elements (mostly plugins) that require a stronger guarantee than this, but for a number of built-in elements, it's pretty useful. Examples include <img>, which doesn't start loading until it's attached, and <input>, several of which have bizarre differences when attached/detached.

I originally wanted to make onAttach/Detach() private, so that no one could keep the event listeners from getting hooked/unhooked accidentally (potentially leading to weird event bugs and memory leaks), but that just wasn't feasible because Panel needs to override it. So I added onLoad() as a 'standard' way to know when an element's attached.

As Sandy points out, there is no certain way to know, in all cases, when you can safely measure an element's size. This is always going to be true in general, and not even a DeferredCommand can fix it, because any change in size of any element (say, from an image loading, or a style change) can theoretically change the size of any other element in the document. It absolutely blows that there's no reliable resize event, but I'm afraid we're stuck with it. That said, onLoad() *is* the first time at which you can reliably measure an element *at all*, and I believe there are cases where that's useful.

Can anyone see an issue with simply moving Panel.onAttach()'s super call to the end of the method, to at least fix the problem at hand? Before we start discussing stuff like addAttachListener(), I want to first pin down the correct ordering of this invocation, and ensure that the one guarantee we make in onLoad (namely that the widget will be attached at that point) is maintained.

@Sandy: When you say that onLoad is "broken" and "should be deprecated", I'd like to understand exactly what purpose it's broken for. As an example of a necessary use-case, CheckBox needs it to set its checked state when attached (and retain it when removed), because of browser weirdness (please ignore the fact that it's actually using onAttach/onDetach -- that was a mistake that will be rectified).

Thanks,

joel.
 

[Scott Blum]

On 6/27/07, Joel Webber <j...@google.com> wrote:

Can anyone see an issue with simply moving Panel.onAttach()'s super call to the end of the method, to at least fix the problem at hand? Before we start discussing stuff like addAttachListener(), I want to first pin down the correct ordering of this invocation, and ensure that the one guarantee we make in onLoad (namely that the widget will be attached at that point) is maintained.

I think this is right.  If I understand this correctly, it has the nice effect that onAttach() should run from head to leaf, while onLoad() should run from leaf to head.

Scott

[Sandy McArthur]

This will have the opposite effect. onAttach() will be called leaf to head.

If you have a detached Panel that contains a Label the attaching flow would be:

* Panel.onAttach()
for each child Widget calls:
* Label.onAttach() which would eventually call the Label's onLoad()
exit back to Panel.onAttach() which calls
* super.onAttach()
which sets attached to true, sets event listeners, can calls
* this.onLoad()

This means that contained widgets will be "attached" and "loaded"
before their parents are. If there were an exception during the attach
process you're left with some attached children widgets and an
unattached parent panel.

It seems reasonable to me that:
* parent widgets should be attached before children widgets.
* children widgets should be detached before parent widgets.

[gwt.team...@gmail.com]

Any time you have an absolutely positioned DOM whose position is
relative to the size of its parent Widget, you need to wait until you
have dimensions before moving it into position. I expect that almost
any Widget more complicated than the built in HTML inputs will
probably have this problem.

Also, we should come up with a standard way of handling DOM resizes
that we cannot catch. For example, the ScrollTable I'm working on
listens for WindowResize events, but in order to handle random DOM
manipulation, I added a Timer that periodically compares the old
dimensions to the current dimensions of the parent element. This
works in testing, but I'm not sure how it would hold up if every
Widget had its own Timer and dimension comparisons.

One option is to have a ResizableWidget class with an abstract
onresize() method, and a non-abstract "redraw" method. onResize will
be called whenever the DOM resizes through some catchable event
(WindowResize, SplitPanel resizing), but the user can call the
redraw() method manually if needed. A static Timer() will
periodically check the dimensions of all ResizableWidget instances,
and if they change, it will call the onresize() method.

On Jun 27, 6:17 pm, "Sandy McArthur" <sandy...@gmail.com> wrote:
> On 6/27/07, Scott Blum <sco...@google.com> wrote:
>
> > On 6/27/07, Joel Webber <j...@google.com> wrote:
> > > Can anyone see an issue with simply moving Panel.onAttach()'s super call
> > to the end of the method, to at least fix the problem at hand? Before we
> > start discussing stuff like addAttachListener(), I want to first pin down
> > the correct ordering of this invocation, and ensure that the one guarantee

> > we make inonLoad(namely that thewidgetwill be attached at that point) is

> > maintained.
>
> > I think this is right. If I understand this correctly, it has the nice

> > effect that onAttach() should run from head to leaf, whileonLoad() should

[Scott Blum]

On 6/27/07, Sandy McArthur <sand...@gmail.com> wrote:

> I think this is right.  If I understand this correctly, it has the nice
> effect that onAttach() should run from head to leaf, while onLoad() should
> run from leaf to head.

This will have the opposite effect. onAttach() will be called leaf to head.

If you have a detached Panel that contains a Label the attaching flow would be:

* Panel.onAttach()
for each child Widget calls:
* Label.onAttach() which would eventually call the Label's onLoad()
exit back to Panel.onAttach() which calls
* super.onAttach()
which sets attached to true, sets event listeners, can calls
* this.onLoad()

onAttach() will run head to leaf, in terms of classes/elements.  I didn't mean superclass vs. subclass.

Scott

[Joel Webber]

Actually, I believe Sandy's right here. In the formulation I suggested, onAttach() will actually run from leaf to head, because I placed super.onAttach() at the end of Panel.onAttach, after the recursion to children.

I've attached a patch that I believe addresses all the issues we've been discussing. It should ensure that:

- onAttach() runs top to bottom, so that the hierarchy can't get in an inconsistent state.

- onLoad() runs bottom to top, so that Panels can assume all of their children are already attached in onLoad()

- onDetach() runs bottom to top, for the same reason that onAttach() runs the other way.

- onUnload() also runs bottom to top, which seems to make sense as well.

It does this by splitting Widget.onAttach's implementation into two pieces. It should be fairly clear from the patch.

Please let me know if anyone sees any holes in this formulation, so I can put the patch up for formal review.

Thanks,

joel.

[Scott Blum]

On 6/28/07, Joel Webber <j...@google.com> wrote:

I've attached a patch that I believe addresses all the issues we've been discussing. It should ensure that:

- onAttach() runs top to bottom, so that the hierarchy can't get in an inconsistent state.

- onLoad() runs bottom to top, so that Panels can assume all of their children are already attached in onLoad()

- onDetach() runs bottom to top, for the same reason that onAttach() runs the other way.

- onUnload() also runs bottom to top, which seems to make sense as well.

It does this by splitting Widget.onAttach's implementation into two pieces. It should be fairly clear from the patch.

Please let me know if anyone sees any holes in this formulation, so I can put the patch up for formal review.

- onUnload() also runs bottom to top, which seems to make sense as well.

My intuition is that this should run the other way.  It seems nice to me that onAttach/onLoad should run in opposite directions, and the same for onUnload/onDetach.  That way you have a good story for cases where you need to do things before or after your children.

Scott

[Sandy McArthur]

On 6/28/07, Joel Webber <j...@google.com> wrote:

> attemptToFixOnLoadAndStuff.patch
> 5K Download

The patch name indicates that you understand the problem with onLoad
too, why stick with the more complicated solution when a simpler one
is available. With this patch the requirement to always call
super.onAttach() is now a sometimes requirement and Widget's API
surface has increased.

What happened to "do the simplest thing that could possibly work"
http://www.google.com/search?q=Do+the+simplest+thing+that+could+possibly+work

The simplest thing that meets everybody's needs is to use onAttach.
Trying to support onLoad is adding complexity which will increase the
compiled app's size, decrease it's speed and makes the code a little
harder to understand.

[Joel Webber]

The patch name indicates that you understand the problem with onLoad
too, why stick with the more complicated solution when a simpler one
is available. With this patch the requirement to always call
super.onAttach() is now a sometimes requirement and Widget's API
surface has increased.

What happened to "do the simplest thing that could possibly work"
http://www.google.com/search?q=Do+the+simplest+thing+that+could+possibly+work

The simplest thing that meets everybody's needs is to use onAttach.
Trying to support onLoad is adding complexity which will increase the
compiled app's size, decrease it's speed and makes the code a little
harder to understand.

I understand that onLoad was being called in an order that was causing problems for Panel. I see no reason why this is a fundamental problem -- it now has semantics that are clearly different (and useful) from onAttach. I also believe it is valuable to have a method that you can override with impunity, not having to worry about when/how you call it's superclass implementation. Finally, onLoad will continue to exist whether we deprecate it or not, so we should make sure its behavior is well-defined and sensible.

So does this address all the fundamental issues everyone is aware of?

[Sandy McArthur]

On 6/28/07, Joel Webber <j...@google.com> wrote:

> So does this address all the fundamental issues everyone is aware of?

Tree.onAttach will need to be reworked like Panel.onAttach.

Also I just noticed Composite.onAttach isn't right but for a different
reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.

I think Composite methods should look like:

protected void onAttach() {
// Call onAttach() on behalf of the contained widget.
widget.onAttach();
onLoad();
}

protected void onDetach() {
// Call onDetach() on behalf of the contained widget.
widget.onDetach();
onUnload();
}

public boolean isAttached() {
return widget.isAttached();
}

Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.

[Joel Webber]

All,

@knorton: can you have a look at this as well? I want as many eyes on this as possible.

I'm attaching a patch that I think will finally clear all this stuff up, and adds a test to keep us honest in the future.

 

To deal with the weird issues in overriding onAttach/onDetach, I changed their implementations in Widget to call the template methods doAttachChildren/doDetachChildren. If you call your childrens' onAttach/onDetach within this template, it will happen at exactly the right time.

For onDetach/onUnload, I settled on having onUnload happen *before* actually detaching, as this is almost always what you need in practice. To give an example, the CheckBox.onUnload() needs to store the state of its 'checked' property *before* being removed from the DOM, because some browsers clobber that value when it's detached (go figure...). I believe the same is true for RichText (though I haven't implemented it yet).

 

Also I just noticed Composite.onAttach isn't right but for a different
reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.

Good call. Done. 

Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.

Also a good call. Done as well.

Thanks again everyone for your patience in helping me get these issues pinned down.

joel.

 

[Kelly Norton]

Oops, sorry. I read over this and still missed the fact that my name was on it.

Just a few comments:

Widget.java
@87 & 118 - Intuitively it seems like onAttach and onDetach should be symmetric operations on the tree, so I'm wondering why onDetach isn't:

    try {
      onUnload();
    } finally {
      doDetachChildren();
      DOM.setEventListener(getElement(), null);
      attached = false;
    }

@151 & 159 - It seems odd to me that onAttach and doAttachChildren do similar things but have slightly different names. onAttachChildren seems like the obvious choice for consistency, is there something different about those methods to warrant a new auxiliary verb?

@156 - File needs a sort. doAttachChildren is out of order.

Composite.java
@96 - Widget.onDetach guards against exceptions being thrown in the local onUnload, but Composite.onDetach does not? Should it?

Panel.java
@98 - "only once" made me think you were saying something else. I think the same wording used in onUnload's doc will work:
"A Panel's onLoad method will be called after all of its children are attached."

WidgetOnLoadTest
@1 - Needs a header to appease checkstyle.

@7 - Needs a javadoc to appease checkstyle.

/kel

On 7/2/07, Joel Webber <j...@google.com > wrote:

All,

@knorton: can you have a look at this as well? I want as many eyes on this as possible.

I'm attaching a patch that I think will finally clear all this stuff up, and adds a test to keep us honest in the future.

 

To deal with the weird issues in overriding onAttach/onDetach, I changed their implementations in Widget to call the template methods doAttachChildren/doDetachChildren. If you call your childrens' onAttach/onDetach within this template, it will happen at exactly the right time.

For onDetach/onUnload, I settled on having onUnload happen *before* actually detaching, as this is almost always what you need in practice. To give an example, the CheckBox.onUnload() needs to store the state of its 'checked' property *before* being removed from the DOM, because some browsers clobber that value when it's detached (go figure...). I believe the same is true for RichText (though I haven't implemented it yet).

 

Also I just noticed Composite. onAttach isn't right but for a different

reason (and has been wrong since at least 1.3.3). It shouldn't be
calling super.onAttach because DOM.setEventListener(getElement(),
this); ends up being called twice on the same DOM element, once for
the Composite instance and once for the Composite's wrapped Widget
instance.

Good call. Done. 

Also, while not currently a problem, direct access to the "attached"
field in Widget only happens to work because it defaults the the right
value needed when Composite.initWidget must be called. It would be
prudent to replace the field access with calls to isAttached() to
prevent future bugs.

Also a good call. Done as well.

Thanks again everyone for your patience in helping me get these issues pinned down.

joel.

 

--
If you received this communication by mistake, you are entitled to one free ice cream cone on me. Simply print out this email including all relevant SMTP headers and present them at my desk to claim your creamy treat. We'll have a laugh at my emailing incompetence, and play a game of ping pong. (offer may not be valid in all States).

[Joel Webber]

Updated patch attached.

On 7/6/07, Kelly Norton <kno...@google.com > wrote:

Oops, sorry. I read over this and still missed the fact that my name was on it.

Just a few comments:

Widget.java
@87 & 118 - Intuitively it seems like onAttach and onDetach should be symmetric operations on the tree, so I'm wondering why onDetach isn't:

    try {
      onUnload();
    } finally {
      doDetachChildren();
      DOM.setEventListener(getElement(), null);
      attached = false;
    }

Good point. That could lead to some subtle differences. Fixed. 

@151 & 159 - It seems odd to me that onAttach and doAttachChildren do similar things but have slightly different names. onAttachChildren seems like the obvious choice for consistency, is there something different about those methods to warrant a new auxiliary verb?

Here's my logic -- see if you think it makes sense:

- onAttach() is called when the widget *is attached*

- doAttachChildren() is a template method giving the subclass the opportunity to do any attachment of children (using the more or less standard doWhatever() naming convention).

@156 - File needs a sort. doAttachChildren is out of order.

Done. 

Composite.java
@96 - Widget.onDetach guards against exceptions being thrown in the local onUnload, but Composite.onDetach does not? Should it?

Another good catch. Fixed.

Panel.java
@98 - "only once" made me think you were saying something else. I think the same wording used in onUnload's doc will work:
"A Panel's onLoad method will be called after all of its children are attached."

Good point. Sounds much less odd that way. 

WidgetOnLoadTest
@1 - Needs a header to appease checkstyle.

@7 - Needs a javadoc to appease checkstyle.

Done, thanks. I really need to re-enable checkstyle. 

[Fred Sauer]

This patch also fixes #1121.

Yeah!

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

[Kelly Norton]

The goods are below.

Here's my logic -- see if you think it makes sense:

- onAttach() is called when the widget *is attached*

- doAttachChildren() is a template method giving the subclass the opportunity to do any attachment of children (using the more or less standard doWhatever() naming convention).

I think that makes sense. After I sent the last review, I started thinking your naming made sense from a Widget implementers standpoint. 

There are a few order issues remaining:
Tree.java - doDetachChildren & doAttachChildren are below isKeyboardNavigationEnabled.
Composite.java - isAttached is in the protected section.

Other than than LGTM.

/kel

[Joel Webber]

Thanks. Sorting done, & committed as r1229.

[Fred Sauer]

Thanks, guys.

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