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

Recursive layout rendering depending on the size of the table / screen #4467

Open
antman3351 opened this issue Apr 17, 2024 · 13 comments · May be fixed by #4598
Open

Recursive layout rendering depending on the size of the table / screen #4467

antman3351 opened this issue Apr 17, 2024 · 13 comments · May be fixed by #4598
Labels
Possible Bug A possible bug that needs investigation

Comments

@antman3351
Copy link
Contributor

Hey Oli,
I have a problem with Tabulator recursively recalculating the table height when the table has a max-height ( allowing the table to shrink if the data is less than the height )

The really strange thing is it doesn't always happen, I can trigger it by zooming out ( my colleague noticed it when his tab crashed ) so I guess it's dependent on screen resolution / size.

image

RowManager.redraw() -> RowManager.adjustTableSize() -> this.redraw()

I modified the adjustTableSize to see what's happening and limit it to 100 redraws

if(!this.fixedHeight && initialHeight != this.element.clientHeight){
       if ( this.redrawing )
       {
	   this.redrawingCount++;
       }
       console.log( `initialHeight: ${initialHeight} this.element.clientHeight: ${this.element.clientHeight}. isRedrawing:${this.redrawing ?1 : 0 }`);

      resized = true;
      if(this.subscribed("table-resize")){
	      this.dispatch("table-resize");
      }else if(this.redrawingCount < 100){
	      this.redraw();
      }
}

Output when zoomed:

initialHeight: 20 this.element.clientHeight: 749. isRedrawing:0 [RowManager.js:1076:27](http://localhost:8082/totalgest/4.0.0/public/assets/global/plugins/tabulator/src/js/core/RowManager.js)
initialHeight: 725 this.element.clientHeight: 1045. isRedrawing:1 [RowManager.js:1076:27](http://localhost:8082/totalgest/4.0.0/public/assets/global/plugins/tabulator/src/js/core/RowManager.js)
initialHeight: 1045 this.element.clientHeight: 749. isRedrawing:1 [RowManager.js:1076:27](http://localhost:8082/totalgest/4.0.0/public/assets/global/plugins/tabulator/src/js/core/RowManager.js)
initialHeight: 725 this.element.clientHeight: 1045. isRedrawing:1 [RowManager.js:1076:27](http://localhost:8082/totalgest/4.0.0/public/assets/global/plugins/tabulator/src/js/core/RowManager.js)
initialHeight: 1045 this.element.clientHeight: 749. isRedrawing:1 [RowManager.js:1076:27]
...

As a fix I've just left the flag to not call this.redraw() when being called from redraw(), I haven't noticed any visual differences so far. I've waited to make a PR because I'd like your opinion. If the problem is caused else where maybe we can still leave a max recursion check and then throw an error ?

Also Is this line correct in VirtualDomVertical.js@101 ?

if(this.rows().length){
    this._virtualRenderFill((topRow === false ? this.rows.length - 1 : topRow), true, topOffset || 0);
}else{

in the if should this.rows.length be a function call this.rows().length as a quick console.log( console.log( this.rows.length, this.rows().length ); in the if gives 0 124

changing the inside call to this.rows() doesn't fix the infinite loop issue.

Tabulator Info

  • 6.2.0 ( also in 6.0.1 )

To Reproduce
I can't reproduce the exact error in JS Fiddle but the browser does hang ( chrome ) and ask if I want to interrupt the script ( no idea if it's the same problem or another issue )
https://jsfiddle.net/antman3351/c1asqtez/53/
make sure the console is minimised ( it doesn't do it with the console open )

Desktop (please complete the following information):

  • OS: Windows 10
  • chrome, safari

Additional context
Add any other context about the problem here.

@antman3351 antman3351 added the Possible Bug A possible bug that needs investigation label Apr 17, 2024
@olifolkerd
Copy link
Owner

olifolkerd commented Apr 17, 2024

Is your table element or it's parent flexed?

@antman3351
Copy link
Contributor Author

Hey,

No the table only has a maxHeight passed in the table options

I recreated a simple version of HTML/JS outside of jsFiddle and put it in a html file

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <link href="https://unpkg.com/[email protected]/dist/css/tabulator.min.css" rel="stylesheet">
</head>
<body>
    <div id="example-table"></div>
    <script>
        (function ()
        {
            this.init = async () =>
            {
                let TabulatorUrl = null;
                if ( confirm( 'Use patched version ? ' ) )
                {
                    console.log( 'Using patched version');
                    TabulatorUrl = 'http://localhost:8082/totalgest/4.0.0/public/assets/global/plugins/tabulator/6.2.0/js/tabulator_esm.custom.js';
                }
                else
                {
                    console.log( 'Using official build');
                    TabulatorUrl = 'https://unpkg.com/[email protected]/dist/js/tabulator_esm.js';
                }

                const { TabulatorFull } = await import( TabulatorUrl );


                //define some sample data
                const _tableData = [
                    { id: 1, name: "Oli Bob", age: "12", col: "red", dob: "" },
                    { id: 2, name: "Mary May", age: "1", col: "blue", dob: "14/05/1982" },
                    { id: 3, name: "Christine Lobowski", age: "42", col: "green", dob: "22/05/1982" },
                    { id: 4, name: "Brendon Philips", age: "125", col: "orange", dob: "01/08/1980" },
                    { id: 5, name: "Margret Marmajuke", age: "16", col: "yellow", dob: "31/01/1999" },
                ];

                let tableData = [];
                for ( let i = 0; i < 10; i++ )
                {
                    tableData = [ ...tableData, ..._tableData ];
                }

                const table = new TabulatorFull( "#example-table", {
                    maxHeight: '90vh',
                    data: tableData,
                    layout: "fitDataStretch",
                    autoResize: false,
                    columns: [ //Define Table Columns
                        { title: "Name", field: "name" },
                        { title: "Age", field: "age", hozAlign: "left" },
                        { title: "Favourite Color", field: "col" },
                        { title: "Date Of Birth", field: "dob", hozAlign: "center" },
                    ],
                } );

                table.on( 'tableBuilt', () =>
                {
                    console.log( 'table built' );
                    // table.blockRedraw(); // Makes no difference
                } );
            };

            this.init();

        }).apply( {} );

    </script>
</body>
</html>

Here's what happens ( the patched version doesn't allow calling the redraw function if adjustTableSize is already called from redraw )

tabulator.recursion.mp4

@antman3351
Copy link
Contributor Author

Update: I had to also skip re-dispatching the table-resize event too as I had a table with the total column widths > table width ( horizontal scrollbar ) that too kept recursing over the same section.

Change now looks like this:

if(!this.fixedHeight && initialHeight != this.element.clientHeight){
  resized = true;
  if(!this.redrawing) // Recursion check
  {
	  if ( this.subscribed( "table-resize" ) )
	  {
		  this.dispatch( "table-resize" );
	  }
	  else
	  {
		  this.redraw();
	  }
  }
}

@timhemming-fico
Copy link

We're also hitting this same issue, the table is recursively redrawing. I've applied the same fix to a patched version of Tabulator and it resolves the issue:

image

@GolfSierra
Copy link

GolfSierra commented Jul 11, 2024

Same issue here, but the fixes do not work for me

@ergsolo
Copy link

ergsolo commented Jul 29, 2024

I got stucked tabulator on small amount of data and then an Error

In my case looks like dispatch('resize-table') is called recursively

Tabulator 6.2.0

Screenshot 2024-07-29 at 20 57 19 Screenshot 2024-07-29 at 20 57 30 Screenshot 2024-07-29 at 20 59 26

@ergsolo
Copy link

ergsolo commented Jul 30, 2024

I've downgraded tabulator to 5.5.4 and it works without issues.
Issue starts producible since 5.6.1.

@BartNSTCL
Copy link

I've had this hit at least 1 user, maybe 2 out of over a hundred. It was odd, because the pages worked in Edge but not Chrome. I went and checked and the user's Chrome was set to 80% zoom. Based on the comments above, maybe if he was running at 100%, the error wouldn't occur. For now, I've rolled back to 5.5.4 as suggested earlier.

@BartNSTCL
Copy link

Has this been addressed by version 6.2.5?

@schwarmco
Copy link

schwarmco commented Oct 12, 2024

i am hitting this same exact issue in 6.3.0 but can confirm, the proposed fix of @antman3351 works:

//check if the table has changed size when dealing with variable height tables
if(!this.fixedHeight && initialHeight != this.element.clientHeight){
    resized = true;
    if(!this.redrawing) { // prevent recursive redraws		
	this.redrawing = true; // flag to set we are in a redrawing
	if(this.subscribed("table-resize")){
		this.dispatch("table-resize");
	}else {
		this.redraw();
	}
	this.redrawing = false; // unset redrawing flag as we're done
    }
}

here:
https://github.com/olifolkerd/tabulator/blob/master/src/js/core/RowManager.js#L1063-L1071

@olifolkerd would you accept a PR for this or you have any other input for that issue?

@olifolkerd
Copy link
Owner

@schwarmco yes a pr would be great 👍

@schwarmco schwarmco linked a pull request Oct 12, 2024 that will close this issue
@schwarmco
Copy link

@olifolkerd thanks for the quick reply! PR: #4598

@m-blank-eneatec
Copy link

Can also confirm that the patch from @antman3351 has solved the recursion error for me. And just like @BartNSTCL, the problem seems to occur when the browser has a zoom setting other than 100% (for me it already happens at 90%).

Perhaps this points to a more fundamental issue in the calculations performed within the redraw functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Bug A possible bug that needs investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants