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

DataTable: Resizing headers in expand mode does not work correctly #3970

Closed
pkmarcotte opened this issue Jan 18, 2023 · 9 comments · Fixed by #3971
Closed

DataTable: Resizing headers in expand mode does not work correctly #3970

pkmarcotte opened this issue Jan 18, 2023 · 9 comments · Fixed by #3971
Assignees
Labels
Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@pkmarcotte
Copy link
Contributor

Describe the bug

See the example of this problem in action directly in the demo on primefaces.org:
https://www.primefaces.org/primereact/datatable/colresize/

Tried this on a Mac OS 12.6.2 using:

  • Firefox Developer Edition 109.0b9
  • Safari 16.2

I investigated this issue further and found the root case.

In DataTable.js, there is a function onColumnResizeEnd. This function is called when the user drags and drops the resizer to the right. Within this function, when column size mode is 'expand', it attempts to do two things:

  • it changes the width of the entire table by the delta amount (updateTableWidth)
  • it goes through each of the columns of the table resizes only the column in question by the delta amount (resizeTableCells)

Here is the code snippet for this portion:

            } else if (props.columnResizeMode === 'expand') {
                const tableWidth = tableRef.current.offsetWidth + delta + 'px';

                const updateTableWidth = (el) => {
                    !!el && (el.style.width = el.style.minWidth = tableWidth);
                };

                updateTableWidth(tableRef.current);

                if (!isVirtualScrollerDisabled()) {
                    updateTableWidth(bodyRef.current);
                    updateTableWidth(frozenBodyRef.current);

                    if (wrapperRef.current) {
                        updateTableWidth(DomHandler.findSingle(wrapperRef.current, '.p-virtualscroller-content'));
                    }
                }

                resizeTableCells(newColumnWidth);
            }

The problem:

  • updateTableWidth actually results in a side effect: the computed width of table cells within the table are automatically scaled by the browser, with each cell getting a relative portion of the new table, taking into account margin and padding. So, for example, if you had table cells of 100px 100px 100px, but a table width of 350px, the actual computed cell width in the browser would be something like 114px 114px 114px. You can see this if you put the following vanilla HTML into a browser and then 'inspect' the cells -- they will all be 114px.
<table style="width: 350px">
  <thead>
    <tr>
      <td style="width: 100px">Beans</td>
      <td style="width: 100px">Beans</td>
      <td style="width: 100px">Beans</td>
    </tr>
  </thead>
</table>
  • with this in mind, look at the code in resizeTableCells:
    const resizeTableCells = (newColumnWidth, nextColumnWidth) => {
        let widths = [];
        let colIndex = DomHandler.index(resizeColumnElement.current);
        let headers = DomHandler.find(tableRef.current, '.p-datatable-thead > tr > th');

        headers.forEach((header) => widths.push(DomHandler.getOuterWidth(header)));

        destroyStyleElement();
        createStyleElement();

        let innerHTML = '';

        widths.forEach((width, index) => {
            let colWidth = index === colIndex ? newColumnWidth : nextColumnWidth && index === colIndex + 1 ? nextColumnWidth : width;
            let style = props.scrollable ? `flex: 1 1 ${colWidth}px !important` : `width: ${colWidth}px !important`;

            innerHTML += `
                .p-datatable[${attributeSelectorState}] .p-datatable-thead > tr > th:nth-child(${index + 1}),
                .p-datatable[${attributeSelectorState}] .p-datatable-tbody > tr > td:nth-child(${index + 1}),
                .p-datatable[${attributeSelectorState}] .p-datatable-tfoot > tr > td:nth-child(${index + 1}) {
                    ${style}
                }
            `;
        });

        styleElement.current.innerHTML = innerHTML;
    };

Note that it uses 'getOuterWidth' -- which corresponds to offsetWidth.

So if you made a DataTable that had a width of 300px, and you had headers with 100px 100px 100px, and then you resized #2 by 50px, you would have something like this:

  • updateTableWidth(350)
  • resizeTableCells (newColumnWidth=150)
  • calculated existing widths = 114 114 114
  • output widths: 114 150 114
  • BUT, this no longer adds up to 350, so the browser will adjust the offsetWidth so that it will be: 103 134 103 (!!!)
  • Each time you do this, the other columns will gradually expand.
  • The core reason for this is that the new column width is calculated BEFORE the table offset, and as a result, the percentage of the total that the cell gets changes, which causes the other cells to change.

Solution #1: reorder calls:

  • I tested this solution locally by modifying the 8.7.0 source. I simply moved the call to 'resizeTableWidths' so that it happens just before the call to updateTableWidth:
            } else if (props.columnResizeMode === 'expand') {
                const tableWidth = tableRef.current.offsetWidth + delta + 'px';

                const updateTableWidth = (el) => {
                    !!el && (el.style.width = el.style.minWidth = tableWidth);
                };

                resizeTableCells(newColumnWidth);
                updateTableWidth(tableRef.current);

                if (!isVirtualScrollerDisabled()) {
                    updateTableWidth(bodyRef.current);
                    updateTableWidth(frozenBodyRef.current);

                    if (wrapperRef.current) {
                        updateTableWidth(DomHandler.findSingle(wrapperRef.current, '.p-virtualscroller-content'));
                    }
                }

            }

This works and looks good, but I don't know if it could have other side effects for people with different parameters on their code.

Solution #2: calculate new column width AFTER the updateTableWidth call

I haven't tested this solution, but in theory it should have a similar effect.

Reproducer

No response

PrimeReact version

8.7.0

React version

17.x

Language

TypeScript

Build / Runtime

Next.js

Browser(s)

Safari 16.2,Chrome

Steps to reproduce the behavior

  1. go to https://www.primefaces.org/primereact/datatable/colresize/
  2. go to the section labelled 'Expand Mode'
  3. Use your mouse to drag the right side of the 'Name' column an inch to the right

Actual: The Code column and also the other columns after Name also changes size slightly. Not by as much as you resize the Name column, but it always changes, and it changes more if you resize Name more. This also happens for any other column aside from the first one. The end result is the column doesn't end up where you dragged it.

This only seems to be a problem with Expand Mode -- Fit mode works correctly.

Expected behavior

Name column resized to the exact spot where you dragged it, all other columns to the right shifted by that amount but not resized, all other columns to the left untouched.

@pkmarcotte pkmarcotte added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Jan 18, 2023
@melloware
Copy link
Member

Awesome reproducer can you submit a PR?

Here are some similar issues that I think might also be fixed / related...

#3795

#3919

#2723

I feel like they are all symptoms of what you are describing.

@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Jan 18, 2023
@pkmarcotte
Copy link
Contributor Author

yeah I'll take a look at submitting a PR for PrimeReact for the one I tested.

pkmarcotte added a commit to pkmarcotte/primereact that referenced this issue Jan 18, 2023
@melloware melloware added this to the 8.7.4 milestone Jan 18, 2023
@mertsincan mertsincan modified the milestones: 8.7.4, 10.0.0 Jan 26, 2023
@shibuMadhavam
Copy link

Hope the fix of this issue is not available in version 9

@pkmarcotte
Copy link
Contributor Author

Hope the fix of this issue is not available in version 9

it is not. it looks like it is slated for the 10.0.0 milestone.

melloware pushed a commit to pkmarcotte/primereact that referenced this issue May 3, 2023
@melloware melloware modified the milestones: 10.0.0, 9.4.0 May 3, 2023
melloware pushed a commit that referenced this issue May 3, 2023
* - fixed 3970: DataTable in expand mode resizing problem.
meant to address #3970

* - run "npm run format"

* - change comment per request
@1Map
Copy link

1Map commented Aug 16, 2023

@melloware
Copy link
Member

@1Map why not submit a PrimeVue PR porting this fix?

@1Map
Copy link

1Map commented Aug 16, 2023

@1Map why not submit a PrimeVue PR porting this fix?

I did (primefaces/primevue#4276), not sure how long until someone merge it back into their master branch?

@1Map
Copy link

1Map commented Aug 16, 2023

@melloware Was this bug really fixed in PrimeReact? Reason why I ask is: If I navigate to:

https://primereact.org/datatable/#resize_expandmode

It still behaves wrong.

@melloware
Copy link
Member

melloware commented Aug 16, 2023

@1Map that showcase has not been updated for 10.0.0 yet it is still the pre-patched version.

There was a fix in 9.6.0 that broke the showcase unrelated to this change.

see: #4471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants