* Fix rendering of TreeGrid's frozen columns after hierarchy-column reset (#12028) * Add tests * Fix getVisibleFrozenColumnCount() if SelectionMode is multi * Update ComboBox popup position comparison to use correct top value. (#12041) Fixes #12029 * Clear thread local instances on connection lost in push handler (#12042) Adopted from https://github.com/vaadin/flow/pull/8567 Co-authored-by: Tarek Oraby <42799254+tarekoraby@users.noreply.github.com> Co-authored-by: Anna Koskinen <Ansku@users.noreply.github.com> Co-authored-by: Tatu Lund <tatu@vaadin.com>tags/8.11.1
@@ -906,7 +906,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
// ComboBox itself may be incorrectly positioned, don't try to | |||
// adjust horizontal popup position yet. Earlier width | |||
// calculations must be performed anyway to avoid flickering. | |||
if (top != topPosition) { | |||
if (top != getAbsoluteTop()) { | |||
// Variable 'left' still contains the original popupLeft, | |||
// 'top' has been updated, thus vertical position needs | |||
// adjusting. | |||
@@ -935,7 +935,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
} | |||
// Only update the position if it has changed. | |||
if (top != topPosition || left != getPopupLeft()) { | |||
if (top != getAbsoluteTop() || left != getPopupLeft()) { | |||
setPopupPosition(left, top); | |||
} | |||
menu.scrollSelectionIntoView(); |
@@ -7416,7 +7416,15 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>, | |||
// for the escalator the hidden columns are not in the frozen column | |||
// count, but for grid they are. thus need to convert the index | |||
for (int i = 0; i < frozenColumnCount; i++) { | |||
int limit = getFrozenColumnCount(); | |||
if (getSelectionColumn().isPresent()) { | |||
// If the grid is in MultiSelect mode, getColumn(0) in the following | |||
// for loop returns the selection column. Accordingly, verifying | |||
// which frozen columns are visible if the selection column is | |||
// present should take this fact into account. | |||
limit++; | |||
} | |||
for (int i = 0; i < limit; i++) { | |||
if (i >= getColumnCount() || getColumn(i).isHidden()) { | |||
numberOfColumns--; | |||
} |
@@ -44,6 +44,7 @@ import com.vaadin.shared.ApplicationConstants; | |||
import com.vaadin.shared.JsonConstants; | |||
import com.vaadin.shared.communication.PushMode; | |||
import com.vaadin.ui.UI; | |||
import com.vaadin.util.CurrentInstance; | |||
import elemental.json.JsonException; | |||
@@ -316,13 +317,29 @@ public class PushHandler { | |||
} | |||
void connectionLost(AtmosphereResourceEvent event) { | |||
VaadinSession session = null; | |||
try { | |||
session = handleConnectionLost(event); | |||
} finally { | |||
if (session != null) { | |||
session.access(new Runnable() { | |||
@Override | |||
public void run() { | |||
CurrentInstance.clearAll(); | |||
} | |||
}); | |||
} | |||
} | |||
} | |||
private VaadinSession handleConnectionLost(AtmosphereResourceEvent event) { | |||
// We don't want to use callWithUi here, as it assumes there's a client | |||
// request active and does requestStart and requestEnd among other | |||
// things. | |||
if (event == null) { | |||
getLogger().log(Level.SEVERE, | |||
"Could not get event. This should never happen."); | |||
return; | |||
return null; | |||
} | |||
AtmosphereResource resource = event.getResource(); | |||
@@ -330,7 +347,7 @@ public class PushHandler { | |||
if (resource == null) { | |||
getLogger().log(Level.SEVERE, | |||
"Could not get resource. This should never happen."); | |||
return; | |||
return null; | |||
} | |||
VaadinServletRequest vaadinRequest = new VaadinServletRequest( | |||
@@ -342,7 +359,7 @@ public class PushHandler { | |||
} catch (ServiceException e) { | |||
getLogger().log(Level.SEVERE, | |||
"Could not get session. This should never happen", e); | |||
return; | |||
return null; | |||
} catch (SessionExpiredException e) { | |||
// This happens at least if the server is restarted without | |||
// preserving the session. After restart the client reconnects, gets | |||
@@ -351,7 +368,7 @@ public class PushHandler { | |||
getLogger().log(Level.FINER, | |||
"Session expired before push disconnect event was received", | |||
e); | |||
return; | |||
return session; | |||
} | |||
UI ui = null; | |||
@@ -375,13 +392,13 @@ public class PushHandler { | |||
getLogger().log(Level.FINE, | |||
"Could not get UI. This should never happen," | |||
+ " except when reloading in Firefox and Chrome -" | |||
+ " see http://dev.vaadin.com/ticket/14251."); | |||
return; | |||
+ " see https://github.com/vaadin/framework/issues/5449."); | |||
return session; | |||
} else { | |||
getLogger().log(Level.INFO, | |||
"No UI was found based on data in the request," | |||
+ " but a slower lookup based on the AtmosphereResource succeeded." | |||
+ " See http://dev.vaadin.com/ticket/14251 for more details."); | |||
+ " See https://github.com/vaadin/framework/issues/5449 for more details."); | |||
} | |||
} | |||
@@ -426,6 +443,7 @@ public class PushHandler { | |||
// can't call ErrorHandler, we (hopefully) don't have a lock | |||
} | |||
} | |||
return session; | |||
} | |||
private static UI findUiUsingResource(AtmosphereResource resource, |
@@ -0,0 +1,66 @@ | |||
/* | |||
* Copyright 2000-2020 Vaadin Ltd. | |||
* | |||
* 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.vaadin.server; | |||
import java.util.Collections; | |||
import java.util.List; | |||
import javax.servlet.ServletException; | |||
import com.vaadin.tests.util.MockDeploymentConfiguration; | |||
/** | |||
* | |||
* @author Vaadin Ltd | |||
*/ | |||
public class MockVaadinServletService extends VaadinServletService { | |||
public MockVaadinServletService() throws ServiceException { | |||
this(new MockDeploymentConfiguration()); | |||
} | |||
public MockVaadinServletService( | |||
DeploymentConfiguration deploymentConfiguration) throws ServiceException { | |||
this(new VaadinServlet(), deploymentConfiguration); | |||
} | |||
public MockVaadinServletService(VaadinServlet servlet, | |||
DeploymentConfiguration deploymentConfiguration) throws ServiceException { | |||
super(servlet, deploymentConfiguration); | |||
try { | |||
servlet.init(new MockServletConfig()); | |||
} catch (ServletException e) { | |||
throw new RuntimeException(e); | |||
} | |||
} | |||
@Override | |||
protected List<RequestHandler> createRequestHandlers() | |||
throws ServiceException { | |||
return Collections.emptyList(); | |||
} | |||
@Override | |||
public void init() { | |||
try { | |||
super.init(); | |||
} catch (ServiceException e) { | |||
throw new RuntimeException(e); | |||
} | |||
} | |||
} |
@@ -18,6 +18,10 @@ public class MockVaadinSession extends VaadinSession { | |||
super(service); | |||
} | |||
public MockVaadinSession() throws ServiceException { | |||
super(new MockVaadinServletService()); | |||
} | |||
@Override | |||
public void close() { | |||
super.close(); |
@@ -0,0 +1,79 @@ | |||
/* | |||
* Copyright 2000-2020 Vaadin Ltd. | |||
* | |||
* 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.vaadin.server.communication; | |||
import java.util.concurrent.Future; | |||
import org.atmosphere.cpr.AtmosphereRequest; | |||
import org.atmosphere.cpr.AtmosphereResource; | |||
import org.atmosphere.cpr.AtmosphereResourceEvent; | |||
import org.junit.Assert; | |||
import org.junit.Test; | |||
import org.mockito.Mockito; | |||
import com.vaadin.server.MockVaadinServletService; | |||
import com.vaadin.server.MockVaadinSession; | |||
import com.vaadin.server.ServiceException; | |||
import com.vaadin.server.SessionExpiredException; | |||
import com.vaadin.server.VaadinRequest; | |||
import com.vaadin.server.VaadinSession; | |||
import com.vaadin.ui.UI; | |||
public class PushHandlerTest { | |||
MockVaadinSession session = null; | |||
@Test | |||
public void connectionLost_currentInstancesAreCleared() | |||
throws SessionExpiredException, ServiceException { | |||
session = new MockVaadinSession() { | |||
@Override | |||
public Future<Void> access(Runnable runnable) { | |||
runnable.run(); | |||
return Mockito.mock(Future.class); | |||
} | |||
}; | |||
VaadinSession.setCurrent(session); | |||
Assert.assertNotNull(VaadinSession.getCurrent()); | |||
MockVaadinServletService service = null; | |||
service = new MockVaadinServletService() { | |||
@Override | |||
public com.vaadin.server.VaadinSession findVaadinSession( | |||
VaadinRequest request) throws SessionExpiredException { | |||
return session; | |||
} | |||
@Override | |||
public UI findUI(VaadinRequest request) { | |||
return null; | |||
} | |||
}; | |||
service.init(); | |||
PushHandler handler = new PushHandler(service); | |||
AtmosphereResource resource = Mockito.mock(AtmosphereResource.class); | |||
AtmosphereRequest request = Mockito.mock(AtmosphereRequest.class); | |||
Mockito.when(resource.getRequest()).thenReturn(request); | |||
AtmosphereResourceEvent event = Mockito | |||
.mock(AtmosphereResourceEvent.class); | |||
Mockito.when(event.getResource()).thenReturn(resource); | |||
handler.connectionLost(event); | |||
Assert.assertNull(VaadinSession.getCurrent()); | |||
} | |||
} |
@@ -61,6 +61,7 @@ public class ClassesSerializableTest { | |||
"com\\.vaadin\\.server\\.VaadinPortlet", // | |||
"com\\.vaadin\\.server\\.MockServletConfig", // | |||
"com\\.vaadin\\.server\\.MockServletContext", // | |||
"com\\.vaadin\\.server\\.MockVaadinServletService", // | |||
"com\\.vaadin\\.server\\.Constants", // | |||
"com\\.vaadin\\.server\\.VaadinServiceClassLoaderUtil", // | |||
"com\\.vaadin\\.server\\.VaadinServiceClassLoaderUtil\\$GetClassLoaderPrivilegedAction", // |
@@ -13,7 +13,7 @@ public class ComboBoxAtBottomEdgeWithinHorizontalLayout extends AbstractTestUI { | |||
@Override | |||
protected void setup(VaadinRequest request) { | |||
ComboBox<Integer> comboBox = new ComboBox<>(); | |||
comboBox.setItems(Arrays.asList(100, 200, 300, 400, 500)); | |||
comboBox.setItems(Arrays.asList(102, 205, 302, 402, 500)); | |||
HorizontalLayout horizontalLayout = new HorizontalLayout(); | |||
horizontalLayout.addComponent(comboBox); |
@@ -0,0 +1,75 @@ | |||
package com.vaadin.tests.components.treegrid; | |||
import com.vaadin.data.TreeData; | |||
import com.vaadin.server.VaadinRequest; | |||
import com.vaadin.tests.components.AbstractTestUI; | |||
import com.vaadin.ui.Button; | |||
import com.vaadin.ui.Component; | |||
import com.vaadin.ui.CssLayout; | |||
import com.vaadin.ui.Grid; | |||
import com.vaadin.ui.Label; | |||
import com.vaadin.ui.TreeGrid; | |||
public class TreeGridChangeHierarchyColumn extends AbstractTestUI { | |||
@Override | |||
protected String getTestDescription() { | |||
return "TreeGrid in MultiSelect mode should take hiden columns into account when" | |||
+ " rendering frozen columns after hierarchy-column reset."; | |||
} | |||
@Override | |||
protected Integer getTicketNumber() { | |||
return 12026; | |||
} | |||
@Override | |||
protected void setup(VaadinRequest request) { | |||
TreeGrid<String> treeGrid = new TreeGrid<>(); | |||
treeGrid.setId("TreeGrid"); | |||
for (int i = 0; i < 20; i++) { | |||
String columnId = String.valueOf(i); | |||
Grid.Column<String, Component> column = addColumn(treeGrid, | |||
columnId); | |||
column.setCaption(columnId); | |||
column.setId(columnId); | |||
} | |||
TreeData<String> data = treeGrid.getTreeData(); | |||
data.addItem(null, "child"); | |||
data.addItem("child", "grandChild"); | |||
treeGrid.setHierarchyColumn(treeGrid.getColumns().get(0)); | |||
Button hideHierCol = new Button("Hide Hierarchy Column"); | |||
hideHierCol.addClickListener(e -> { | |||
treeGrid.getHierarchyColumn().setHidden(true); | |||
}); | |||
hideHierCol.setId("hideHierColButton"); | |||
Button setHierCol = new Button("Set new Hierarchy Column"); | |||
setHierCol.addClickListener(e -> { | |||
treeGrid.getColumns().stream().filter(column -> !column.isHidden()) | |||
.findFirst().ifPresent(col -> { | |||
treeGrid.setHierarchyColumn(col.getId()); | |||
}); | |||
}); | |||
setHierCol.setId("setHierColButton"); | |||
treeGrid.setSelectionMode(Grid.SelectionMode.MULTI); | |||
treeGrid.setFrozenColumnCount(1); | |||
addComponents(treeGrid, hideHierCol, setHierCol); | |||
} | |||
private Grid.Column<String, Component> addColumn(Grid<String> grid, | |||
String columnId) { | |||
return grid.addComponentColumn(val -> { | |||
Label label = new Label(columnId); | |||
label.setWidth(50, Unit.PIXELS); | |||
return new CssLayout(label); | |||
}); | |||
} | |||
} |
@@ -3,6 +3,7 @@ package com.vaadin.tests.components.combobox; | |||
import org.junit.Test; | |||
import org.openqa.selenium.WebElement; | |||
import com.vaadin.testbench.By; | |||
import com.vaadin.testbench.elements.ComboBoxElement; | |||
import com.vaadin.tests.tb3.MultiBrowserTest; | |||
@@ -26,4 +27,38 @@ public class ComboBoxAtBottomEdgeWithinHorizontalLayoutTest | |||
cbBottom, popupBottom), cbBottom, popupBottom); | |||
} | |||
@Test | |||
public void ensurePopupPositionUpdatesWhenFiltered() { | |||
openTestURL(); | |||
ComboBoxElement cb = $(ComboBoxElement.class).first(); | |||
cb.openPopup(); | |||
WebElement popup = cb.getSuggestionPopup(); | |||
int initialTop = popup.getLocation().getY(); | |||
// filter a bit | |||
cb.findElement(By.vaadin("#textbox")).sendKeys("2"); | |||
int updatedTop = popup.getLocation().getY(); | |||
assertLessThan(String.format( | |||
"Popup should be repositioned when " | |||
+ "filtered. Initial: %s, Updated: %s", | |||
initialTop, updatedTop), initialTop, updatedTop); | |||
int cbBottom = cb.getLocation().getY() + cb.getSize().getHeight(); | |||
assertGreaterOrEqual(String.format( | |||
"Popup should still open above the ComboBox when " | |||
+ "filtered a bit. ComboBox: %s, Popup: %s", | |||
cbBottom, updatedTop), cbBottom, updatedTop); | |||
// filter more | |||
cb.clear(); | |||
cb.findElement(By.vaadin("#textbox")).sendKeys("1"); | |||
popup = cb.getSuggestionPopup(); | |||
updatedTop = popup.getLocation().getY(); | |||
assertLessThanOrEqual(String.format( | |||
"Popup should open below the ComboBox when " | |||
+ "filtered down to one result. ComboBox: %s, Popup: %s", | |||
cbBottom, updatedTop), cbBottom, updatedTop); | |||
} | |||
} |
@@ -0,0 +1,40 @@ | |||
package com.vaadin.tests.components.treegrid; | |||
import static org.junit.Assert.assertEquals; | |||
import java.util.List; | |||
import org.junit.Test; | |||
import org.openqa.selenium.WebElement; | |||
import com.vaadin.testbench.By; | |||
import com.vaadin.testbench.elements.ButtonElement; | |||
import com.vaadin.testbench.elements.TreeGridElement; | |||
import com.vaadin.tests.tb3.MultiBrowserTest; | |||
public class TreeGridChangeHierarchyColumnTest extends MultiBrowserTest { | |||
@Test | |||
public void renderingFrozenColumnsShouldFactorInHiddenColumns() { | |||
openTestURL(); | |||
waitForElementPresent(By.id("TreeGrid")); | |||
waitForElementPresent(By.id("hideHierColButton")); | |||
waitForElementPresent(By.id("setHierColButton")); | |||
TreeGridElement treeGrid = $(TreeGridElement.class).id("TreeGrid"); | |||
ButtonElement hideHierCol = $(ButtonElement.class) | |||
.id("hideHierColButton"); | |||
ButtonElement setHierCol = $(ButtonElement.class) | |||
.id("setHierColButton"); | |||
hideHierCol.click(); | |||
setHierCol.click(); | |||
// Wait for the new hierarchy column to be rendered | |||
waitForElementPresent(By.className("v-treegrid-expander")); | |||
List<WebElement> frozenCells = treeGrid | |||
.findElements(By.className("frozen")); | |||
assertEquals("Only the MultiSelect column should have frozen cells.", 2, | |||
frozenCells.size()); | |||
} | |||
} |