Remove onLayoutMeasure and getLayoutSize from sidebars (#32519)

* Remove onLayoutMeasure and getLayoutSize from sidebars * change resize logic slightly * ensure to execute toolbar logic at least once
ampproject · Feb 9, 2021 · ff93080 · ff93080
1 parent aace6d6
commit ff93080
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 32 deletions.
diff --git a/extensions/amp-sidebar/0.1/amp-sidebar.js b/extensions/amp-sidebar/0.1/amp-sidebar.js
@@ -34,6 +34,10 @@ import {descendsFromStory} from '../../../src/utils/story';
 import {dev, devAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {handleAutoscroll} from './autoscroll';
+import {
+  observeContentSize,
+  unobserveContentSize,
+} from '../../../src/utils/size-observer';
 import {removeFragment} from '../../../src/url';
 import {setModalAsClosed, setModalAsOpen} from '../../../src/modal';
 import {setStyles, toggle} from '../../../src/style';
@@ -141,6 +145,11 @@ export class AmpSidebar extends AMP.BaseElement {
 
     /** @private {boolean} */
     this.disableSwipeClose_ = false;
+
+    this.onResized_ = this.onResized_.bind(this);
+
+    /** @private {?UnlistenDef} */
+    this.onViewportResizeUnlisten_ = null;
   }
 
   /** @override */
@@ -196,20 +205,7 @@ export class AmpSidebar extends AMP.BaseElement {
             this.user().error(TAG, 'Failed to instantiate toolbar', e);
           }
         });
-
-        if (toolbarElements.length) {
-          this.getViewport().onResize(
-            debounce(
-              this.win,
-              () => {
-                this.toolbars_.forEach((toolbar) => {
-                  toolbar.onLayoutChange();
-                });
-              },
-              100
-            )
-          );
-        }
+        this.onResized_();
       });
 
     if (this.isIos_) {
@@ -297,6 +293,22 @@ export class AmpSidebar extends AMP.BaseElement {
     this.setupGestures_(this.element);
   }
 
+  /** @override */
+  attachedCallback() {
+    this.onViewportResizeUnlisten_ = this.viewport_.onResize(
+      debounce(this.win, this.onResized_, 100)
+    );
+    this.onResized_();
+  }
+
+  /** @override */
+  detachedCallback() {
+    if (this.onViewportResizeUnlisten_) {
+      this.onViewportResizeUnlisten_();
+      this.onViewportResizeUnlisten_ = null;
+    }
+  }
+
   /**
    * Loads the extension for nested menu if sidebar contains one and it
    * has not been installed already.
@@ -374,8 +386,8 @@ export class AmpSidebar extends AMP.BaseElement {
     return screenReaderCloseButton;
   }
 
-  /** @override */
-  onLayoutMeasure() {
+  /** @private */
+  onResized_() {
     this.getAmpDoc()
       .whenReady()
       .then(() => {
@@ -524,6 +536,8 @@ export class AmpSidebar extends AMP.BaseElement {
       this.openerElement_ = openerElement;
       this.initialScrollTop_ = this.viewport_.getScrollTop();
     }
+
+    observeContentSize(this.element, this.onResized_);
   }
 
   /**
@@ -571,6 +585,7 @@ export class AmpSidebar extends AMP.BaseElement {
         tryFocus(this.openerElement_);
       }
     }
+    unobserveContentSize(this.element, this.onResized_);
     return true;
   }
 

diff --git a/extensions/amp-sidebar/0.2/amp-sidebar.js b/extensions/amp-sidebar/0.2/amp-sidebar.js
@@ -35,6 +35,10 @@ import {dev, devAssert, userAssert} from '../../../src/log';
 import {dict} from '../../../src/utils/object';
 import {handleAutoscroll} from './autoscroll';
 import {isExperimentOn} from '../../../src/experiments';
+import {
+  observeContentSize,
+  unobserveContentSize,
+} from '../../../src/utils/size-observer';
 import {removeFragment} from '../../../src/url';
 import {setModalAsClosed, setModalAsOpen} from '../../../src/modal';
 import {setStyles, toggle} from '../../../src/style';
@@ -141,6 +145,11 @@ export class AmpSidebar extends AMP.BaseElement {
       // The sidebar is already animated by swipe to dismiss, so skip animation.
       () => this.dismiss_(true, ActionTrust.HIGH)
     );
+
+    this.onResized_ = this.onResized_.bind(this);
+
+    /** @private {?UnlistenDef} */
+    this.onViewportResizeUnlisten_ = null;
   }
 
   /** @override */
@@ -192,20 +201,7 @@ export class AmpSidebar extends AMP.BaseElement {
             this.user().error(TAG, 'Failed to instantiate toolbar', e);
           }
         });
-
-        if (toolbarElements.length) {
-          this.getViewport().onResize(
-            debounce(
-              this.win,
-              () => {
-                this.toolbars_.forEach((toolbar) => {
-                  toolbar.onLayoutChange();
-                });
-              },
-              100
-            )
-          );
-        }
+        this.onResized_();
       });
 
     this.maybeBuildNestedMenu_();
@@ -294,6 +290,22 @@ export class AmpSidebar extends AMP.BaseElement {
     this.setupGestures_(this.element);
   }
 
+  /** @override */
+  attachedCallback() {
+    this.onViewportResizeUnlisten_ = this.viewport_.onResize(
+      debounce(this.win, this.onResized_, 100)
+    );
+    this.onResized_();
+  }
+
+  /** @override */
+  detachedCallback() {
+    if (this.onViewportResizeUnlisten_) {
+      this.onViewportResizeUnlisten_();
+      this.onViewportResizeUnlisten_ = null;
+    }
+  }
+
   /**
    * Loads the extension for nested menu if sidebar contains one and it
    * has not been installed already.
@@ -371,8 +383,8 @@ export class AmpSidebar extends AMP.BaseElement {
     return screenReaderCloseButton;
   }
 
-  /** @override */
-  onLayoutMeasure() {
+  /** @private */
+  onResized_() {
     this.getAmpDoc()
       .whenReady()
       .then(() => {
@@ -522,6 +534,8 @@ export class AmpSidebar extends AMP.BaseElement {
       this.openerElement_ = openerElement;
       this.initialScrollTop_ = this.viewport_.getScrollTop();
     }
+
+    observeContentSize(this.element, this.onResized_);
   }
 
   /**
@@ -565,6 +579,7 @@ export class AmpSidebar extends AMP.BaseElement {
         tryFocus(this.openerElement_);
       }
     }
+    unobserveContentSize(this.element, this.onResized_);
     return true;
   }