diff options
3 files changed, 215 insertions, 4 deletions
diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/ComponentRendererConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/ComponentRendererConnector.java index cc8647e58f..649cec0b36 100644 --- a/client/src/main/java/com/vaadin/client/connectors/grid/ComponentRendererConnector.java +++ b/client/src/main/java/com/vaadin/client/connectors/grid/ComponentRendererConnector.java @@ -15,12 +15,20 @@ */ package com.vaadin.client.connectors.grid; +import java.util.HashSet; +import java.util.Iterator; +import java.util.logging.Logger; + import com.google.gwt.core.client.GWT; +import com.google.gwt.event.shared.HandlerRegistration; import com.google.gwt.user.client.ui.SimplePanel; +import com.google.gwt.user.client.ui.Widget; import com.vaadin.client.ComponentConnector; import com.vaadin.client.ConnectorMap; import com.vaadin.client.renderers.Renderer; import com.vaadin.client.renderers.WidgetRenderer; +import com.vaadin.client.ui.AbstractComponentConnector; +import com.vaadin.client.ui.AbstractConnector; import com.vaadin.client.widget.grid.RendererCellReference; import com.vaadin.shared.ui.Connect; import com.vaadin.shared.ui.grid.renderers.ComponentRendererState; @@ -35,7 +43,10 @@ import com.vaadin.ui.renderers.ComponentRenderer; */ @Connect(ComponentRenderer.class) public class ComponentRendererConnector - extends AbstractGridRendererConnector<String> { + extends AbstractGridRendererConnector<String> { + + private HashSet<String> knownConnectors = new HashSet<>(); + private HandlerRegistration handlerRegistration; @Override protected Renderer<String> createRenderer() { @@ -50,13 +61,22 @@ public class ComponentRendererConnector @Override public void render(RendererCellReference cell, String connectorId, - SimplePanel widget) { + SimplePanel widget) { + createConnectorHierarchyChangeHandler(); + Widget connectorWidget = null; if (connectorId != null) { ComponentConnector connector = (ComponentConnector) ConnectorMap - .get(getConnection()).getConnector(connectorId); - widget.setWidget(connector.getWidget()); + .get(getConnection()).getConnector(connectorId); + if (connector != null) { + connectorWidget = connector.getWidget(); + knownConnectors.add(connectorId); + } + } + if (connectorWidget != null) { + widget.setWidget(connectorWidget); } else if (widget.getWidget() != null) { widget.remove(widget.getWidget()); + knownConnectors.remove(connectorId); } } }; @@ -66,4 +86,39 @@ public class ComponentRendererConnector public ComponentRendererState getState() { return (ComponentRendererState) super.getState(); } + + @Override + public void onUnregister() { + unregisterHierarchyHandler(); + super.onUnregister(); + } + + /** + * Adds a listener for grid hierarchy changes to find detached connectors + * previously handled by this renderer in order to detach from DOM their widgets + * before {@link AbstractComponentConnector#onUnregister()} is invoked + * otherwise an error message is logged. + */ + private void createConnectorHierarchyChangeHandler() { + if (handlerRegistration == null) { + handlerRegistration = getGridConnector().addConnectorHierarchyChangeHandler(event -> { + Iterator<String> iterator = knownConnectors.iterator(); + while (iterator.hasNext()) { + ComponentConnector connector = (ComponentConnector) ConnectorMap.get(getConnection()).getConnector(iterator.next()); + if (connector != null && connector.getParent() == null) { + connector.getWidget().removeFromParent(); + iterator.remove(); + } + } + }); + } + } + + private void unregisterHierarchyHandler() { + if (this.handlerRegistration != null) { + this.handlerRegistration.removeHandler(); + this.handlerRegistration = null; + } + } + } diff --git a/uitest/src/main/java/com/vaadin/tests/components/grid/GridComponentsVisibility.java b/uitest/src/main/java/com/vaadin/tests/components/grid/GridComponentsVisibility.java new file mode 100644 index 0000000000..8c8810f7a8 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/grid/GridComponentsVisibility.java @@ -0,0 +1,66 @@ +package com.vaadin.tests.components.grid; + +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; +import java.util.stream.IntStream; + +import com.vaadin.annotations.Widgetset; +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUIWithLog; +import com.vaadin.ui.Button; +import com.vaadin.ui.Grid; +import com.vaadin.ui.Label; +import com.vaadin.ui.TextField; +import com.vaadin.ui.renderers.ComponentRenderer; + +@Widgetset("com.vaadin.DefaultWidgetSet") +public class GridComponentsVisibility extends AbstractTestUIWithLog { + + private Map<String, TextField> textFields = new HashMap<>(); + private int counter = 0; + + @Override + protected void setup(VaadinRequest request) { + Grid<String> grid = new Grid<>(); + grid.addColumn(string -> new Label(string), new ComponentRenderer()) + .setId("label").setCaption("Column 0"); + grid.getDefaultHeaderRow().getCell("label") + .setComponent(new Label("Label")); + grid.addComponentColumn(string -> { + if (textFields.containsKey(string)) { + log("Reusing old text field for: " + string); + return textFields.get(string); + } + TextField textField = new TextField(); + textField.setValue(string); + textField.setWidth("100%"); + textFields.put(string, textField); + return textField; + }).setId("textField").setCaption("TextField"); + grid.addColumn(string -> { + + Button button = new Button("Click Me!", + event -> toggleFieldVisibility(string)); + button.setId(string.replace(' ', '_').toLowerCase(Locale.ROOT)); + return button; + }, new ComponentRenderer()).setId("button").setCaption("Button"); + // make sure the buttons and focus outlines fit completely in a row + grid.setRowHeight(40); + + grid.getDefaultHeaderRow().join("textField", "button") + .setText("Other Components"); + + addComponent(grid); + grid.setSizeFull(); + + grid.setItems(IntStream.range(0, 5).boxed() + .map(i -> "Row " + (i + (counter * 1000)))); + + } + + private void toggleFieldVisibility(String string) { + textFields.get(string).setVisible(!textFields.get(string).isVisible()); + } + +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridComponentsVisibilityTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridComponentsVisibilityTest.java new file mode 100644 index 0000000000..14fa82a58e --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridComponentsVisibilityTest.java @@ -0,0 +1,90 @@ +package com.vaadin.tests.components.grid; + +import java.util.concurrent.ThreadLocalRandom; +import java.util.function.Function; +import java.util.logging.Level; +import java.util.stream.IntStream; + +import com.vaadin.testbench.By; +import com.vaadin.testbench.elements.GridElement; +import com.vaadin.testbench.elements.TextFieldElement; +import com.vaadin.tests.tb3.MultiBrowserTest; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class GridComponentsVisibilityTest extends MultiBrowserTest { + + @Test + public void changingVisibilityOfComponentInFirstRowShouldNotThrowClientSideExceptions() { + testHideComponent(grid -> 0); + } + + @Test + public void changingVisibilityOfComponentShouldNotThrowClientSideExceptions() { + testHideComponent(grid -> ThreadLocalRandom.current().nextInt(1, (int) grid.getRowCount() - 1)); + } + + @Test + public void changingVisibilityOfComponentInLastRowShouldNotThrowClientSideExceptions() { + testHideComponent(grid -> (int) grid.getRowCount() - 1); + } + + private void testHideComponent(Function<GridElement, Integer> rowUnderTestSupplier) { + openTestURL("debug"); + GridElement grid = $(GridElement.class).first(); + int rowUnderTest = rowUnderTestSupplier.apply(grid); + assertTrue("Text field should be visible", grid.getCell(rowUnderTest, 1) + .isElementPresent(TextFieldElement.class)); + assertOtherConnectorsArePresent(grid, rowUnderTest); + + clearDebugMessages(); + clickVisibilityToggleButton(grid, rowUnderTest); + assertNotClientSideErrors(); + assertOnlyTextFieldOnTestedRowIsNotPresent(grid, rowUnderTest); + + clearDebugMessages(); + clickVisibilityToggleButton(grid, rowUnderTest); + assertNotClientSideErrors(); + assertAllTextFieldsArePresent(grid, rowUnderTest); + + clearDebugMessages(); + clickVisibilityToggleButton(grid, rowUnderTest); + assertNotClientSideErrors(); + assertOnlyTextFieldOnTestedRowIsNotPresent(grid, rowUnderTest); + + clickVisibilityToggleButton(grid, rowUnderTest); + } + + private void assertOnlyTextFieldOnTestedRowIsNotPresent(GridElement grid, int rowUnderTest) { + assertFalse("Text field should not be visible", grid.getCell(rowUnderTest, 1) + .isElementPresent(TextFieldElement.class)); + assertOtherConnectorsArePresent(grid, rowUnderTest); + } + + private void assertAllTextFieldsArePresent(GridElement grid, int rowUnderTest) { + assertTrue("Text field should be visible", grid.getCell(rowUnderTest, 1) + .isElementPresent(TextFieldElement.class)); + assertOtherConnectorsArePresent(grid, rowUnderTest); + } + + private void assertNotClientSideErrors() { + assertNoErrorNotifications(); + // Should not log "Widget is still attached to the DOM ..." error message + assertNoDebugMessage(Level.SEVERE); + } + + private void clickVisibilityToggleButton(GridElement grid, int rowUnderTest) { + grid.getRow(rowUnderTest).getCell(2).findElement(By.className("v-button")).click(); + } + + private void assertOtherConnectorsArePresent(GridElement grid, int rowUnderTest) { + IntStream.range(1, (int) grid.getRowCount()) + .filter(row -> row != rowUnderTest) + .forEach(row -> + assertTrue("Text field should be visible", grid.getCell(row, 1) + .isElementPresent(TextFieldElement.class)) + ); + } +} |