NeXTs / Clusterize.js

Tiny vanilla JS plugin to display large data sets easily
https://clusterize.js.org
MIT License
7.22k stars 413 forks source link

Unable to scroll to the bottom of a table when there is only 1 cluster #70

Open rjohnson06 opened 8 years ago

rjohnson06 commented 8 years ago

I'm seeing an issue where I can't scroll to the bottom of the table. The scrollbar position jerks back up before it reaches the bottom : http://output.jsbin.com/midasuvuce/4 . Increasing the rows_in_block to 50 seems to fix the issue.

This is the configuration I'm using var clusterize = new Clusterize({ contentId: 'content-area', rows_in_block: 26, tag: 'tr', show_no_data_row: false, scrollId: 'venue-ticket-list' });

NeXTs commented 8 years ago

Hi, it's not that easy to investigate an issue in provided jsbin as soon as there are whole html page

Could you please check if this problem reproduces at clusterize v0.16.0.

NeXTs commented 8 years ago

Take a look, single cluster (amount of rows fits one cluster), no repaint on scrolling http://codepen.io/NeXTs/pen/BzAjmB Does scrollbar position jerks back up for you here? If yes - send me info regarding browser, it's version and OS

rjohnson06 commented 8 years ago

It works fine for me in the example you sent.

I changed the clusterize version to v0.16.0 here : http://output.jsbin.com/midasuvuce/4 and removed the rest of the html, I still see the issue.

Using the latest google chrome 51.0.2704.106 m

NeXTs commented 8 years ago

First of all add required css

<link href="https://cdn.jsdelivr.net/clusterize.js/0.16.1/clusterize.css" rel="stylesheet" type="text/css">

and give me a moment, investigating..

NeXTs commented 8 years ago

I guess .footer should be moved out of #list-tickets and placed as child of #list-child-ctn, after #venue-ticket-list, or even after #list-child-ctn.

Set max-height: 479px for .clusterize-scroll

NeXTs commented 8 years ago

Does it works for you? http://jsbin.com/cunacerahi/1/edit?html,output

rjohnson06 commented 8 years ago

That does work for me.

Strange, it seems removing the fixed width from .list-ctn fixes the issue

.list-ctn { width: 100%; }

fixes the issue

http://jsbin.com/rilofepane/1/edit?html,output

rjohnson06 commented 8 years ago

Oh, it looks like width 100% makes the height of the rows more consistent.

rjohnson06 commented 8 years ago

I investigated a little more, it seems getClusterNum returns 0 while scrolling, but near the bottom of the list it sometimes returns 1. There's only 1 cluster though, so it just re-adds the original cluster/rows. http://output.jsbin.com/daqugusebi

I think it has something to do with the fact that opts.item_height is set to 59, but the average height of the rows is ~65. If I set opts.item_height the average height that fixes the issue.

I think I will just set rows_in_block to 50, I haven't yet reproduced the issue with that setting.

NeXTs commented 8 years ago

this jsbin doesn't have necessary styles again

rjohnson06 commented 8 years ago

oh sorry, I forgot to mention the styles are bundled into this stylesheet : https://mapwidget2.seatics.com/Css/defaultBreakpoint?v=GPUrOnSRFpShM4qyVwazDo3uMo6WhbKYHEk4IYQ6XTI1

rjohnson06 commented 7 years ago

Hi, not sure if I should post this here since it was already closed. Had this issue crop up again. I was able to create a much simpler example this time to demonstrate the issue. :
http://jsbin.com/tadujeleza/edit?html,output

It is definitely an issue with inconsistent heights of my rows. The first row has a height a 50, but the average height of the rows in the list is ~59. If you resize the window in the demo, item_height gets reset to a height of 58 from a row in the middle of the list and the issue no longer occurs.

Couple of things I noticed from debugging. When starting from the beginning of the list, scrollEv calls insertToDOM when scroll_top passes 7500 and getClusterNum returns 1. After insertToDOM, there are 4 rows in the list, with the height placeholder above them. scrollEv gets called again immediately, scroll_top is now < 7500, getClusterNum returns 0 and insertToDOM is called again.

I tried adding rows until the issue no longer happens. It seems the issue only happens if the heights of the rows left in the list (after insertToDOM for cluster # 1) is less than the height of the scroll area.

Possible fixes : Check if we're on the "last" cluster Is it necessary to add that extra cluster? If we have 154 rows in the list and rows per cluster is 200, what is the point of calling insertToDOM for 4 rows that are already in the list?

Error handling After calling insertToDOM, if item_height * num_rows_in_current_cluster < scrollArea height, pull more rows into the DOM.

Row Height Sampling In the background, measure heights of the rows in the list. Slowly build a more accurate average height of the rows in the list without disrupting UI performance.

Could you give me some feedback on these fixes? I will try to implement them, see if they work, but there are probably design considerations I am missing. I understand if the heights are inconsistent, then the height of the placeholders will not be accurate, but as long as the user can at least scroll to all rows in the list, I can accept big scroll jumps.

NeXTs commented 7 years ago

Hey Yeah, it was designed to work with similar rows height and very first row takes for measurements. In your case all rows are same, except very first one. The worst possible scenario :D

In theory as quick fix (if your second row has same height as all others) you can change this line to if(this.content_elem.children.length <= 2) this.html(rows[1] + rows[1] + rows[1]); and measurements should be correct.

I'll take a closer look to this issue in spare time

rjohnson06 commented 7 years ago

I'm going to just force all of our rows to be the same height, That turned out to be much easier than I thought. Thanks for your help.

Was thinking more about the problem of variable heights (for fun). It seems there are issues not just with the last cluster, but with moving between all clusters. Also, even if the average height is known, the different heights are not guaranteed to be evenly distributed. To allow for varying heights, the actual height of the first cluster would have to be measured as well as the heights of the next clusters as you scroll into them. Don't really see a way of avoiding that performance hit.

CaptBli commented 7 years ago

getClusterNum doesn't work correctly as rjohnson06 commented on Jul 21, 2016. The problem is when the number of rows is close to the cluster boundary the last few rows can't be scrolled to because it first insertToDOM() with the next cluster and then tries to scroll to the item and then calls insertToDOM() again with the previous cluster which moves us away. Changing the size of the Cluster only moves it to a new place. While a change directly to the getClusterNum to prevent a change when the last rows are already in the current cluster should fix the problem.

NeXTs commented 7 years ago

Could you please provide example at codepen?

ansari9546 commented 4 years ago

I dont whether it is related to this issue or not but in my case sometime when I create the cluster and immediately scroll then rows starts flickering. wont be able to do any thing untill unless I destroy the cluster and again initialize the cluster. Not know the exact case in which it happens.