Skip to content

Fix: use latest IntersectionObserver entry to determine visibility (#4751) #4756

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

Merged
merged 2 commits into from
Jul 5, 2025

Conversation

mumberrymountain
Copy link
Contributor

@mumberrymountain mumberrymountain commented May 16, 2025

  • Fixes a bug by using the latest IntersectionObserver entry to determine element visibility

@azmy60
Copy link
Collaborator

azmy60 commented Jun 23, 2025

hey @mumberrymountain , thanks for the PR!

Not sure what we are trying to fix here. Could you please add more details like steps to replicate the issue this PR is fixing? Probably a video to go through this would be very helpful as well! :)

@mumberrymountain
Copy link
Contributor Author

mumberrymountain commented Jun 23, 2025

Hello @azmy60

The issue I Tried to fix is #4751. You can find more details in the link, and you can reproduce the errors using this sample:
https://jsfiddle.net/LTXHNN/L47s10gc/4/

this.visibilityObserver = new IntersectionObserver((entries) => {
this.visible = entries[0].isIntersecting;
			
if(!this.initialized){
	this.initialized = true;
	this.initialRedraw = !this.visible;
}else{
	if(this.visible){
		this.redrawTable(this.initialRedraw);
		this.initialRedraw = false;
	}
}
});
		
this.visibilityObserver.observe(this.table.element);

I’m not the person who first wrote this code... so I can't say for certain, but I infer that this code has been added to redraw the table when the element first becomes visible, or whenever it becomes visible again.

this.visible = entries[0].isIntersecting;

the code check entries[0].isIntersectingto determine whether element is visible or not, but when the intersection states of observed targets change in quick succession, the browser batches those changes and passes them together in a single callback, which means entries[0] might not represent the latest state of the element.

container.removeChild(tableDIv); // observer only checks visibility states of this one when tableDiv is removed from the container

let data = [
   { id: 1, name: "王五", age: 28, city: "深圳", startDate: "2021-09-10" },
];
table.addData(data);

container.appendChild(tableDIv); // but the observer is supposed to check visibility state of this one when tableDiv is shown again and redraw the table

So, as I mentioned in comment above, observer fails to detect the latest visibility state of the table element, and does not redraw the table, even though data has been added so it needs to be redrawn.

This is the reason why I fixed entries[0].isIntersecting to entries[entries.length - 1].isIntersecting

@azmy60
Copy link
Collaborator

azmy60 commented Jun 24, 2025

Oh I see. Just tested your solution and it works! But before we merge, we need to add a unit test. Let me know if you are open to do so.

@mumberrymountain
Copy link
Contributor Author

@azmy60 I can add tests if you like me do so. Is there any convention I suppose to follow? or do I just have to simply add new test file to test/unit folder?

@azmy60
Copy link
Collaborator

azmy60 commented Jun 25, 2025

Since this fixes the ResizeTable module, the new test should be in test/unit/modules/ResizeTable.spec.js.

@mumberrymountain
Copy link
Contributor Author

@azmy60 Hey, just finished adding unit tests for this issue.

@azmy60
Copy link
Collaborator

azmy60 commented Jul 5, 2025

Thanks!

@azmy60 azmy60 merged commit 56bfd08 into olifolkerd:master Jul 5, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants