From ac6b54322ceccd782b640cc5a62335c1b369d1db Mon Sep 17 00:00:00 2001
From: Dani Palou <dani@moodle.com>
Date: Thu, 22 Feb 2018 09:27:49 +0100
Subject: [PATCH] MOBILE-2327 core: Destroy buttons moved by
 core-navbar-buttons

---
 .../messages/pages/discussion/discussion.html |  2 +-
 .../navbar-buttons/navbar-buttons.ts          | 70 +++++++++++++------
 src/providers/utils/dom.ts                    | 12 +++-
 3 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/src/addon/messages/pages/discussion/discussion.html b/src/addon/messages/pages/discussion/discussion.html
index 8589244d0..3b2e2db08 100644
--- a/src/addon/messages/pages/discussion/discussion.html
+++ b/src/addon/messages/pages/discussion/discussion.html
@@ -7,7 +7,7 @@
         <button ion-button icon-only clear="true" (click)="toggleDelete()" [hidden]="!canDelete">
             <ion-icon name="trash"></ion-icon>
         </button>
-        <a *ngIf="showProfileLink" core-user-link [userId]="userId" [attr.aria-label]=" 'core.user.viewprofile' | translate">
+        <a [hidden]="!showProfileLink" core-user-link [userId]="userId" [attr.aria-label]=" 'core.user.viewprofile' | translate">
             <img class="button core-bar-button-image" [src]="profileLink" core-external-content onError="this.src='assets/img/user-avatar.png'">
         </a>
     </core-navbar-buttons>
diff --git a/src/components/navbar-buttons/navbar-buttons.ts b/src/components/navbar-buttons/navbar-buttons.ts
index b56416cbf..c9e1a8d30 100644
--- a/src/components/navbar-buttons/navbar-buttons.ts
+++ b/src/components/navbar-buttons/navbar-buttons.ts
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import { Component, Input, OnInit, ContentChildren, ElementRef, QueryList } from '@angular/core';
+import { Component, Input, OnInit, OnDestroy, ContentChildren, ElementRef, QueryList } from '@angular/core';
 import { Button } from 'ionic-angular';
 import { CoreLoggerProvider } from '../../providers/logger';
 import { CoreDomUtilsProvider } from '../../providers/utils/dom';
@@ -26,10 +26,12 @@ import { CoreDomUtilsProvider } from '../../providers/utils/dom';
  *
  * You can use the [hidden] input to hide all the inner buttons if a certain condition is met.
  *
+ * IMPORTANT: Do not use *ngIf in the buttons inside this component, it can cause problems. Please use [hidden] instead.
+ *
  * Example usage:
  *
  * <core-navbar-buttons end>
- *     <button ion-button icon-only *ngIf="buttonShown" [attr.aria-label]="Do something" (click)="action()">
+ *     <button ion-button icon-only [hidden]="!buttonShown" [attr.aria-label]="Do something" (click)="action()">
  *         <ion-icon name="funnel"></ion-icon>
  *     </button>
  * </core-navbar-buttons>
@@ -38,33 +40,27 @@ import { CoreDomUtilsProvider } from '../../providers/utils/dom';
     selector: 'core-navbar-buttons',
     template: '<ng-content></ng-content>'
 })
-export class CoreNavBarButtonsComponent implements OnInit {
+export class CoreNavBarButtonsComponent implements OnInit, OnDestroy {
 
     protected BUTTON_HIDDEN_CLASS = 'core-navbar-button-hidden';
 
     // If the hidden input is true, hide all buttons.
     @Input('hidden') set hidden(value: boolean) {
         this._hidden = value;
-        if (this._buttons) {
-            this._buttons.forEach((button: Button) => {
-                this.showHideButton(button);
-            });
-        }
+        this.showHideAllElements();
     }
 
-    // Get all the buttons inside this directive.
+    // Get all the ion-buttons inside this directive and apply the role bar-button.
     @ContentChildren(Button) set buttons(buttons: QueryList<Button>) {
-        this._buttons = buttons;
         buttons.forEach((button: Button) => {
             button.setRole('bar-button');
-            this.showHideButton(button);
         });
     }
 
     protected element: HTMLElement;
-    protected _buttons: QueryList<Button>;
     protected _hidden: boolean;
     protected logger: any;
+    protected movedChildren: Node[];
 
     constructor(element: ElementRef, logger: CoreLoggerProvider, private domUtils: CoreDomUtilsProvider) {
         this.element = element.nativeElement;
@@ -91,7 +87,9 @@ export class CoreNavBarButtonsComponent implements OnInit {
                 if (buttonsContainer) {
                     this.mergeContextMenus(buttonsContainer);
 
-                    this.domUtils.moveChildren(this.element, buttonsContainer);
+                    this.movedChildren = this.domUtils.moveChildren(this.element, buttonsContainer);
+                    this.showHideAllElements();
+
                 } else {
                     this.logger.warn('The header was found, but it didn\'t have the right ion-buttons.', selector);
                 }
@@ -188,15 +186,45 @@ export class CoreNavBarButtonsComponent implements OnInit {
     }
 
     /**
-     * Show or hide a button.
-     *
-     * @param {Button} button Button to show or hide.
+     * Show or hide all the elements.
      */
-    protected showHideButton(button: Button): void {
-        if (this._hidden) {
-            button.getNativeElement().classList.add(this.BUTTON_HIDDEN_CLASS);
-        } else {
-            button.getNativeElement().classList.remove(this.BUTTON_HIDDEN_CLASS);
+    protected showHideAllElements(): void {
+        if (this.movedChildren) {
+            this.movedChildren.forEach((child: Node) => {
+                this.showHideElement(child);
+            });
+        }
+    }
+
+    /**
+     * Show or hide an element.
+     *
+     * @param {Node} element Element to show or hide.
+     */
+    protected showHideElement(element: Node): void {
+        // Check if it's an HTML Element
+        if (element instanceof Element) {
+            if (this._hidden) {
+                element.classList.add(this.BUTTON_HIDDEN_CLASS);
+            } else {
+                element.classList.remove(this.BUTTON_HIDDEN_CLASS);
+            }
+        }
+    }
+
+    /**
+     * Component destroyed.
+     */
+    ngOnDestroy(): void {
+        // This component was destroyed, remove all the buttons that were moved.
+        // The buttons can be moved outside of the current page, that's why we need to manually destroy them.
+        // There's no need to destroy context menu items that were merged because they weren't moved from their DOM position.
+        if (this.movedChildren) {
+            this.movedChildren.forEach((child) => {
+                if (child.parentElement) {
+                    child.parentElement.removeChild(child);
+                }
+            });
         }
     }
 }
diff --git a/src/providers/utils/dom.ts b/src/providers/utils/dom.ts
index 7706c15fd..233f98841 100644
--- a/src/providers/utils/dom.ts
+++ b/src/providers/utils/dom.ts
@@ -480,11 +480,19 @@ export class CoreDomUtilsProvider {
      *
      * @param {HTMLElement} oldParent The old parent.
      * @param {HTMLElement} newParent The new parent.
+     * @return {Node[]} List of moved children.
      */
-    moveChildren(oldParent: HTMLElement, newParent: HTMLElement): void {
+    moveChildren(oldParent: HTMLElement, newParent: HTMLElement): Node[] {
+        const movedChildren: Node[] = [];
+
         while (oldParent.childNodes.length > 0) {
-            newParent.appendChild(oldParent.childNodes[0]);
+            const child = oldParent.childNodes[0];
+            movedChildren.push(child);
+
+            newParent.appendChild(child);
         }
+
+        return movedChildren;
     }
 
     /**