olifolkerd / tabulator

Interactive Tables and Data Grids for JavaScript
http://tabulator.info
MIT License
6.74k stars 817 forks source link

Infinite loop when browser zoomed to 80% #2974

Closed h-tendy closed 4 years ago

h-tendy commented 4 years ago

Hi Oli - Here's a minor issue I'm running into. Sometimes, to increase the density of the table, I set the zoom size of the Chrome browser to 80%. When I do this (and depending on the exact contents of the table), tabulator runs into an infinite loop while rendering the table. I added a Debug statement and it turns out to be some sort of a rounding error. The infinite loop is between adjustTableSize() and redraw() of the RowManager.

I'm using 4.7.2. And my table is NOT fixed-height.

tabulator.js:3811 Debug: initialHeight, clientHeight: 19 18
tabulator.js:3811 Debug: initialHeight, clientHeight: 19 18
tabulator.js:3811 Debug: initialHeight, clientHeight: 19 18
tabulator.js:3811 Debug: initialHeight, clientHeight: 19 18
ReactTabulator.js:53 Uncaught (in promise) RangeError: Maximum call stack size exceeded
    at RowManager.adjustTableSize (tabulator.js:3793)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)
    at RowManager.redraw (tabulator.js:3856)
    at RowManager.adjustTableSize (tabulator.js:3817)

I applied this fix, and it seems to do the trick. In adjustTableSize, I replaced the != check by Math.abs(initialHeight - this.element.clientHeight) > 1 condition. Of course, you may find a better way to fix this! Thank you so much.

        if(!this.fixedHeight && Math.abs(initialHeight - this.element.clientHeight) > 1){
            modExists = this.table.modExists("resizeTable");

            if((modExists && !this.table.modules.resizeTable.autoResize) || !modExists){
                this.redraw();
            }
        }
olifolkerd commented 4 years ago

Hey @h-tendy

Thanks for getting in touch,

If you are filing a bug report, please use the Bug Report Issue Template it will prompt you to provide all the information we need to help you.

It is really important that you provide a link to a JS Fiddle or Code Pen that demonstrates your issue. most problems are caused either by user error or on planned interactions between different modules of Tabulator or between different CSS themes. Without seeing how your table is setup, it is impossible for us to fully understand the issue. They also provide an important way for us to check that a potential fix actually addresses your issue. It also shows us your table constructor which is vital in reproducing the issue.

Having this live code is particularly important for your rendering issue as it is likely the result of the table not properly initialising a variable or an issue with one of the built in CSS themes and it is really important that we can trace the issue from start to finish to ensure it is fixed.

Thank you for your suggested fix, If you could provide a fiddle that demonstrates your setup, i would be happy to test and apply it.

Cheers

Oli :)

h-tendy commented 4 years ago

Understood, I will get the jfiddle as soon as I can. I did choose to investigate the possible problem a bit more deeply inside the code, to be sure. Maybe I should try with a vanilla installation in jfiddle instead. Problem is I'm using quite a few features (ajax, various handlers, markdown inside the cells, highlighting of cells on hover, a few custom editors, via react-tabulator wrapped in another class to avoid another issue) and hence I need to invest some time to get a version for jfiddle with all the same settings! Thanks much for your prompt response as usual.

olifolkerd commented 4 years ago

Hey @h-tendy

Thanks for that, It just wants to be a minimal test case, so it dosn't need to replicate everything about your setup. In fact it would be preferable if it didn't, It just wants to show enough to trigger the issue you are experiencing.

Have a look at the Stack Overflow Guide to Producing a Minimal Reproducible Example for more info

Cheers

Oli

olifolkerd commented 4 years ago

Hey @h-tendy

I have pushed a fix to the master branch to remove any potential sub-pixel zoom based rounding errors from triggering a redraw, but as i was unable to replicate the issue myself i cannot be sure it resolves things adequately.

It will be included in todays patch release.

Cheers

Oli :)

h-tendy commented 4 years ago

Thank you so much. As soon as I upgrade, will let you know in the use-case I have. Cheers.

jaredhughes-rise commented 4 years ago

Hello - I was see the same error in the console reported by @h-tendy .

I updated to the latest (4.8.1) and now see a slightly different error: image

And this is originating here: image

olifolkerd commented 4 years ago

Hey @jaredhughes-rise

Do you have a JS Fiddle or Code Pen that demonstrates the issue,

It is pretty much impossible for me to resolve this until someone can create a minimal test case that shows in in action as i am unable to generate it on my own machine

Cheers

Oli :)

treasuryspring commented 4 years ago

If it can help anyone, after setting virtualDom: false, the issue disappeared.

brimimc commented 3 years ago

@olifolkerd So I think I can reproduce this using a series of zoom in/out and maximize window (with a paginated table). I'll work on getting exact steps to reproduce, but what I'm seeing is the this.table.footerManager.getElement().offsetHeight fluctuates by one pixel

VM25449:1 otherHeight  78  this.table.footerManager.getElement().offsetHeight  35
VM25450:1 this.table.element.clientHeight  95
VM25451:1  this.table.footerManager.getElement().offsetHeight  36
VM25452:1 redraw - autoResize false
VM25453:1 otherHeight  79  this.table.footerManager.getElement().offsetHeight  36
VM25454:1 this.table.element.clientHeight  95
VM25455:1  this.table.footerManager.getElement().offsetHeight  35
VM25456:1 redraw - autoResize false
VM25457:1 otherHeight  78  this.table.footerManager.getElement().offsetHeight  35
VM25458:1 this.table.element.clientHeight  95
VM25459:1  this.table.footerManager.getElement().offsetHeight  36
VM25460:1 redraw - autoResize false
VM25461:1 otherHeight  79  this.table.footerManager.getElement().offsetHeight  36
VM25462:1 this.table.element.clientHeight  95
VM25463:1  this.table.footerManager.getElement().offsetHeight  35
VM25464:1 redraw - autoResize false

inside of RowManager.prototype.adjustTableSize after the style height is reset

this.element.style.height = "";

Also of note, normally this.table.modules.resizeTable.autoResize is true so this infinite loop does not happen. I think somehow using the window maximize/fullscreen mode (I'm on OS X) somehow causes it to be set to false

olifolkerd commented 3 years ago

This has hopefully already been resolved on the 4.9 branch, which should be released on sunday