Table component first check in#1879
Conversation
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| 'checkbox-group': fieldTypes.SELECT, | ||
| 'checkbox': fieldTypes.CHECKBOX, | ||
| 'date-input': fieldTypes.TEXT, | ||
| 'datetime-input': fieldTypes.TEXT, |
There was a problem hiding this comment.
Previously, the replace dialog restricted targets to the same typeMap family (e.g. text-input could only be replaced with TEXT-family components). This was too limiting inside table row cells where any policy-allowed component should be swappable.
Changes:
isUnderCoreTableRow: detects when the editable is inside a table row/header and skips the type-family check entirely for those cells.buildAllowedTemplatePathSet: still enforces the parent container's policy, so only components the row actually allows appear in the list.getTemplateFieldType+ null guard: fixes a crash when a component's.jsonomitsfieldType— now falls back tomodel.json.- Added
datetime-inputandmultiline-inputtotypeMapso they match correctly in standard (non-table) replace flows.
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
| @@ -0,0 +1,1108 @@ | |||
| /******************************************************************************* | |||
There was a problem hiding this comment.
Author-side editor hook for the core table component. Powers all structural editing actions on a table in the AEM page editor — add/delete/reorder rows, add/delete columns, and merge/split header cells — by making Sling POST requests to manipulate JCR nodes directly and refreshing the table editable. Also patches the AEM overlay renderer to correctly position edit overlays on <tr> elements, which can get misplaced due to browser foster-parenting of table markup.
|
|
||
| /** | ||
| * Table row cells must not expose the standard Delete action (row/column flows handle structure). | ||
| * @param {Granite.author.Editable} editable |
There was a problem hiding this comment.
Table row cells must not expose the standard Delete action like normal components. The rows must be deleted through the delete row option added in the edit config of the table row.
552a08c to
e878597
Compare
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Accessibility Violations Found
|
| @@ -0,0 +1,233 @@ | |||
| /******************************************************************************* | |||
There was a problem hiding this comment.
tableview.js — purpose
This file is the runtime view for the Adaptive Form Table component. It has three responsibilities:
-
Row insertion () — inserts a cloned
<tr>into the correct<tbody>position using the row model's index. Handles three cases: empty tbody (append), index 0 (prepend before the first sibling), and mid-list (insert after the preceding sibling resolved by model index). -
Hook attribute patching (
#syncTableRowHooks) —FormView.Utils.updateIdpatches elementidattributes on cloned nodes but does not touchdata-cmp-hook-*attributes. Without this step, cloned add/remove buttons retain the template row's stale ID, causing every dynamically-added row to dispatch the wrong model index. This method re-stampsdata-cmp-hook-add-instanceanddata-cmp-hook-remove-instancewith the newly assigned model ID immediately after insertion. -
min/maxOccur enforcement (
#syncTableRowRepeatableControls) — after every addition or removal, walks every live row and setsdata-cmp-visibleon its add and remove buttons according to the instance manager's current item count vs.minOccur/maxOccur. AmaxOccurof-1means unbounded;minOccurof-1means no floor.
| handler="CQ.FormsCoreComponents.editorhooks.viewQualifiedName" | ||
| icon="viewSOMExpression" | ||
| text="View Qualified Name"/> | ||
| <addcolumn |
There was a problem hiding this comment.
Added addColumn and deleteColumn option in the text editconfig as the option to add column and delete will be displayed when the column cell is clicked which will be a text component so added a condition isCoreTableHeaderCell(editable) to display this option.
armaang1729
left a comment
There was a problem hiding this comment.
Inline comments explaining the key changes in FormPanel.js, InstanceManager.js, and utils.js.
| * @returns {HTMLElement|undefined} | ||
| */ | ||
| getRepeatableInstancesContainerElement() { | ||
| return this.element.parentElement?.parentElement; |
There was a problem hiding this comment.
New virtual hook that returns the DOM container holding all sibling repeat instances. The default (parentElement?.parentElement) covers the standard Sling-wrapped panel structure. Subclasses like TableRow override this to return <tbody> instead, since a <tr> sits at a different nesting depth. instantiateInstanceManager() reads this so InstanceManager knows where to append/insert cloned instances.
| * @returns {HTMLElement|undefined} | ||
| */ | ||
| getRepeatableDomWrapper() { | ||
| return this.element.parentElement; |
There was a problem hiding this comment.
Virtual hook that returns the node to clone as the repeat template and to remove when an instance is deleted. Default is parentElement (the Sling wrapper div). TableRow overrides this to return the <tr> itself. Without this hook, updateTemplate and handleRemoval in InstanceManager would always clone/remove the wrong element for table rows.
| this.markerElement = markerElement; | ||
| **/ | ||
| const wrapper = (typeof childView.getRepeatableDomWrapper === "function") | ||
| ? childView.getRepeatableDomWrapper() |
There was a problem hiding this comment.
Uses the view's declared wrapper node (e.g. <tr> for a table row) instead of always parentElement. Cloning the wrong node would produce a template with a mismatched tag that breaks table structure when inserted into <tbody>.
| } | ||
| return this.handleAddition(addedModel, beforeElement); | ||
| const addedHtmlElement = this.handleAddition(addedModel, beforeElement); | ||
| if (addedHtmlElement) { |
There was a problem hiding this comment.
createFieldsForAddedElement was previously called in two separate branches of #syncViewModel. Consolidating it here means any path through #addChildInstance automatically gets field initialization — no risk of a future caller forgetting it.
| return; | ||
| } | ||
| let removedChildView = this.children.find((cv) => cv && cv.getId() === removedModel.id); | ||
| if (!removedChildView && typeof removedModel.index === "number" && removedModel.index >= 0 |
There was a problem hiding this comment.
Changed from index-based to id-based lookup because model.index can be stale after a prior remove shifts the children array. The index fallback is kept only for cases where the view hasn't been registered by id yet (e.g. prefill-removed instances). this.children.indexOf(removedChildView) at the end re-derives the live array position rather than trusting the model value.
| //removing just the parent view HTML child instance, to avoid repainting of UI for removal of each child HTML | ||
| removedInstanceView.element.parentElement.remove(); | ||
| let toRemove; | ||
| if (typeof removedInstanceView.getRepeatableDomWrapper === "function") { |
There was a problem hiding this comment.
Delegates to getRepeatableDomWrapper so the correct node (e.g. the whole <tr>) is removed. Without this, removing a table row would call .remove() on a <div> wrapper that doesn't exist in the DOM, silently doing nothing.
| const fieldElements = addedElement.querySelectorAll(fieldCreatorSet['fieldSelector']); | ||
| const fieldElements = Array.from(addedElement.querySelectorAll(fieldCreatorSet['fieldSelector'])); | ||
| if (addedElement.matches && addedElement.matches(fieldCreatorSet['fieldSelector'])) { | ||
| fieldElements.unshift(addedElement); |
There was a problem hiding this comment.
querySelectorAll only searches descendants — it never matches the element itself. When addedElement is a <tr data-cmp-is="adaptiveFormTableRow">, the existing call returned nothing and the row's view was never created. The matches check at the root fixes this for any component whose outermost element carries data-cmp-is.
| */ | ||
| static #removeChildReferences(fieldView, formContainer) { | ||
| if (!fieldView) { | ||
| return; |
There was a problem hiding this comment.
Null guard added so a missing fieldView exits early instead of throwing on fieldView.children.
| for (let index = 0; index < childViewList.length; index++) { | ||
| Utils.#removeChildReferences(childViewList[index]); | ||
| Utils.#removeChildReferences(childViewList[index], formContainer); | ||
| } |
There was a problem hiding this comment.
The recursive call was previously missing formContainer, so it received undefined and silently skipped #removeFieldId for all grandchild views. Deep nested panels (e.g. a panel inside a repeatable table row) would leak their field references in the form container's field map.
| */ | ||
| static updateId(htmlElement, oldId, newId) { | ||
| if (htmlElement.id === oldId) { | ||
| htmlElement.id = newId; |
There was a problem hiding this comment.
Same root vs. descendants issue as createFieldsForAddedElement. When htmlElement is the cloned <tr> and its id needs to be swapped, querySelectorAll('#oldId') finds nothing because it only searches children. The early return on a direct id match handles this before falling through to the descendant search.
armaang1729
left a comment
There was a problem hiding this comment.
Inline comments for tablerowview.js and repeatable.updatelabel.cy.js
| * @returns {HTMLElement} | ||
| */ | ||
| getRepeatableDomWrapper() { | ||
| const parent = this.element.parentElement; |
There was a problem hiding this comment.
This is the consumer of the virtual hook defined in FormPanel. The check for bemBlock on parentElement handles the edit-mode case where Sling wraps the <tr> in a <div class="cmp-adaptiveform-tablerow"> for the authoring overlay. In publish mode the <tr> has no such wrapper, so this.element (the <tr> itself) is returned directly. InstanceManager uses this for both cloning the template and tearing down removed rows.
| * @returns {HTMLElement} | ||
| */ | ||
| getRepeatableInstancesContainerElement() { | ||
| const tableBody = this.element.closest(".cmp-adaptiveform-table__body") || |
There was a problem hiding this comment.
Returns <tbody> as the container so InstanceManager.handleAddition inserts cloned rows into the right parent. Using closest handles both the semantic <tbody class="cmp-adaptiveform-table__body"> and any fallback structure, with parentElement as a last resort to stay consistent with the base class default.
| * @param {Object} label - The label state object. | ||
| */ | ||
| updateLabel(label) { | ||
| // Table rows don't have visible labels, so this is a no-op |
There was a problem hiding this comment.
No-op intentionally — table rows have no visible label of their own (column headers are owned by tableheader). Without this override, applyState would call the base class updateLabel which would try to update a non-existent label DOM element.
| * @param {Object} state - The state to be applied. | ||
| */ | ||
| applyState(state) { | ||
| this.updateVisible(state.visible); |
There was a problem hiding this comment.
Matches the FormPanel.applyState signature but explicitly skips anything table-row-irrelevant (e.g. updateLabel is a no-op here). Calling initializeHelpContent is kept because a row's visibility/enabled state still drives data-cmp-visible / data-cmp-enabled attributes that the rules engine reads.
| } | ||
|
|
||
| FormView.Utils.setupField(({element, formContainer}) => { | ||
| return new TableRow({element, formContainer}) |
There was a problem hiding this comment.
Registers TableRow as the view for all elements matching [data-cmp-is="adaptiveFormTableRow"]. This is the standard FormView.Utils.setupField pattern — it both creates the view for elements already in the DOM and wires up the mutation observer so dynamically added rows (repeat add) are initialized automatically.
| checkLabelText(textinputid11, panelid11, 'Text Input2', 'Panel2'); | ||
| // find Panel[1]'s remove button by its model index | ||
| const removeBtn1 = Object.values(formContainer._fields).find(f => | ||
| f.getModel && |
There was a problem hiding this comment.
Previous code used a hardcoded _fields array index to find the remove button, which broke after the instance removal logic was changed to id-based lookup (the field map order is no longer guaranteed to match insertion order). This change finds the remove button by inspecting the model — specifically the parent panel's index === 1 — making the test resilient to field map reordering.
| cy.get(`#${removeBtn1.getId()}`).find("button").click().then(() => { | ||
| // after Panel[1] removed, find the surviving panel at index 1 by model | ||
| const panel1remaining = Object.values(formContainer._fields).find(f => | ||
| f.getModel && |
There was a problem hiding this comment.
After removal, the surviving panel at model index === 1 and its child text input are located dynamically rather than by a fixed _fields position. This is necessary because #removeChildInstance now splices by the live array index, which can shift the order of remaining entries in formContainer._fields.
e878597 to
90ced91
Compare
90ced91 to
16d7af4
Compare
… dependencies which are declared in the content.xml
16d7af4 to
3db51ce
Compare
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: