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

Fix offset() for elements within foreign object context #10483

Merged
merged 6 commits into from Sep 4, 2023

Conversation

kumilingus
Copy link
Contributor

Context

After inserting the table into SVG , the cell selection boxes will stop appearing. The user can edit the cell, but the input element is hidden (or misplaced).

Selecting the B3 cell:
image

Editing the B3 cell:
image

I managed to locate the root cause of the problem. It lies within the DOM utility offset that does not account for the DOM element to be inside a foreign object. The foreign object has neither offsetLeft nor offsetTop properties defined and the offset() method ends up returning { top: NaN, left: NaN }.

An example use case is the possibility to insert tables into an SVG document and connect columns or rows using links.
https://jsfiddle.net/kumilingus/j4s8bf97/

How has this been tested?

  1. A unit test has been added.
  2. A patched version of Handsontable with the same demo: https://jsfiddle.net/kumilingus/fq7zyr5b/

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Related issue(s):

  1. [Bug]: Selection is not displayed when the table is in a foreign object #10480

Affected project(s):

  • handsontable

Checklist:

@kumilingus
Copy link
Contributor Author

It seems that your unit test environment does not allow the offset method to be tested? Perhaps that's the reason why there was no test for the method in the first place?

The DOM elements seem to have offsetParent always set to null and no size. Any suggestion how to test the issue?

@AMBudnik
Copy link
Contributor

AMBudnik commented Sep 1, 2023

Hi @kumilingus

I think that at this point, the PR is ready for a review. Thank you for sharing.

@budnix
Copy link
Member

budnix commented Sep 1, 2023

The DOM elements seem to have offsetParent always set to null and no size. Any suggestion how to test the issue?

The *.unit.js files are run in a fake DOM environment (JSDOM), and the results may not be accurate. I'd suggest moving the test to the element.spec.js file. *.spec.js files are running in the real browser.

@kumilingus
Copy link
Contributor Author

Hi @budnix, I moved the test to the element.spec.js file, but I wasn't sure how to run it. 🤦 😄

@budnix
Copy link
Member

budnix commented Sep 4, 2023

Hi @budnix, I moved the test to the element.spec.js file, but I wasn't sure how to run it. 🤦 😄

I reviewed the code, and besides one mistake, it looks good 👌

PS: You can run these tests by running npm run test:e2e (in the handsontable directory), or you can narrow them by npm run test:e2e --testPathPattern=element.

Co-authored-by: Krzysztof Budnik <571316+budnix@users.noreply.github.com>
@budnix budnix merged commit 091c0be into handsontable:develop Sep 4, 2023
18 of 19 checks passed
budnix added a commit that referenced this pull request Sep 4, 2023
budnix added a commit that referenced this pull request Sep 4, 2023
@AMBudnik
Copy link
Contributor

AMBudnik commented Dec 1, 2023

Hi @kumilingus

I have great news! Your merged pull request was just published with Handsontable v14.0.0

Thank you again for your input.

Here's #9754 the full list of changes for this release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants