Refactor InlinePanel implementation to be CSP-compatible #13168
Conversation
lb-
added
component:Security
component:Panels
javascript
There was a problem hiding this comment.
@Chiemezuo thank you for getting momentum on this, I have a few comments if that's OK.
I have not run this latest locally but feel free to ping me on this PR if you want me to do a fuller review with running locally.
I would also suggest we need some more unit tests for the changes to the OrderableController.
| 'maxFormsInput', | ||
| 'minFormsInput', |
There was a problem hiding this comment.
I know this re-order was in one of the earlier commits, but it's probably not necessary, feel free to keep the minFormsInput before the maxFormsInput.
| this.totalValue = parseInt(this.totalFormsInputTarget.value, 10); | ||
| this.minValue = parseInt(this.minFormsInputTarget.value, 10); | ||
| this.maxValue = parseInt(this.maxFormsInputTarget.value, 10); | ||
| } | ||
|
|
||
| /** | ||
| * Ensure that any deleted containers are hidden when connected (from HTML response) |
There was a problem hiding this comment.
| * Ensure that any deleted containers are hidden when connected (from HTML response) | |
| * Ensure that any deleted children are hidden when connected (from HTML POST response) |
| * Ensure that any deleted containers are hidden when connected (from HTML response) | ||
| * and remove any error message elements so that it doesn't count towards the number | ||
| * of errors on the tab at the top of the page. | ||
| * @todo - check this actually works for any timing issues from w-count controller. |
There was a problem hiding this comment.
Have you checked this for timing issues? If so, maybe remove this @todo.
| @@ -148,20 +175,25 @@ export class FormsetController extends Controller<HTMLElement> { | |||
| target.setAttribute( | |||
| targetAttrName, | |||
| (target.getAttribute(targetAttrName)?.split(' ') ?? []) | |||
| .filter((name) => name !== 'child') | |||
| // .filter((name) => name !== 'child') | |||
There was a problem hiding this comment.
If we no longer need this, best to just remove the line instead of commenting out.
| @@ -173,7 +205,19 @@ export class FormsetController extends Controller<HTMLElement> { | |||
| * When removed, add the class and update the total count. | |||
| * Also update the DELETE input for this form. | |||
| */ | |||
| childTargetDisconnected(target: HTMLElement) { | |||
| deletedTargetConnected(target: HTMLElement) { | |||
| // only run if the target was previously a child (non-deleted) target | |||
There was a problem hiding this comment.
Maybe put this comment in the method's JSDoc instead.
| if field.name.endswith(forms.formsets.MIN_NUM_FORM_COUNT): | ||
| field.field.widget.attrs["data-w-formset-target"] = "minFormsInput" | ||
| if field.name.endswith(forms.formsets.MAX_NUM_FORM_COUNT): | ||
| field.field.widget.attrs["data-w-formset-target"] = "maxFormsInput" |
There was a problem hiding this comment.
I am not 100% sure how, but I feel as though we can do better here and either somehow pull in the abstractions already provided from wagtail/admin/forms/formsets.py or pull out into a method within this class.
I am sure @laymonage may have some suggestions.
| ) | ||
|
|
||
| attrs["data-w-formset-deleted-class"] = ( | ||
| "w-transition-opacity w-duration-300 w-ease-out w-opacity-0" |
There was a problem hiding this comment.
Similar to above, it would be great to see us somehow use the abstraction that contains this class already in wagtail/admin/forms/formsets.py.
I am not 100% sure how though.
cc @laymonage
| # Set data-controller, including any existing value | ||
| # Used by the w-formset controller (FormsetController) | ||
| # to add dynamic behaviour | ||
| attrs["data-controller"] = " ".join( |
There was a problem hiding this comment.
We will want to add unit tests to ensure we do correctly keep any data-controller that's set on an InlinePanel panel class.
| @@ -15,40 +15,22 @@ | |||
| {% endif %} | |||
|
|
|||
| <div id="id_{{ self.formset.prefix }}-FORMS"> | |||
There was a problem hiding this comment.
Why not put the data-w-formset-target="forms" & data-w-orderable-target="container" here?
| <div id="id_{{ self.formset.prefix }}-FORMS"> | |
| <div id="id_{{ self.formset.prefix }}-FORMS" data-w-formset-target="forms" data-w-orderable-target="container"> |
And keep this as the parent of the inline panel children as per the current code?
Remembering that the inline_panel.html will have the data-controller=... on it, so we can still have the mostly existing DOM structure in this template.
Maybe review these changes and see if there's a cleaner way without such a large diff to this file. I am pretty sure it was solved in our call by setting the data-controller on the outer InlinePanel class in Python.
| @@ -2,17 +2,25 @@ | |||
|
|
|||
| {% fragment as id %}inline_child_{{ child.form.prefix }}{% endfragment %} | |||
| {% fragment as panel_id %}{{ id }}-panel{% endfragment %} | |||
| <div data-inline-panel-child id="{{ id }}" data-contentpath-disabled> | |||
| <div data-w-formset-target="child" id="{{ id }}" data-contentpath-disabled data-w-orderable-target="item" data-w-orderable-item-id="{{ id }}"> | |||
There was a problem hiding this comment.
Please move the id to before the data attributes, as per our style guide.
https://docs.wagtail.org/en/stable/contributing/ui_guidelines.html#html-guidelines
laymonage
changed the title
This PR fixes the first and third of the sub-issues in #12940 and is a follow-up to @lb- 's proof of concept branch: https://github.com/wagtail/wagtail/compare/main...lb-:poc/formset-round-two?expand=1 along with some more refinements also by him.
It is also a part of the CSP compatibility project for GSoC 2025
Summary of Changes:
inline_panel.htmland an accompanying refactoring to use thew-formsetandw-orderablecontrollers there and ininline_panel_child.htmladd.htmlandedit.htmlw-formsetrefactorings for some forms across board.Tests?
Still underway.