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

Grid: Scrolling not working as expected - Throws JS error #5182

Closed
AB-xdev opened this issue Jun 23, 2023 · 7 comments
Closed

Grid: Scrolling not working as expected - Throws JS error #5182

AB-xdev opened this issue Jun 23, 2023 · 7 comments
Assignees

Comments

@AB-xdev
Copy link

AB-xdev commented Jun 23, 2023

Description

When performing a click on a Grid, which select the contents of another TreeGrid the following error occurs:

There seems to be an error in Vaadin Grid:
undefined
Please submit an issue to https://github.com/vaadin/flow-components/issues/new/choose home:7:245
    error http://localhost:8080/home:7
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    anonymous http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js line 3 > Function:3
    K3 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    kg http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Xm http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    rm http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    gb http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    zr http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    eE http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    H http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Gu http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    Fg http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Jr http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    ob http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    N http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    onreadystatechange http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    od http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    A2 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    wy http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    (Async: EventHandlerNonNull)
    t1 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    e3 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Qd http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    c3 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Zd http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    B3 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    gf http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Vm http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    M http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    H http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    yf http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Jy http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    G http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Nl http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    od http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    A2 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    wy http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    E http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    (Async: setTimeout handler)
    Yc http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    j1 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    at http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    O1 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    ni http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    j2 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Ib http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Mg/a[y]< http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    od http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    A2 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    wy http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    doSelection http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    doSelection http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    __activeItemChanged http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    Zs http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    ni http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    _propertiesChanged http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    _propertiesChanged http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    _flushProperties http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:173
    _invalidateProperties http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    set http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:173
    _activateItem http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7460
    _onClick http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7460
    (Async: EventListener.handleEvent)
    ready http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7460
    ready http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7485
    ready http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7542
    ready http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3

we recently updated to Vaadin 24.1.1 so I highly suspect that this causes the problems.

Similar issue: #5177

UPDATE: The problem is likely related to TreeGrid#scrollToIndex, without that method everything works fine.

Expected outcome

No error

Minimal reproducible example

public class DemoView extends VerticalLayout
{
	private final Grid<Dummy> grid = new Grid<>();
	private final TreeGrid<Dummy> treeGrid = new TreeGrid<>();
	
	public DemoView()
	{
		final List<Dummy> items = List.of(
			new Dummy("1", List.of(new Dummy("1A"))),
			new Dummy("2"));
		
		this.grid.addColumn(Dummy::string);
		this.grid.setItems(items.stream().flatMap(Dummy::flatten).toList());
		
		this.treeGrid.addHierarchyColumn(Dummy::string);
		this.treeGrid.setTreeData(new TreeData<Dummy>().addItems(items, Dummy::nested));
		
		this.add(this.grid, this.treeGrid);
		
		this.grid.addSelectionListener(ev -> ev.getFirstSelectedItem()
			.ifPresent(itm -> {
				this.grid.select(itm);
				this.treeGrid.expand(items.get(0));
				this.treeGrid.select(itm);
				
				// Can also be replace by
				// Optional.ofNullable(this.treeGrid.getDataCommunicator().getIndex(itm)).ifPresent(this.treeGrid::scrollToIndex);
				Optional.ofNullable(this.treeGrid.getDataCommunicator().getIndex(itm))
					.ifPresent(idx -> this.getUI().ifPresent(ui ->
						ui.beforeClientResponse(this.treeGrid, ctx -> this.treeGrid.scrollToIndex(idx))
					));
			}));
	}
	
	record Dummy(String string, List<Dummy> nested)
	{
		public Dummy(final String string)
		{
			this(string, List.of());
		}
		
		public static Stream<Dummy> flatten(final Dummy dummy)
		{
			return Stream.concat(
				Stream.of(dummy),
				dummy.nested().stream().flatMap(Dummy::flatten)
			);
		}
	}
}

Note: ui.beforeClientResponse is used due to vaadin/vaadin-grid-flow#1048 (comment)

Steps to reproduce

  1. Deploy above code
  2. Click through the grid at the top
  3. After some clicks you will notice that the TreeGrid is losing items and starts throwing above failure
Vaadin5182.mp4

Environment

Vaadin version(s): 24.1.1
OS: -

Browsers

Issue is not browser related

@AB-xdev
Copy link
Author

AB-xdev commented Jun 23, 2023

I think the scrollToIndex function of the webcomponent might be broken:

If I make a bigger Treegrid with multiple expanded entries (they are expanded dynamically when the user clicks stuff - not instantly after the Treegrid was created), e.g:

  • 0
  • 1
    • 1A
    • 1B
  • 2
    • 2A
    • 2B
  • 3

and try to scroll to index = 3 - which should scrollto 1B - I end up at 3

@AB-xdev AB-xdev changed the title 'There seems to be an error in Vaadin Grid: undefined' Grid: Scrolling not working as expected - Throws JS error Jun 23, 2023
@sissbruecker
Copy link
Contributor

Apart from the error, note that TreeGrid.scrollToIndex does not scroll to the effective index, it scrolls to the index of the respective level that corresponds to the index of the method parameter. In your example, if you want to scroll to 1B, you would have to call treeGrid.scrollToIndex(1, 1) (indexes are zero-based I believe).

@AB-xdev
Copy link
Author

AB-xdev commented Jun 23, 2023

Apart from the error...

Yes I also found just out about this:

Looks like a lot has gone wrong when introducing this change:

  • The scrollToIndex(rowIndex) no longer does what it describes: Scrolling to the rowIndex - instead it trys to scroll to the index of the rootItems.
  • This change is not in the changelog (for 24.1.0 & 24.1.1)
  • How am I supposed to get the array of indizies? I can't find a newly introduced method for it.
  • I can get the index of the current element by using treeGrid.getDataCommunicator().getIndex(item) however I can't use it anymore now because the above mentioned method no longer works
  • There is still no method like scrollToItem(T item) inside Grid and TreeGrid - I think the change was meant for this method. Otherwise it makes 0 sense in my eyes because why would you introduce a method that can't be used effectively...

@sissbruecker
Copy link
Contributor

sissbruecker commented Jun 23, 2023

This has been introduced as a behavior altering change, as the previous scrollToIndex implementation was unreliable to useless, see the linked tickets, as well as #1016.

The whole root of the issue is that, due to lazy-loading of child nodes, the component does not have enough information to scroll to an effective index. Thus developers need to specify the path through the tree. I'm not familiar with the behavior of HierarchicalDataCommunicator.getIndex and whether it considers expanded nodes. From a quick look it does not necessary fetch child items from the data provider when nodes are expanded, so I would assume that it is not reliable either.

A scrollToItem method would certainly be useful, but it has the same issue - we don't have any information or data provider APIs to check where in the tree the item is, unless (in the worst case) we load the whole tree from the data provider.

It should definitely have been mentioned in the release notes, I'll see if we can improve that. We'll also check if we can improve the JavaDoc for the method.

@AB-xdev
Copy link
Author

AB-xdev commented Jun 23, 2023

Okay so I tried to make it work with the new Index-Array / new API... and it also doesn't work:

PoC

Code

Note: Values are hardcoded for simplification

@Route("demo")
public class DemoView extends VerticalLayout
{
	private final Grid<Dummy> grid = new Grid<>();
	private final TreeGrid<Dummy> treeGrid = new TreeGrid<>();
	
	public DemoView()
	{
		final Dummy dummy3b1 = new Dummy("3B1");
		final Dummy dummy3b = new Dummy("3B", List.of(
			dummy3b1,
			new Dummy("3B2")
		));
		final Dummy dummy3 = new Dummy("3", List.of(
			new Dummy("3A"),
			dummy3b,
			new Dummy("3C")
		));
		final List<Dummy> items = List.of(
			new Dummy("1", List.of(
				new Dummy("1A"),
				new Dummy("1B"))),
			new Dummy("2"),
			dummy3,
			new Dummy("4"));
		
		this.grid.addColumn(Dummy::string);
		this.grid.setItems(items.stream().flatMap(Dummy::flatten).toList());
		
		this.treeGrid.addHierarchyColumn(Dummy::string);
		this.treeGrid.setTreeData(new TreeData<Dummy>().addItems(items, Dummy::nested));
		
		this.add(this.grid, this.treeGrid);
		
		this.grid.addSelectionListener(ev -> ev.getFirstSelectedItem()
			.ifPresent(itm -> {
				this.grid.select(itm);
				
				this.treeGrid.expand(dummy3, dummy3b);
				this.treeGrid.select(itm);
				
				this.treeGrid.scrollToIndex(2, 1, 0);
			}));
	}
	
	record Dummy(String string, List<Dummy> nested)
	{
		public Dummy(final String string)
		{
			this(string, List.of());
		}
		
		public static Stream<Dummy> flatten(final Dummy dummy)
		{
			return Stream.concat(
				Stream.of(dummy),
				dummy.nested().stream().flatMap(Dummy::flatten)
			);
		}
	}
}

Steps to reproduce:

  • Click onto 3b1
Results in JS error
There seems to be an error in Vaadin Grid:
o is undefined
Please submit an issue to https://github.com/vaadin/flow-components/issues/new/choose demo:7:245
    error http://localhost:8080/demo:7
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    _loadPage http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7476
    doEnsureSubCacheForScaledIndex http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    ensureSubCacheForScaledIndex http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    _getItem http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7476
    _updateScrollerItem http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7542
    __updateElement http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:3860
    update http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:3860
    update http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:3860
    update http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:3860
    __updateVisibleRows http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7542
    requestContentUpdate http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7542
    __selectedItemsChanged http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7509
    mo http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    ni http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    _propertiesChanged http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    _propertiesChanged http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    _flushProperties http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:173
    _invalidateProperties http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:199
    set http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:173
    doSelection http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    set http://localhost:8080/VAADIN/build/generated-flow-imports-e3a47d93.js:7645
    tryCatchWrapper http://localhost:8080/VAADIN/build/FlowBootstrap-feff2646.js:1
    anonymous http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js line 3 > Function:3
    K3 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    kg http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Xm http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    rm http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    gb http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    zr http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    eE http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    H http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Gu http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    Fg http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    Jr http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    ob http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    N http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    onreadystatechange http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    od http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
    A2 http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:3
    wy http://localhost:8080/VAADIN/build/FlowClient-d5d5e377.js:1
Vaadin5182_2.mp4

@AB-xdev
Copy link
Author

AB-xdev commented Jun 23, 2023

A scrollToItem method would certainly be useful, but it has the same issue - we don't have any information or data provider APIs to check where in the tree the item is, unless (in the worst case) we load the whole tree from the data provider.

I kind of understand that but I think usually a user wants to scroll to a specific item and not a specific index.

To my use-case:
Let's say you have e.g. a TreeGrid with 500items, the user has selected a "default" item that should always be displayed inside the TreeGrid when he enters the website.

I'm not sure how this is currently possible in any way with the scrollTo-API, however the select-API can do these kinds of stuff but it doesn't scrolls your item into the view.
So maybe it would be possible to somehow combine these 2 APIs so if an item is selected it's automatically scrolled into the view?

@sissbruecker
Copy link
Contributor

Did check the error caused by scrollToIndex, and it seems to have the same root cause as #5187. Closing in favor of that, as I have added the results of my investigation there.

Apart from that I have improved the release notes for 24.1 to note about the change in the behavior of the scrollToIndex(int rowIndex) method, and opened a PR for improving its JavaDoc to clarify that it only considers items in the root level of the tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants