custom HTMLElement inside header loses event handler

[ghiscoding]

Describe the bug

This bug was first reported in Angular-Slickgrid but is really code related to Slickgrid-Universal, so I'm closing the Angular-Slickgrid issue and opening this new one with same info

When I use a custom HTMLElement inside the name property of a column definition like this

  const div = document.createElement('div');
  const button = document.createElement('button');
  button.textContent = 'click';
  div.appendChild(button);
  button.addEventListener('click', () => alert('hi'));

    this.columnDefinitions = [
      { id: 'title', name: div, field: 'title' },

In angular slickgrid the click event handler does not trigger whereas if I do the same in a normal slickgrid (6pac repo) grid the click event is fired.

Reproduction

https://stackblitz.com/edit/stackblitz-starters-akwtsl?file=src%2Fslickgrid-angular%2Fslickgrid-angular.component.html

Which Framework are you using?

Angular

Environment Info

| Executable          | Version |
| ------------------- | ------- |
| Angular-Slickgrid   | 8.3.1 |
| Slickgrid-Universal | 5.3.2 |

Validations

• [X] Follow our Code of Conduct
• [X] Read the Wikis.
• [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• [X] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• [X] The provided reproduction is a minimal reproducible example of the bug.

[stackblitz[bot]]

Start a new pull request in StackBlitz Codeflow.

• stackblitz-starters-akwtsl (Open reproduction)

[ghiscoding]

This is a regression caused by PR #1476 which was fixing another bug #1475

in PR #1476, I used cloneNode(true) to clone the HTML element but that method doesn't copy the event listeners. This change does not exist in SlickGrid, hence why the new bug doesn't show up in SlickGrid.

At this point, I'm not sure if the use of cloneNode() is the best approach and if not, then what approach should we use to not regress #1475. Note that the button listener does work after removing the cloneNode() but reopens my previous fix for #1475. If I keep the cloneNode() then I'll have to copy all optional event listeners by hand (not sure how to do that though), see this SO How to copy a DOM node with event listeners?... so anyway, considering this new issue, I don't think the cloneNode() was the best thing to do in the first place, it would require more troubleshooting to understand why bug #1475 happened in the first place and removing the clone would fix current issue

@zewa666 @ardakod I'm opened to suggestion here

side note, my tendency of using animated gif can be quite useful, especially when revisiting old issues like this one

[zewa666]

well the root-cause is understandable.

1. First time around you create the span element for the title. Its not yet attached to the DOM so appendChild inside slickGrid.ts->applyHtmlCode will do so
2. When the picker menu is constructed it tries to assemble the name via pickerHeaderColumnValueExtractor from global-grid-options.ts. In here there is an issue tough ... it grabs the name by supposing it's a string, yet it is the in step1 created span element. Next it calls applyHtmlCode again, which will execute again appendChild on the same element, thus move the element from the column header to the picker.
3. Next time rendering the columnheaders due to column toggling, the same referenced DOM Element will be moved from the picker to the columnHeader, leaving the empty place

so the issue I'd say is how the label for the picker is constructed as merely the text needs to be displayed there but not the actual element moved. I've created a PR with a potential solution.

[ghiscoding]

yeah I found that if I modify the global value extractor as below, it fixes the issue without the need for the clone. I'm not sure reading the .textContent is always the right way to do it though

function pickerHeaderColumnValueExtractor(column: Column, gridOptions?: GridOption) {
  let colName = column?.name ?? '';
  if (colName instanceof HTMLElement) {
    colName = colName.textContent || '';
  }
  const headerGroup = column?.columnGroup || '';
  const columnGroupSeparator = gridOptions?.columnGroupSeparator ?? ' - ';
  if (headerGroup) {
    return headerGroup + columnGroupSeparator + colName;
  }
  return colName;
}

EDIT

ohhh I just realized you did the exact same changes lol, thanks for the PR, I left some comments

[zewa666]

yeah it was just a quick one to show where the potential solution could go. Seeing you did the same makes me feel like its the proper thing to do. I've closed my PR since you're at it anyways.

if you don't like textContent you could instead use the deepClone here since you'll most likely not care about the attached handlers in the context of the columnPicker. I'd personally stick with the textContent as it feels cleaner though

[ghiscoding]

yeah the concern I have is the following (I've also put the same comment in your now closed PR)

I had another possible problem with the .textContent, this will read all element texts but is that really ideal though? I mean I just tested this with an HTML element that has some text and a button with text and it gives me what is shown below (the button text is also pulled). However, I'm not sure that we can do much about it?!?

that is using the following code

const div = document.createElement('div');
    const button = document.createElement('button');
    button.className = 'button button-small';
    button.textContent = 'click';
    button.addEventListener('click', () => alert('hi'));
    div.appendChild(document.createTextNode('Column 1 '));
    div.appendChild(button);

    this.columnDefinitions = [
      {
        id: 'sel', name: div, field: 'num', width: 80, type: FieldType.number,
        excludeFromExport: true,
        resizable: true,
        filterable: true,
        selectable: false,
        focusable: false
      },

[zewa666]

well if you'd want to maintain that functionality here too, which I quite frankly think is a sideeffect users typically dont think off, we'd need a deepClone of the element for the picker.

I think the better approach alltogether would be to introduce something like a pickerLabel: string | HTMLElement | DocumentFragment property on the column interface which would be taken first and textContent of name as a fallback.

[ghiscoding]

https://github.com/ghiscoding/slickgrid-universal/blob/b1d42f539b4e1f501e9652d8c2bcd27f29ea700a/packages/common/src/extensions/slickColumnPicker.ts#L49-L50

so do you mean something like this instead

headerColumnValueExtractor: (columnDef: Column) => getHtmlStringOutput(columnDef.pickerLabel || columnDef.name || '', 'innerHTML')

EDIT

actually it seems that I have to do this change on the global value extractor instead

[zewa666]

yep exactly, that would keep textContent as default, which most likely will be fine 9 out of 10 times. but for these other cases you can create the label to override the behavior

[ghiscoding]

Yeah that seems like a nice feature, now with this new prop (renamed it to columnPickerLabel), it shows my custom label

this.columnDefinitions = [
      {
        id: 'sel', name: div, field: 'num', width: 80, type: FieldType.number,
+       columnPickerLabel: 'Custom Label',
        excludeFromExport: true,
        resizable: true,
        filterable: true,
        selectable: false,
        focusable: false
      },

[ghiscoding]

ok so I've opened PR #1607, I'm just missing a bit more unit tests and E2E tests but apart from that the code should be good as it is and fix this issue without regressing. @zewa666 thanks a ton for the feedback provided, it was again very helpful 😉