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

Column resizing: handle coalesced pointer move events #3594

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Conversation

nstepien
Copy link
Contributor

@nstepien nstepien commented Sep 5, 2024

Our column resize test are flaky in CI, which lead me to rethink this part of the codebase.

I think what's happening is that when we run the following code

  await page.mouse.down();
  await page.mouse.move(x + resizeBy + 5, y);
  await page.mouse.up();

playwright may not wait for pointer events to be dispatched before executing the next command,
so mouse.up() would run before the next render frame/before the pointermove event is dispatched,
so the pointermove event may be coalesced into the lostpointercapture event.

An alternative could be to add pauses between each mouse commands,
but handling the final pointer position does make sense, and is the right thing to do.

I couldn't reproduce the flaky test locally, so this is all theoretical.

https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/getCoalescedEvents

@nstepien nstepien self-assigned this Sep 5, 2024
}
}

function onLostPointerCapture() {
function onDoubleClick() {
hasDoubleClicked = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have kept the onDoubleClick prop, and used a ref, but I'm not sure that'd be 100% safe, what if we fail to reset the ref in time in the onLostPointerCapture handler?

@nstepien nstepien marked this pull request as ready for review September 5, 2024 18:16
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.27%. Comparing base (75fc5f3) to head (e633cf8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3594      +/-   ##
==========================================
+ Coverage   95.25%   95.27%   +0.02%     
==========================================
  Files          44       44              
  Lines        1559     1566       +7     
  Branches      465      466       +1     
==========================================
+ Hits         1485     1492       +7     
  Misses         74       74              
Files with missing lines Coverage Δ
src/HeaderCell.tsx 92.30% <100.00%> (+0.48%) ⬆️

@nstepien nstepien enabled auto-merge (squash) September 6, 2024 18:39
@nstepien nstepien merged commit adef894 into main Sep 6, 2024
3 checks passed
@nstepien nstepien deleted the coalesce branch September 6, 2024 18:41
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