Fixes for three issues:
1652: Use getElement() consistently in UIObject and Widget
2195: Change UIObject.setElement() so that it's callable only once
2196: Remove Composite.getElement()
2197: Composite.onBrowserEvent() is never called
I have also confirmed that the changes for issue 1652 (along with removing Composite.getElement()) generate a compiled KitchenSink with no references to getElement() (it's inlined in every case).
Patch by: jgw
Review by: bruce (desk review)
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@2133 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/client/ui/AbsolutePanel.java b/user/src/com/google/gwt/user/client/ui/AbsolutePanel.java
index 8118e29..1cd716b 100644
--- a/user/src/com/google/gwt/user/client/ui/AbsolutePanel.java
+++ b/user/src/com/google/gwt/user/client/ui/AbsolutePanel.java
@@ -51,7 +51,17 @@
* Creates an empty absolute panel.
*/
public AbsolutePanel() {
- setElement(DOM.createDiv());
+ this(DOM.createDiv());
+ }
+
+ /**
+ * Creates an AbsolutePanel with the given element. This is package-protected
+ * so that it can be used by {@link RootPanel}.
+ *
+ * @param elem the element to be used for this panel
+ */
+ AbsolutePanel(Element elem) {
+ setElement(elem);
// Setting the panel's position style to 'relative' causes it to be treated
// as a new positioning context for its children.
diff --git a/user/src/com/google/gwt/user/client/ui/Composite.java b/user/src/com/google/gwt/user/client/ui/Composite.java
index 1a7b893..7368260 100644
--- a/user/src/com/google/gwt/user/client/ui/Composite.java
+++ b/user/src/com/google/gwt/user/client/ui/Composite.java
@@ -15,7 +15,8 @@
*/
package com.google.gwt.user.client.ui;
-import com.google.gwt.user.client.Element;
+import com.google.gwt.user.client.DOM;
+import com.google.gwt.user.client.Event;
/**
* A type of widget that can wrap another widget, hiding the wrapped widget's
@@ -36,18 +37,6 @@
private Widget widget;
- /**
- * This override checks to ensure {@link #initWidget(Widget)} has been called.
- */
- @Override
- public Element getElement() {
- if (widget == null) {
- throw new IllegalStateException("initWidget() was never called in "
- + this.getClass().getName());
- }
- return super.getElement();
- }
-
@Override
public boolean isAttached() {
if (widget != null) {
@@ -95,8 +84,23 @@
}
@Override
+ public void onBrowserEvent(Event event) {
+ // Delegate events to the widget.
+ widget.onBrowserEvent(event);
+ }
+
+ @Override
protected void onAttach() {
+ // Call the widget's onAttach() first.
widget.onAttach();
+
+ // Clobber the widget's call to setEventListener(), causing all events to
+ // be routed to this composite, which will delegate back to the widget by
+ // default (note: it's not necessary to clear this in onDetach(), because
+ // the widget's onDetach will do so).
+ DOM.setEventListener(getElement(), this);
+
+ // Call onLoad() directly, because we're not calling super.onAttach().
onLoad();
}
diff --git a/user/src/com/google/gwt/user/client/ui/Image.java b/user/src/com/google/gwt/user/client/ui/Image.java
index 979c9a3..e5942f1 100644
--- a/user/src/com/google/gwt/user/client/ui/Image.java
+++ b/user/src/com/google/gwt/user/client/ui/Image.java
@@ -99,7 +99,7 @@
private static class UnclippedState extends State {
UnclippedState(Image image) {
- image.setElement(DOM.createImg());
+ image.replaceElement(DOM.createImg());
image.sinkEvents(Event.ONCLICK | Event.MOUSEEVENTS | Event.ONLOAD
| Event.ONERROR | Event.ONMOUSEWHEEL);
}
@@ -179,7 +179,7 @@
this.width = width;
this.height = height;
this.url = url;
- image.setElement(impl.createStructure(url, left, top, width, height));
+ image.replaceElement(impl.createStructure(url, left, top, width, height));
image.sinkEvents(Event.ONCLICK | Event.MOUSEEVENTS | Event.ONMOUSEWHEEL);
fireSyntheticLoadEvent(image);
}
diff --git a/user/src/com/google/gwt/user/client/ui/NamedFrame.java b/user/src/com/google/gwt/user/client/ui/NamedFrame.java
index 08ce31d..66e0cc9 100644
--- a/user/src/com/google/gwt/user/client/ui/NamedFrame.java
+++ b/user/src/com/google/gwt/user/client/ui/NamedFrame.java
@@ -69,7 +69,7 @@
DOM.setInnerHTML(div, "<iframe name='" + name + "'>");
Element iframe = DOM.getFirstChild(div);
- setElement(iframe);
+ replaceElement(iframe);
}
/**
diff --git a/user/src/com/google/gwt/user/client/ui/RootPanel.java b/user/src/com/google/gwt/user/client/ui/RootPanel.java
index d10b6d6..c1f0738 100644
--- a/user/src/com/google/gwt/user/client/ui/RootPanel.java
+++ b/user/src/com/google/gwt/user/client/ui/RootPanel.java
@@ -57,10 +57,11 @@
*/
public static RootPanel get(String id) {
// See if this RootPanel is already created.
- RootPanel gwt = rootPanels.get(id);
- if (gwt != null) {
- return gwt;
+ RootPanel rp = rootPanels.get(id);
+ if (rp != null) {
+ return rp;
}
+
// Find the element that this RootPanel will wrap.
Element elem = null;
if (id != null) {
@@ -74,8 +75,12 @@
}
// Create the panel and put it in the map.
- rootPanels.put(id, gwt = new RootPanel(elem));
- return gwt;
+ if (elem == null) {
+ // 'null' means use document's body element.
+ elem = getBodyElement();
+ }
+ rootPanels.put(id, rp = new RootPanel(elem));
+ return rp;
}
/**
@@ -108,12 +113,7 @@
}
private RootPanel(Element elem) {
- if (elem == null) {
- // 'null' means use document's body element.
- elem = getBodyElement();
- }
-
- setElement(elem);
+ super(elem);
onAttach();
}
}
diff --git a/user/src/com/google/gwt/user/client/ui/UIObject.java b/user/src/com/google/gwt/user/client/ui/UIObject.java
index 65c7fad..ea27f10 100644
--- a/user/src/com/google/gwt/user/client/ui/UIObject.java
+++ b/user/src/com/google/gwt/user/client/ui/UIObject.java
@@ -82,6 +82,13 @@
* </p>
*/
public abstract class UIObject {
+
+ static final String SETELEMENT_TWICE_ERROR = "Element may only be set once";
+
+ static final String MISSING_ELEMENT_ERROR = "This UIObject's element is not set; "
+ + "you may be missing a call to either Composite.initWidget() or "
+ + "UIObject.setElement()";
+
/**
* The implementation of the set debug id method, which does nothing by
* default.
@@ -440,9 +447,15 @@
/**
* Gets a handle to the object's underlying DOM element.
*
+ * This method should not be overridden. It is non-final solely to support
+ * legacy code that depends upon overriding it. If it is overridden, the
+ * subclass implementation must not return a different element than was
+ * previously set using {@link #setElement(Element)}.
+ *
* @return the object's browser element
*/
public Element getElement() {
+ assert (element != null) : MISSING_ELEMENT_ERROR;
return element;
}
@@ -453,7 +466,7 @@
* @return the object's offset height
*/
public int getOffsetHeight() {
- return DOM.getElementPropertyInt(element, "offsetHeight");
+ return DOM.getElementPropertyInt(getElement(), "offsetHeight");
}
/**
@@ -463,7 +476,7 @@
* @return the object's offset width
*/
public int getOffsetWidth() {
- return DOM.getElementPropertyInt(element, "offsetWidth");
+ return DOM.getElementPropertyInt(getElement(), "offsetWidth");
}
/**
@@ -497,7 +510,7 @@
* @return the object's title
*/
public String getTitle() {
- return DOM.getElementProperty(element, "title");
+ return DOM.getElementProperty(getElement(), "title");
}
/**
@@ -506,7 +519,7 @@
* @return <code>true</code> if the object is visible
*/
public boolean isVisible() {
- return isVisible(element);
+ return isVisible(getElement());
}
/**
@@ -543,7 +556,7 @@
// This exists to deal with an inconsistency in IE's implementation where
// it won't accept negative numbers in length measurements
assert extractLengthValue(height.trim().toLowerCase()) >= 0 : "CSS heights should not be negative";
- DOM.setStyleAttribute(element, "height", height);
+ DOM.setStyleAttribute(getElement(), "height", height);
}
/**
@@ -605,9 +618,9 @@
*/
public void setTitle(String title) {
if (title == null || title.length() == 0) {
- DOM.removeElementAttribute(element, "title");
+ DOM.removeElementAttribute(getElement(), "title");
} else {
- DOM.setElementAttribute(element, "title", title);
+ DOM.setElementAttribute(getElement(), "title", title);
}
}
@@ -618,7 +631,7 @@
* to hide it
*/
public void setVisible(boolean visible) {
- setVisible(element, visible);
+ setVisible(getElement(), visible);
}
/**
@@ -631,7 +644,7 @@
// This exists to deal with an inconsistency in IE's implementation where
// it won't accept negative numbers in length measurements
assert extractLengthValue(width.trim().toLowerCase()) >= 0 : "CSS widths should not be negative";
- DOM.setStyleAttribute(element, "width", width);
+ DOM.setStyleAttribute(getElement(), "width", width);
}
/**
@@ -656,10 +669,10 @@
*/
@Override
public String toString() {
- if (element == null) {
+ if (getElement() == null) {
return "(null handle)";
}
- return DOM.toString(element);
+ return DOM.toString(getElement());
}
/**
@@ -683,7 +696,7 @@
* @return the element to which style names will be applied
*/
protected Element getStyleElement() {
- return element;
+ return getElement();
}
/**
@@ -713,18 +726,28 @@
/**
* Sets this object's browser element. UIObject subclasses must call this
- * method before attempting to call any other methods.
+ * method before attempting to call any other methods, and it may only be
+ * called once.
*
- * If the browser element has already been set, then the current element's
- * position is located in the DOM and removed. The new element is added into
- * the previous element's position.
+ * @param elem the object's element
+ */
+ protected void setElement(Element elem) {
+ assert (element == null) : SETELEMENT_TWICE_ERROR;
+ this.element = elem;
+ }
+
+ /**
+ * Replaces this object's browser element.
+ *
+ * This method exists only to support a specific use-case in Image, and
+ * should not be used by other classes.
*
* @param elem the object's new element
*/
- protected void setElement(Element elem) {
- if (this.element != null) {
+ void replaceElement(Element elem) {
+ if (element != null) {
// replace this.element in its parent with elem.
- replaceNode(this.element, elem);
+ replaceNode(element, elem);
}
this.element = elem;
diff --git a/user/src/com/google/gwt/user/client/ui/Widget.java b/user/src/com/google/gwt/user/client/ui/Widget.java
index f8809c7..e4590cd 100644
--- a/user/src/com/google/gwt/user/client/ui/Widget.java
+++ b/user/src/com/google/gwt/user/client/ui/Widget.java
@@ -164,18 +164,8 @@
protected void onUnload() {
}
- /**
- * Sets this object's browser element. Widget subclasses must call this method
- * before attempting to call any other methods.
- *
- * If a browser element has already been attached, then it is replaced with
- * the new element. The old event listeners are removed from the old browser
- * element, and the event listeners are set up on the new browser element.
- *
- * @param elem the object's new element
- */
@Override
- protected void setElement(Element elem) {
+ void replaceElement(Element elem) {
if (isAttached()) {
// Remove old event listener to avoid leaking. onDetach will not do this
// for us, because it is only called when the widget itself is detached
@@ -183,7 +173,7 @@
DOM.setEventListener(getElement(), null);
}
- super.setElement(elem);
+ super.replaceElement(elem);
if (isAttached()) {
// Hook the event listener back up on the new element. onAttach will not
// do this for us, because it is only called when the widget itself is
diff --git a/user/test/com/google/gwt/user/UISuite.java b/user/test/com/google/gwt/user/UISuite.java
index 3fc844d..b2798e5 100644
--- a/user/test/com/google/gwt/user/UISuite.java
+++ b/user/test/com/google/gwt/user/UISuite.java
@@ -21,6 +21,7 @@
import com.google.gwt.user.client.WindowTest;
import com.google.gwt.user.client.ui.AbsolutePanelTest;
import com.google.gwt.user.client.ui.CheckBoxTest;
+import com.google.gwt.user.client.ui.CompositeTest;
import com.google.gwt.user.client.ui.CustomButtonTest;
import com.google.gwt.user.client.ui.DOMTest;
import com.google.gwt.user.client.ui.DecoratorPanelTest;
@@ -78,6 +79,7 @@
suite.addTestSuite(CheckBoxTest.class);
suite.addTestSuite(ClippedImagePrototypeTest.class);
suite.addTestSuite(CommandExecutorTest.class);
+ suite.addTestSuite(CompositeTest.class);
suite.addTestSuite(CookieTest.class);
suite.addTestSuite(CustomButtonTest.class);
suite.addTestSuite(DecoratorPanelTest.class);
diff --git a/user/test/com/google/gwt/user/client/ui/CompositeTest.java b/user/test/com/google/gwt/user/client/ui/CompositeTest.java
new file mode 100644
index 0000000..0343bc9
--- /dev/null
+++ b/user/test/com/google/gwt/user/client/ui/CompositeTest.java
@@ -0,0 +1,104 @@
+/*
+ * Copyright 2008 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.google.gwt.user.client.ui;
+
+import com.google.gwt.junit.client.GWTTestCase;
+import com.google.gwt.user.client.Command;
+import com.google.gwt.user.client.DOM;
+import com.google.gwt.user.client.DeferredCommand;
+import com.google.gwt.user.client.Event;
+
+/**
+ * Tests for {@link Composite}.
+ */
+public class CompositeTest extends GWTTestCase {
+
+ public String getModuleName() {
+ return "com.google.gwt.user.User";
+ }
+
+ private static class EventTestComposite extends Composite {
+ TextBox tb = new TextBox();
+ boolean widgetFocusFired;
+ boolean widgetLostFocusFired;
+ boolean domFocusFired;
+ boolean domBlurFired;
+
+ public EventTestComposite() {
+ initWidget(tb);
+ sinkEvents(Event.FOCUSEVENTS);
+
+ tb.addFocusListener(new FocusListener() {
+ public void onLostFocus(Widget sender) {
+ widgetLostFocusFired = true;
+ }
+
+ public void onFocus(Widget sender) {
+ widgetFocusFired = true;
+ }
+ });
+ }
+
+ @Override
+ public void onBrowserEvent(Event event) {
+ switch (DOM.eventGetType(event)) {
+ case Event.ONFOCUS:
+ domFocusFired = true;
+ // Eat the focus event.
+ return;
+
+ case Event.ONBLUR:
+ domBlurFired = true;
+ // *Don't* eat the blur event.
+ break;
+ }
+
+ super.onBrowserEvent(event);
+ }
+ }
+
+ public void testBrowserEvents() {
+ final EventTestComposite c = new EventTestComposite();
+ RootPanel.get().add(c);
+
+ this.delayTestFinish(1000);
+
+ // Focus, then blur, the composite's text box. This has to be done in
+ // deferred commands, because focus events usually require the event loop
+ // to be pumped in order to fire.
+ DeferredCommand.addCommand(new Command() {
+ public void execute() {
+ DeferredCommand.addCommand(new Command() {
+ public void execute() {
+ // Ensure all events fired as expected.
+ assertTrue(c.domFocusFired);
+ assertTrue(c.domBlurFired);
+ assertTrue(c.widgetLostFocusFired);
+
+ // Ensure that the widget's focus event was eaten by the
+ // composite's implementation of onBrowserEvent().
+ assertFalse(c.widgetFocusFired);
+ finishTest();
+ }
+ });
+
+ c.tb.setFocus(false);
+ }
+ });
+
+ c.tb.setFocus(true);
+ }
+}
diff --git a/user/test/com/google/gwt/user/client/ui/UIObjectTest.java b/user/test/com/google/gwt/user/client/ui/UIObjectTest.java
index e5dec24..7f9bc0c 100644
--- a/user/test/com/google/gwt/user/client/ui/UIObjectTest.java
+++ b/user/test/com/google/gwt/user/client/ui/UIObjectTest.java
@@ -199,6 +199,43 @@
assertFalse(containsClass(o, "i-awt-heart"));
}
+ public void testMissingElementAssertion() {
+ try {
+ Widget w = new Widget() {
+ };
+
+ w.getElement();
+ fail("Expected assertion failure");
+ } catch (AssertionError e) {
+ assertEquals(UIObject.MISSING_ELEMENT_ERROR, e.getMessage());
+ }
+
+ try {
+ Composite c = new Composite() {
+ };
+
+ c.getElement();
+ fail("Expected assertion failure");
+ } catch (AssertionError e) {
+ assertEquals(UIObject.MISSING_ELEMENT_ERROR, e.getMessage());
+ }
+ }
+
+ public void testSetElementTwiceFails() {
+ UIObject o = new UIObject() {
+ {
+ setElement(DOM.createDiv());
+ }
+ };
+
+ try {
+ o.setElement(DOM.createSpan());
+ fail("Expected assertion failure");
+ } catch (AssertionError e) {
+ assertEquals(UIObject.SETELEMENT_TWICE_ERROR, e.getMessage());
+ }
+ }
+
private void assertPrimaryStyleNameEquals(UIObject o, String className) {
String attr = DOM.getElementProperty(o.getElement(), "className");
assertTrue(attr.indexOf(className) == 0);
