Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8218745: TableView: visual glitch at borders on horizontal scrolling #630

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -357,7 +357,7 @@ private final void setRootHeader(NestedTableColumnHeader value) {

/** {@inheritDoc} */
@Override protected void layoutChildren() {
double x = scrollX;
double x = snapPositionX(scrollX);
double headerWidth = snapSizeX(getRootHeader().prefWidth(-1));
double prefHeight = getHeight() - snappedTopInset() - snappedBottomInset();
double cornerWidth = snapSizeX(flow.getVbar().prefWidth(-1));
@@ -3077,8 +3077,9 @@ public void setNode(Node n) {
}

public void setClipX(double clipX) {
setLayoutX(-clipX);
clipRect.setLayoutX(clipX);
double snappedClipX = snapPositionX(clipX);
setLayoutX(-snappedClipX);
clipRect.setLayoutX(snappedClipX);
Copy link
Member

@kevinrushforth kevinrushforth Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely not the right place to snap the coordinates to a pixel boundary. This is usually done as part of layout. I'm also skeptical of the fact that you added it to setClipX but not setClipY.

Copy link
Member Author

@Maran23 Maran23 Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't set clipY because it's not part of the story and clipY is always set to 0 when the VirtualFlow is vertical (which is the default for all VirtualFlowContainer. Only ListView can be non-vertical - This should be another bug and test case). That's why there is no issue with it normally.

I'm not sure if this is the wrong place. E.g. ScrollPaneSkin also listens on the scrollbar valueProperty and sets the snapped scrollbar value with setLayoutX - which is very similar then here. We set the layoutX here as well (for clip and the container).

Finally: While I understand it does not fix every scale, isn't it at least a good first step? (When we agree this is the correct location here)

}

public void setClipY(double clipY) {
@@ -30,6 +30,7 @@
import javafx.scene.control.IndexedCell;
import javafx.scene.control.ScrollBar;
import javafx.scene.layout.StackPane;
import javafx.scene.layout.Region;

public class VirtualFlowShim<T extends IndexedCell> extends VirtualFlow<T> {

@@ -73,7 +74,7 @@ public ScrollBar shim_getVbar() {
return super.getVbar();
}

public ClippedContainer get_clipView() {
public Region get_clipView() {
return super.clipView;
}

@@ -38,6 +38,10 @@ public class Person {
private final SimpleStringProperty email;
private final ReadOnlyIntegerWrapper age;

public Person(String fName) {
this(fName, 0);
}

public Person(String fName, int age) {
this(fName, null, null, age);
}
@@ -46,6 +46,8 @@
import javafx.collections.SetChangeListener;
import javafx.css.PseudoClass;
import javafx.scene.Node;
import javafx.scene.control.skin.NestedTableColumnHeader;
import javafx.scene.control.skin.VirtualFlowShim;
import test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils;
import test.com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
import test.com.sun.javafx.scene.control.infrastructure.KeyModifier;
@@ -5584,4 +5586,28 @@ public void test_addingNewItemsDoesNotChangePseudoClassSelectedState() {

sl.dispose();
}

@Test public void testScrollingXIsSnapped() {
TableColumn<Person, String> firstNameCol = new TableColumn<>("First Name");
firstNameCol.setCellValueFactory(new PropertyValueFactory<>("firstName"));

TableView<Person> table = new TableView<>();
table.setItems(FXCollections.observableArrayList(new Person("VeryLongStringVeryLongString")));
table.getColumns().add(firstNameCol);

StageLoader loader = new StageLoader(table);

Toolkit.getToolkit().firePulse();

NestedTableColumnHeader rootHeader = VirtualFlowTestUtils.getTableHeaderRow(table).getRootHeader();
VirtualScrollBar scrollBar = VirtualFlowTestUtils.getVirtualFlowHorizontalScrollbar(table);

double newValue = 25.125476811;
double snappedNewValue = table.snapPositionX(newValue);
scrollBar.setValue(newValue);

assertEquals(-snappedNewValue, rootHeader.getLayoutX(),0);

loader.dispose();
}
}
@@ -40,8 +40,11 @@
import javafx.event.Event;
import javafx.scene.control.IndexedCell;
import javafx.scene.Node;
import javafx.scene.control.skin.VirtualFlow;
import javafx.scene.shape.Circle;

import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
import test.javafx.scene.control.SkinStub;
import javafx.scene.input.ScrollEvent;

@@ -1317,6 +1320,26 @@ protected double computeMaxHeight(double width) {
if (cell != null) assertFalse(cell.isVisible());
}
}

@Test public void testScrollingXIsSnapped() {
StageLoader loader = new StageLoader(flow);
flow.resize(50, flow.getHeight());

pulse();

double newValue = 25.125476811;
double snappedNewValue = flow.snapPositionX(newValue);
flow.shim_getHbar().setValue(newValue);

double layoutX = flow.get_clipView().getLayoutX();
double clipLayoutX = flow.get_clipView().getClip().getLayoutX();

assertEquals(snappedNewValue, clipLayoutX, 0.0);
assertEquals(-snappedNewValue, layoutX, 0.0);

loader.dispose();
}

}

class GraphicalCellStub extends IndexedCellShim<Node> {