Fix for issue 4694. Adds a 'visibility' property to layout layers, along with
a hack to LayoutImplIE8 that ensures relative units are recalculated
correctly on visibility change. Also changes LayoutPanel and TabLayoutPanel
to ensure that they use this new layer property, rather than modifying the
container element's style directly.
Review at http://gwt-code-reviews.appspot.com/869801
Review by: jlabanca@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@8775 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/layout/client/Layout.java b/user/src/com/google/gwt/layout/client/Layout.java
index c5d9d10..2bd46bf 100644
--- a/user/src/com/google/gwt/layout/client/Layout.java
+++ b/user/src/com/google/gwt/layout/client/Layout.java
@@ -159,6 +159,7 @@
targetHeight;
Alignment hPos = Alignment.STRETCH, vPos = Alignment.STRETCH;
+ boolean visible = true;
Layer(Element container, Element child, Object userObject) {
this.container = container;
@@ -172,7 +173,7 @@
* <p>
* This is the element that sits between the parent and child elements. It
* is normally necessary to operate on this element only when you need to
- * modify certain CSS properties, such as visibility.
+ * modify CSS properties that are not directly modeled by the Layer class.
* </p>
*
* @return the container element
@@ -315,6 +316,15 @@
this.targetTopUnit = topUnit;
this.targetHeightUnit = heightUnit;
}
+
+ /**
+ * Sets the layer's visibility.
+ *
+ * @param visible
+ */
+ public void setVisible(boolean visible) {
+ this.visible = visible;
+ }
}
private LayoutImpl impl = GWT.create(LayoutImpl.class);
diff --git a/user/src/com/google/gwt/layout/client/LayoutImpl.java b/user/src/com/google/gwt/layout/client/LayoutImpl.java
index 5c3b647..a21f3f7 100644
--- a/user/src/com/google/gwt/layout/client/LayoutImpl.java
+++ b/user/src/com/google/gwt/layout/client/LayoutImpl.java
@@ -23,6 +23,7 @@
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.Element;
import com.google.gwt.dom.client.Style;
+import com.google.gwt.dom.client.Style.Display;
import com.google.gwt.dom.client.Style.Overflow;
import com.google.gwt.dom.client.Style.Position;
import com.google.gwt.dom.client.Style.Unit;
@@ -138,6 +139,12 @@
public void layout(Layer layer) {
Style style = layer.container.getStyle();
+ if (layer.visible) {
+ style.clearDisplay();
+ } else {
+ style.setDisplay(Display.NONE);
+ }
+
style.setProperty(
"left", layer.setLeft ? (layer.left + layer.leftUnit.getType()) : "");
style.setProperty(
diff --git a/user/src/com/google/gwt/layout/client/LayoutImplIE6.java b/user/src/com/google/gwt/layout/client/LayoutImplIE6.java
index 9be077b..70a0856 100644
--- a/user/src/com/google/gwt/layout/client/LayoutImplIE6.java
+++ b/user/src/com/google/gwt/layout/client/LayoutImplIE6.java
@@ -20,6 +20,8 @@
import com.google.gwt.dom.client.DivElement;
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.Style;
+import com.google.gwt.dom.client.Style.Display;
import com.google.gwt.dom.client.Style.Overflow;
import com.google.gwt.dom.client.Style.Position;
import com.google.gwt.event.logical.shared.ResizeEvent;
@@ -154,7 +156,15 @@
@Override
public void layout(Layer layer) {
- Element elem = layer.getContainerElement();
+ Element elem = layer.container;
+ Style style = elem.getStyle();
+
+ if (layer.visible) {
+ style.clearDisplay();
+ } else {
+ style.setDisplay(Display.NONE);
+ }
+
setLayer(elem, layer);
}
diff --git a/user/src/com/google/gwt/layout/client/LayoutImplIE8.java b/user/src/com/google/gwt/layout/client/LayoutImplIE8.java
index 344f7fb..63d487b 100644
--- a/user/src/com/google/gwt/layout/client/LayoutImplIE8.java
+++ b/user/src/com/google/gwt/layout/client/LayoutImplIE8.java
@@ -15,22 +15,53 @@
*/
package com.google.gwt.layout.client;
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.Node;
+import com.google.gwt.dom.client.NodeList;
import com.google.gwt.dom.client.Style;
+import com.google.gwt.dom.client.Style.Display;
import com.google.gwt.dom.client.Style.Unit;
import com.google.gwt.layout.client.Layout.Layer;
/**
- * This implementation is used on IE7 and IE8. Unlike {@link LayoutImpl}, it
- * converts all values to pixels before setting them. This is necessary because
- * these browsers incorrectly calculate the relative sizes and positions of CSS
+ * This implementation is used on IE8. Unlike {@link LayoutImpl}, it converts
+ * all values to pixels before setting them. This is necessary because this
+ * browser incorrectly calculates the relative sizes and positions of CSS
* properties specified in certain units (e.g., when the value of an 'em' is
* non-integral in pixels).
*/
public class LayoutImplIE8 extends LayoutImpl {
+ private static native Layer getLayer(Element container) /*-{
+ return container.__layer;
+ }-*/;
+
+ private static native void setLayer(Element container, Layer layer) /*-{
+ // Potential leak: This is cleaned up in detach().
+ container.__layer = layer;
+ }-*/;
+
@Override
public void layout(Layer layer) {
Style style = layer.container.getStyle();
+ setLayer(layer.container, layer);
+
+ // Whenever the visibility of a layer changes, we need to ensure that
+ // layout() is run again for it. This is because the translation of units
+ // to pixel values will be incorrect for invisible elements, and thus must
+ // be fixed when they become visible.
+ if (layer.visible) {
+ String oldDisplay = style.getDisplay();
+ style.clearDisplay();
+
+ // We control the layer element, so assume that any non-zero display
+ // property means it was set to 'none'.
+ if (oldDisplay.length() > 0) {
+ updateVisibility(layer.container);
+ }
+ } else {
+ style.setDisplay(Display.NONE);
+ }
if (layer.setLeft) {
setValue(layer, "left", layer.left, layer.leftUnit, false, false);
@@ -95,6 +126,20 @@
}
}
+ @Override
+ public void onDetach(Element parent) {
+ removeLayerRefs(parent);
+ }
+
+ private native void removeLayerRefs(Element parent) /*-{
+ for (var i = 0; i < parent.childNodes.length; ++i) {
+ var container = parent.childNodes[i];
+ if (container.__layer) {
+ container.__layer = null;
+ }
+ }
+ }-*/;
+
private void setValue(Layer layer, String prop, double value, Unit unit,
boolean vertical, boolean noNegative) {
switch (unit) {
@@ -119,4 +164,23 @@
layer.getContainerElement().getStyle().setProperty(prop,
(int) (value + 0.5), unit);
}
+
+ private void updateVisibility(Element container) {
+ // If this element has an associated layer, re-run layout for it.
+ Layer layer = getLayer(container);
+ if (layer != null) {
+ layout(layer);
+ }
+
+ // Walk all children, looking for elements with a '__layer' property. If one
+ // exists, call layout() for that element. This is not cheap, but it's the
+ // only way to correctly ensure that layout units get translated correctly.
+ NodeList<Node> nodes = container.getChildNodes();
+ for (int i = 0; i < nodes.getLength(); ++i) {
+ Node node = nodes.getItem(i);
+ if (node.getNodeType() == Node.ELEMENT_NODE) {
+ updateVisibility(node.<Element>cast());
+ }
+ }
+ }
}
diff --git a/user/src/com/google/gwt/user/client/ui/LayoutPanel.java b/user/src/com/google/gwt/user/client/ui/LayoutPanel.java
index 9b918bb..52f2ebb 100644
--- a/user/src/com/google/gwt/user/client/ui/LayoutPanel.java
+++ b/user/src/com/google/gwt/user/client/ui/LayoutPanel.java
@@ -344,8 +344,7 @@
*/
public void setWidgetVisible(Widget child, boolean visible) {
assertIsChild(child);
- Element container = getWidgetContainerElement(child);
- setVisible(container, visible);
+ getLayer(child).setVisible(visible);
child.setVisible(visible);
animate(0);
}
diff --git a/user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java b/user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java
index 010f5a7..53733a2 100644
--- a/user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java
+++ b/user/src/com/google/gwt/user/client/ui/TabLayoutPanel.java
@@ -18,7 +18,6 @@
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.Element;
import com.google.gwt.dom.client.Style;
-import com.google.gwt.dom.client.Style.Display;
import com.google.gwt.dom.client.Style.Unit;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
@@ -601,8 +600,7 @@
private void layoutChild(Widget child) {
panel.setWidgetLeftRight(child, 0, Unit.PX, 0, Unit.PX);
panel.setWidgetTopBottom(child, barHeight, barUnit, 0, Unit.PX);
- panel.getWidgetContainerElement(child).getStyle().setDisplay(
- Display.NONE);
+ panel.setWidgetVisible(child, false);
child.addStyleName(CONTENT_STYLE);
child.setVisible(false);
}
diff --git a/user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java b/user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java
index 27dbd1d..8cb1ce5 100644
--- a/user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java
+++ b/user/test/com/google/gwt/user/client/ui/TabLayoutPanelTest.java
@@ -107,6 +107,49 @@
});
}
+ /**
+ * Ensures that hidden children are layed out properly when their tabs are
+ * selected, when they're sized in EM units. This has been a problem on IE8
+ * (see issue 4694).
+ */
+ @DoNotRunWith({Platform.HtmlUnitBug})
+ public void testHiddenChildLayoutEM() {
+ final TabLayoutPanel p = new TabLayoutPanel(2, Unit.EM);
+ p.setSize("128px", "128px");
+ RootPanel.get().add(p);
+
+ final Label foo = new Label("foo");
+ p.add(foo, new Label("foo"));
+
+ // Add the 'bar' label in a nested LayoutPanel. This is meant to test that
+ // layout propagates correctly into nested layout panels (something that
+ // should happen naturally, but we have a specific hack for on IE8).
+ final DockLayoutPanel inner = new DockLayoutPanel(Unit.EM);
+ final Label bar = new Label("bar");
+ inner.addSouth(bar, 2);
+ inner.add(new Label("center"));
+ p.add(inner, new Label("bar"));
+
+ delayTestFinish(2000);
+ DeferredCommand.addCommand(new Command() {
+ public void execute() {
+ p.selectTab(1);
+ DeferredCommand.addCommand(new Command() {
+ public void execute() {
+ // Assert that the 'bar' label is of non-zero size on both axes.
+ // The problem fixed in issue 4694 was causing its height to be
+ // zero on IE8, because the EM units weren't being calculated
+ // properly when it was initially hidden.
+ assertTrue(bar.getOffsetWidth() > 0);
+ assertTrue(bar.getOffsetHeight() > 0);
+
+ finishTest();
+ }
+ });
+ }
+ });
+ }
+
public void testInsertBeforeSelected() {
TabLayoutPanel p = new TabLayoutPanel(2, Unit.EM);
p.add(new Label("foo"), "foo");
