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(pat-inject): Fix problem with inserting table rows. #1238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thet
Copy link
Member

@thet thet commented Mar 31, 2025

Problem

Daniel reported a problem on signal concerning table-row injections. At one point in the current release cycle I had to add a temporary wrapper element to be able to load a string and treat it as a DOM element so that javascript functions can be used to identify elements.
I used a DIV.
However tr elements cannot be added to divs, therefore the table-row injection failed.

Solution in this Pull Request

A <tr> can live within a <template>. Changing the temporary div to a template solves the issue.

This temporary wrapper never makes it into the page, instead is only needed to transform an HTML string to a DOM node, so that we can do DOM operations, like querySelectorAll on it. That is used to find all images within the to-be-injected source and attach an event handler on it.
After that operation, the tag is discarded again.

Technical note
There was a problem with injecting table rows introduced in: b0f94fb The problem occured when setting a table row as content of a temporary <div> wrapper. But a <tr> is not a valid child node of a <div>. This caused visually destroyed tables. Using a <template> tag as wrapper instead of a div solved the problem.

There was a problem with injecting table rows introduced in: b0f94fb
The problem occured when setting a table row as content of a temporary
<div> wrapper. But a <tr> is not a valid child node of a <div>. This
caused visually destroyed tables.

Using a <template> tag as wrapper instead of a div solved the problem.
@thet
Copy link
Member Author

thet commented Mar 31, 2025

.

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.

1 participant