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: breaks after one call to updateStyleElement() #16211

Closed
ymg2006 opened this issue Aug 11, 2024 · 5 comments · Fixed by #16239 · 4 remaining pull requests
Closed

Table: breaks after one call to updateStyleElement() #16211

ymg2006 opened this issue Aug 11, 2024 · 5 comments · Fixed by #16239 · 4 remaining pull requests
Labels
LTS-FIXED-15.4.26 LTS-FIXED-16.9.15 LTS-PORTABLE Type: Bug Issue contains a bug related to a specific component. Something about the component is not working
Milestone

Comments

@ymg2006
Copy link
Contributor

ymg2006 commented Aug 11, 2024

Describe the bug

@cetincakiroglu @mehmetcetin01140
Commit #16154 completely breaks table after resizeTableCells or onColumnDrop.

@aaronshim maybe reading innerhtml-vs-innertext-vs-textcontent helps to figure out why the whole table is broken after 17.18.7 to 17.18.8 commit.

Environment

Windows 11

Reproducer

No response

Angular version

17.3.6

PrimeNG version

17.18.8

Build / Runtime

Angular CLI App

Language

TypeScript

Node version (for AoT issues node --version)

20.12.1

Browser(s)

All

Steps to reproduce the behavior

Do anything in the table that makes a call to resizeTableCellsm then the whole css and ids and etc are broken.

Expected behavior

No response

@ymg2006 ymg2006 added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Aug 11, 2024
@cetincakiroglu cetincakiroglu added this to the 17.18.9 milestone Aug 11, 2024
ymg2006 pushed a commit to ymg2006/primeng that referenced this issue Aug 11, 2024
…ement()

closes primefaces#16211 use domSanitizer to bypass code generated Trusted Types.
@aaronshim
Copy link
Contributor

Hi @ymg2006

Thanks for surfacing the issue. The three call sites replaced with textContent in #16154 appeared to be <style> tags, which are compatible with having the styles (as in, no nested tags and no markup inside the style) set with textContent. (The reasoning is described in #16153)

I am not sure why there is markup being set to innerHTML of these <style> tags, but thanks for looking into a workaround.

To the maintainers: if this UI breakage behavior was as easy to reproduce as described in this issue, was there a test to catch this that I was supposed to run in the contributor guidelines? Is there a place where we can introduce such basic rendering tests?

@ymg2006
Copy link
Contributor Author

ymg2006 commented Aug 12, 2024

Hi @ymg2006

Thanks for surfacing the issue. The three call sites replaced with textContent in #16154 appeared to be <style> tags, which are compatible with having the styles (as in, no nested tags and no markup inside the style) set with textContent. (The reasoning is described in #16153)

I am not sure why there is markup being set to innerHTML of these <style> tags, but thanks for looking into a workaround.

To the maintainers: if this UI breakage behavior was as easy to reproduce as described in this issue, was there a test to catch this that I was supposed to run in the contributor guidelines? Is there a place where we can introduce such basic rendering tests?

Hi @aaronshim,
Reproducing the issue is simple, create a table and add a lot of data to it, just resize the width of one column, everything will break.
I already have fixed the issue in commit #16212 and also other places where innerHtml is being set as trusted.

@cetincakiroglu
Copy link
Contributor

Hi @ymg2006,

To make it faster and include the fix in this week's milestone, we've fixed this issue for just the table. We'll review the pr of this issue also to make sure nothing breaks in other 19 components. Thanks a lot for reporting the issue!

@cetincakiroglu cetincakiroglu added Type: Bug Issue contains a bug related to a specific component. Something about the component is not working and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Aug 15, 2024
@ryann3588
Copy link

ryann3588 commented Aug 29, 2024

The committed change for this is breaking the responsive styles for the table in 16.9.15-LTS

<style type="text/css">SafeValue must use [property]=binding: @media screen and (max-width: 960px) { #pn_id_26-table > .p-datatable-thead > tr > th, #pn_id_26-table > .p-datatable-tfoot > tr > td { display: none !important; } #pn_id_26-table > .p-datatable-tbody > tr > td { display: flex; width: 100% !important; align-items: center; justify-content: space-between; } #pn_id_26-table > .p-datatable-tbody > tr > td:not(:last-child) { border: 0 none; } #pn_id_26.p-datatable-gridlines > .p-datatable-wrapper > .p-datatable-table > .p-datatable-tbody > tr > td:last-child { border-top: 0; border-right: 0; border-left: 0; } #pn_id_26-table > .p-datatable-tbody > tr > td > .p-column-title { display: block; } } (see https://g.co/ng/security#xss)</style>

@nick-allen
Copy link

See #16291, breaking 17.18.9 as well.

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