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

Table incorrectly determines cursor position when scaled by CSS transform. #6919

Open
frangkli opened this issue Jul 19, 2024 · 7 comments
Open

Comments

@frangkli
Copy link

Environment

  • Package version(s): @blueprintjs/icons: 5.10.0, @blueprintjs/table: 5.1.7, @blueprintjs/core: 5.10.5
  • Operating System: MacOS
  • Browser name and version: Arc version 1.51.0

Code Sandbox

Link to a minimal repro: link

Steps to reproduce

  1. Setup a table component
  2. Apply external CSS scale transform
  3. Hover over with mouse

Actual behavior

The cell highlighted is not actually the one that is under the cursor.

Expected behavior

Cell, row, and column correctly highlighted.

Possible solution

Fix conversions in table/src/locator.ts. Done here: commit. Happy to send a PR for this but please advise on unit testing.

@evansjohnson
Copy link
Contributor

I don't think there's any expectation currently that the table should work with CSS scale transforms. It's a reasonable FR, but I don't think we will assign resources towards this any time soon.

If you're able to set up the docs with an example showing this works and figure out how to unit test yourself I could review a PR, but do not have availablility to advise getting this set up.

@frangkli
Copy link
Author

Sure. Are there some special setup steps or requirements for the unit tests? I am running the Karma tests on the HEAD now but the tests are already failing without any code change. The failures are at scrollUtils and TableQuadrantStack.

@evansjohnson
Copy link
Contributor

I recently noticed those as well: #6926

These tests are apparently passing on CI so I imagine it may be something with device settings. I think these tests could be commented out while developing for now. I don't expect we'll have bandwidth to look into why these fail locally any time soon.

I wonder if that will also make it difficult to test zoom related functionality 🤔

The primary concerns here would be ensuring stability of existing functionality.

@frangkli
Copy link
Author

Yeah I've seen that issue as well, I guess I'll just have to comment them out. I assume there are more fundamental fixes for the scale issue with how the dimensions are calculated, but I intend to just have a solution that exposes an extra scale prop as a multiplier that defaults to 1 so it wouldn't break any existing codebases.

@evansjohnson
Copy link
Contributor

Okay, it's fine to explore this but if there are any concerns about stability after a review I won't be able to guarantee we would be able to merge this as a feature

If you need something urgently I'd recommend exploring alternate table solutions. We have also seen issues with the table not supporting customized text sizes in a first class way which you may run into as well (or maybe this is what you're trying to work around?)

@evansjohnson
Copy link
Contributor

thanks for looking into this and sorry about the test issues!

@frangkli
Copy link
Author

No problem, we are using the table in a free-zoom canvas which is why the dynamic zoom becomes a problem. I currently have a fork with the changes implemented and it seems to work fine. If merging is not possible, we'll just maintain the branch ourselves. Regardless, I'll send a PR when I get to implementing the unit tests.

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

No branches or pull requests

2 participants