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

TreeGrid : Still wrong initial calculation of hierarchy column width (reopener of #5595 ) #5813

Closed
SonReimer opened this issue Dec 4, 2023 · 6 comments · Fixed by vaadin/web-components#7165
Assignees

Comments

@SonReimer
Copy link

SonReimer commented Dec 4, 2023

Description

In some situations the bug is still present.
Using the ShowCase in #5595 the bug can still be reproduced if it is extended by a navigating to another page by a button and navigating back by another button or putting two TreeGrids in the same view and pressing refresh.

Expected outcome

I expect the initial column soize to be calculated correctly.

Minimal reproducible example

example3.zip

Steps to reproduce

See description

Environment

Vaadin version(s): 23.3.33
OS: Windows

Browsers

No response

@yuriy-fix yuriy-fix added bug Something isn't working a11y Accessibility issue vaadin-grid Severity: Minor Impact: Low and removed a11y Accessibility issue labels Dec 7, 2023
@yuriy-fix yuriy-fix added the BFP label Feb 21, 2024
@yuriy-fix
Copy link
Contributor

This issue has been added to the priority backlog for further investigation.

@vursen vursen self-assigned this Feb 28, 2024
@vursen
Copy link
Contributor

vursen commented Feb 28, 2024

Hey @SonReimer! I'm investigating your issue and will update you on my progress.

@vursen
Copy link
Contributor

vursen commented Feb 29, 2024

I can confirm the issue and it's only reproducible in 23.3

@vursen
Copy link
Contributor

vursen commented Feb 29, 2024

I rolled back this change locally and started investigating the issue from scratch.

Version 23.3

Here are my insights from debugging:

  1. The connector sets the grid's size to 0, overriding any initial value set by Flow (issue?):

    grid.size = 0; // To avoid NaN here and there before we get proper data

  2. Due to the size being 0, the connector resolves the web component's initial request with an empty array:

  3. The empty response triggers recalculateColumnWidths(), which is skipped due to the absence of rows, but remains scheduled.

  4. Later, $connector.updateSize() sets the correct size, triggering a new root request to the connector.

  5. This time, the connector returns preloaded data:

    if (cache[root]?.[page]) {
    callback(cache[root][page]);
    } else {

  6. During the response processing, this expression in the web component returns false due to the rows not having _item property yet, which prevents child requests at this point (issue?).

  7. Then, the data response also triggers recalculateColumnWidths(), and this time it proceeds because the grid isn't loading anything anymore.

  8. During the recalculation, the virtualizer gets flushed, which triggers child requests.

  9. However, the connector delays these requests with a debouncer, and the recalculation finishes on a partially loaded grid.

Version 24.4

The web component also doesn't make any child requests at step (6) because some rows don't have an index property yet (issue?). The child requests also only happen on virtualizer flush during the recalculation at step (8). However, since version 24.1, the connector resolves such child requests synchronously when the data is preloaded by the server. This ensures their resolution before _recalculateColumnWidths() actually begins calculating the column widths and helps avoid the issue. So, seems this virtualizer fix has no impact (or not anymore) on the issue.

@vursen
Copy link
Contributor

vursen commented Mar 1, 2024

Using this example, the issue can be reproduced with the pure web component in all versions, including latest 24.4:

<script type="module">
  import '@vaadin/grid/all-imports';

  const grid = document.querySelector('vaadin-grid');

  // Set the grid size to 0 to mimic step (1)
  grid.size = 0;
  grid.dataProvider = ({ parentItem, page, pageSize }, cb) => {
    const levelSize = parentItem ? 3 : grid.size;

    const pageItems = [...Array(Math.min(levelSize, pageSize))].map((_, i) => {
      const indexInLevel = page * pageSize + i;

      return {
        name: `${parentItem ? parentItem.name + '-' : ''}${indexInLevel}`,
        children: true,
      };
    });

    if (parentItem) {
      // Return child items asynchronously to mimic step (9)
      setTimeout(() => cb(pageItems, levelSize), 0);
    } else {
      cb(pageItems, levelSize);
    }
  };

  grid.expandedItems = [
    { name: '1' },
    { name: '1-0' },
    { name: '1-0-0' },
    { name: '1-0-0-0' },
    { name: '1-0-0-0-0' },
    { name: '1-0-0-0-0-0' },
  ];
 
  // Update the grid size to mimic step (4)
  setTimeout(() => {
    grid.size = 100;
  }, 1000)
</script>

<vaadin-grid item-id-path="name" width="400px" overflow="end right">
  <vaadin-grid-tree-column path="name" auto-width flex-grow="0"></vaadin-grid-tree-column>
  <vaadin-grid-column path="name" auto-width flex-grow="1"></vaadin-grid-column>
</vaadin-grid>

@vursen
Copy link
Contributor

vursen commented Mar 5, 2024

The fix is merged and will be available in the following upcoming versions:

  • 24.4.0.beta1
  • 24.3.7
  • 24.2.12
  • 23.4.0 (recommended to upgrade, as the issue was reported in V23.3. V23.3 has EOL)

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

Successfully merging a pull request may close this issue.

3 participants