Enterprise-ng: Context menu breaks after toggling the element DOM

[volante007]

Description
When an element using soho-context-menu is toggled using *ngIf, the connected soho-popupmenu is destroyed and removed from DOM when the soho-context-menu is destroyed. This causes issues when several elements share the same soho-popupmenu, or if the toggled element is later shown.

To Reproduce

1. Edit \app\context-menu\context-menu.html sample to use *ngIf toggle

<div class="field">
    <label id="label1" name="label1" soho-label for="field1" >{{normalText}}</label>
    <input id="field1" name="field1" soho-context-menu menu="action-popupmenu" value="Right Click Here"
           *ngIf="!hideInput"
           (selected)="onSelected()"
           (beforeopen)="onBeforeOpen()"
           (close)="onClose()"
           (open)="onOpen()" />
  </div>
  <button soho-button="tertiary" (click)="hideInput = !hideInput">Toggle Input field</button>

1. Run the sample
2. Right-click the various elements that share the same soho-popupmenu element (works fine)
3. Remove input field by clicking the Toggle Input field button
4. Re-add input field by clicking the Toggle Input field button again
5. Right-click any of the elements that use the soho-popupmenu

Expected behavior
Popup-menu is shown on right-click

Actual behavior
Popup-menu is not shown. If right-clicking the toggled element, error is shown in console.

Screenshots
Attached a GIF showing the issue in sample

Additional context
I think sharing a menu between several different items is a common scenario, and this is no longer possible if any of the elements is removed from DOM (temporarily or not). In Homepages Menu widget, all the links / folders share the same context popup-menu, and when navigating between different links, context menu can no longer be used.

The issue was introduced when properly destroying the popup menu in context-menu-directive.ts.
https://github.com/infor-design/enterprise-ng/commit/9ce090817f961043cf8fb806d732ab7315b9d2b0#diff-864e73c329d001149d6cca6689f37fc4

(it used to set it to null only, but is now fully destroyed, unwrapped and removed from DOM with this.contextMenu.destroy() )

Possible solution
Would it be possible to have an @Input to soho-context-menu, unwrapOnDestroy or similar, that defaults to false? If so, do not destroy and remove connected popupmenu from DOM.

[tmcconechy]

@pwpatton - Whats your thoughts on this one?

[pwpatton]

The markup for the menu should include the menu-wrapper element. Instead of it being generated by soho (that's the first thing as ngIf keeps track of elements to remove for later so if the element get moved around angular throws an error).

[pwpatton]

What's the markup for the context menu itself?

[volante007]

In sample:

 <!-- Context menu used in all examples (except the menu button)-->
  <ul soho-popupmenu id="action-popupmenu" >
    <li soho-popupmenu-item *ngFor="let menuEntry of contextEntries" [isDisabled]="menuEntry.disabled">
      <a soho-popupmenu-label *ngIf="!menuEntry.url" menuId="{{menuEntry.id}}"  >{{menuEntry.displayString}}</a>
      <a soho-popupmenu-label *ngIf="menuEntry.url" menuUrl="{{menuEntry.url}}" >{{menuEntry.displayString}}</a>
    </li>
  </ul>

I've not seen any samples of soho-popupmenu / soho-context-menu where the wrapper is included. But even if the wrapper is included, it will still be removed from DOM, not moved around?

wrapper.off().remove();

Line 2096 in teardown(), popupmenu.js

After this, none of the soho-context-menu items will work since their shared menu is gone.

[pwpatton]

And that's fine when destroying the menu. But this is what we do and it seems to work fine.

  <div *ngIf="displayContextMenu" class="popupmenu-wrapper" role="application" aria-hidden="true">
    <ul soho-popupmenu ... ></ul>
  </div>

[volante007]

The issue is not toggling the soho-popupmenu, but the soho-context-menu elements that share the same soho-popupmenu.

[bthharper]

I assume this is because destroy is being called on the popupmenu? The removal of the wrapper code is in the standard jQuery widget.

[volante007]

Yes it's in the jQuery control, called from soho-context-menu ngOnDestroy. And since context menu is a popupmenu control in the end, it destroys and removes the related soho-popupmenu, when not desired.

[bthharper]

As the popupmenu is separately managed (by the soho-popupmenu component), I would say that the jQuery context-menu control should not destroy the attached popupmenu widget. That will be destroyed when the soho-popupmenu component is destroyed.

Maybe we need an option on the jQuery control to disable/enable the destruction of the associated popupmenu? Set to disable by default in angular apps?

Although, I would have thought this issue could be replicated in a vanilla jQuery apps too? So maybe implicit destruction of the popupmenu should never be performed.

[volante007]

Yes, I think an easy fix is to remove
this.contextMenu.destroy(); // actually popupmenu.destroy()
from soho-context-menu.directive.ts

Because as you say Theo, destroy() will be called for the popup menu plugin in ngOnDestroy of soho-popupmenu.component.ts

[bthharper]

Gotcha - it's the same jquery popupmenu api in both directive and component.

I agree - we should remove the call to destroy in the directive, as it's managed elsewhere.

[pwpatton]

I agree as well, for context menu leave destruction in the hands of the controlling component. We'll need to identify those what those are and have them see if they have a soho-context-menu directive to call destroy on.

[tmcconechy]

BTW this example does show it working in the enterprise components http://master-enterprise.demo.design.infor.com/components/contextmenu/example-index.html

If thats the solution that works someone will pr?

[bthharper]

Yes - needs to be assigned. I can do it later if that's okay, unless @pwpatton has time.

[bthharper]

@volante as you correctly identified, just comment out the destroy in soho-context-menu.directive.ts.

[bthharper]

Use of a popupmenu component with several different html elements as context menus does not work as expected if the element hosting the popupmenu is removed from the DOM (which is often the case with angular components).

This issue is not just an angular issue, but can also be replicated in raw enterprise widgets.

The following markup creates two inputs, each sharing the same popup menu. Deleting the first input prevents the context menu from appearing for the remaining input.

    <button id="popdown-destroy" class="btn" type="button">
      <span>Destroy</span>
    </button>

    <div class="field" id="field1">
      <label for="input-menu">Label</label>
      <input type="text" value="Right Click Me" id="input-menu"/>
    </div>

    <div class="field" id="field2">
      <label for="input-menu2">Label2</label>
      <input type="text" value="Right Click Me" id="input-menu2"/>
    </div>

    <ul id="action-popupmenu" class="popupmenu">
      <li><a href="#">Cut</a></li>
      <li><a href="#">Copy</a></li>
      <li><a href="#">Paste</a></li>
    </ul>

    // Share the popupmenu with the two context menus.
    const contextMenu = $('#input-menu');
    contextMenu.popupmenu({trigger:'rightClick', menu: "action-popupmenu", attachToBody: true})
    const contextMenuApi = contextMenu.data('popupmenu');

    const contextMenu2 = $('#input-menu2')
    contextMenu2.popupmenu({trigger:'rightClick', menu: "action-popupmenu", attachToBody: true});
    const contextMenuApi2 = contextMenu2.data('popupmenu');

    const popupMenu = $('#action-popupmenu');
    popupMenu.popupmenu({ attachToBody: true });
    const popupMenuApi = popupMenu.data('popupmenu');

    // This emulates an *ngIf on the first field, where it's presence is toggled.
    $('#popdown-destroy').click(() => {
      // turn off events
      contextMenu.off();

      // destroy the api
      contextMenuApi.destroy();

      // Remove the context.
      contextMenu.parent().remove();
    });

[bthharper]

@tmcconechy we discussed this in teams and we felt the ep controls need to be changed to reference count usages of the popupmenu.

Did you get around to raising an issue in the enterprise project as I couldn't find one?

[tmcconechy]

I think the issue is this one https://github.com/infor-design/enterprise/issues/1025

[bthharper]

Thank Tim, not sure why I couldn't find it now!

[tmcconechy]

Think this has been completed