fix(dialog): Wait for rAF/timeout to apply open class (#3682)

(cherry picked from commit 4fd076b)
material-components · Oct 8, 2018 · 3206521 · 3206521
1 parent 8c8dee8
commit 3206521
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 32 deletions.
diff --git a/packages/mdc-dialog/README.md b/packages/mdc-dialog/README.md
@@ -331,7 +331,6 @@ Method Signature | Description
 `addBodyClass(className: string) => void` | Adds a class to the `<body>`.
 `removeBodyClass(className: string) => void` | Removes a class from the `<body>`.
 `eventTargetMatches(target: !EventTarget, selector: string) => void` | Returns `true` if the target element matches the given CSS selector, otherwise `false`.
-`computeBoundingRect()`: Forces the component to recalculate its layout; in the vanilla DOM implementation, this calls `computeBoundingClientRect`.
 `trapFocus() => void` | Sets up the DOM such that keyboard navigation is restricted to focusable elements within the dialog surface (see [Handling Focus Trapping](#handling-focus-trapping) below for more details).
 `releaseFocus() => void` | Removes any effects of focus trapping on the dialog surface (see [Handling Focus Trapping](#handling-focus-trapping) below for more details).
 `isContentScrollable() => boolean` | Returns `true` if `mdc-dialog__content` can be scrolled by the user, otherwise `false`.

diff --git a/packages/mdc-dialog/adapter.js b/packages/mdc-dialog/adapter.js
@@ -65,9 +65,6 @@ class MDCDialogAdapter {
    */
   eventTargetMatches(target, selector) {}
 
-  /** @return {!ClientRect} */
-  computeBoundingRect() {}
-
   trapFocus() {}
   releaseFocus() {}
 

diff --git a/packages/mdc-dialog/foundation.js b/packages/mdc-dialog/foundation.js
@@ -46,7 +46,6 @@ class MDCDialogFoundation extends MDCFoundation {
       addBodyClass: (/* className: string */) => {},
       removeBodyClass: (/* className: string */) => {},
       eventTargetMatches: (/* target: !EventTarget, selector: string */) => {},
-      computeBoundingRect: () => {},
       trapFocus: () => {},
       releaseFocus: () => {},
       isContentScrollable: () => {},
@@ -70,6 +69,9 @@ class MDCDialogFoundation extends MDCFoundation {
     /** @private {boolean} */
     this.isOpen_ = false;
 
+    /** @private {number} */
+    this.animationFrame_ = 0;
+
     /** @private {number} */
     this.animationTimer_ = 0;
 
@@ -100,6 +102,10 @@ class MDCDialogFoundation extends MDCFoundation {
       this.close(strings.DESTROY_ACTION);
     }
 
+    if (this.animationFrame_) {
+      cancelAnimationFrame(this.animationFrame_);
+    }
+
     if (this.animationTimer_) {
       clearTimeout(this.animationTimer_);
       this.handleAnimationTimerEnd_();
@@ -116,19 +122,19 @@ class MDCDialogFoundation extends MDCFoundation {
     this.adapter_.notifyOpening();
     this.adapter_.addClass(cssClasses.OPENING);
 
-    // Force redraw now that display is no longer "none", to establish basis for animation
-    this.adapter_.computeBoundingRect();
-    this.adapter_.addClass(cssClasses.OPEN);
-    this.adapter_.addBodyClass(cssClasses.SCROLL_LOCK);
+    // Wait a frame once display is no longer "none", to establish basis for animation
+    this.runNextAnimationFrame_(() => {
+      this.adapter_.addClass(cssClasses.OPEN);
+      this.adapter_.addBodyClass(cssClasses.SCROLL_LOCK);
 
-    this.layout();
+      this.layout();
 
-    clearTimeout(this.animationTimer_);
-    this.animationTimer_ = setTimeout(() => {
-      this.handleAnimationTimerEnd_();
-      this.adapter_.trapFocus();
-      this.adapter_.notifyOpened();
-    }, numbers.DIALOG_ANIMATION_OPEN_TIME_MS);
+      this.animationTimer_ = setTimeout(() => {
+        this.handleAnimationTimerEnd_();
+        this.adapter_.trapFocus();
+        this.adapter_.notifyOpened();
+      }, numbers.DIALOG_ANIMATION_OPEN_TIME_MS);
+    });
   }
 
   /**
@@ -269,6 +275,20 @@ class MDCDialogFoundation extends MDCFoundation {
     this.adapter_.removeClass(cssClasses.OPENING);
     this.adapter_.removeClass(cssClasses.CLOSING);
   }
+
+  /**
+   * Runs the given logic on the next animation frame, using setTimeout to factor in Firefox reflow behavior.
+   * @param {Function} callback
+   * @private
+   */
+  runNextAnimationFrame_(callback) {
+    cancelAnimationFrame(this.animationFrame_);
+    this.animationFrame_ = requestAnimationFrame(() => {
+      this.animationFrame_ = 0;
+      clearTimeout(this.animationTimer_);
+      this.animationTimer_ = setTimeout(callback, 0);
+    });
+  }
 }
 
 export default MDCDialogFoundation;
diff --git a/packages/mdc-dialog/index.js b/packages/mdc-dialog/index.js
@@ -182,7 +182,6 @@ class MDCDialog extends MDCComponent {
       addBodyClass: (className) => document.body.classList.add(className),
       removeBodyClass: (className) => document.body.classList.remove(className),
       eventTargetMatches: (target, selector) => matches(target, selector),
-      computeBoundingRect: () => this.root_.getBoundingClientRect(),
       trapFocus: () => this.focusTrap_.activate(),
       releaseFocus: () => this.focusTrap_.deactivate(),
       isContentScrollable: () => !!this.content_ && util.isScrollable(/** @type {!Element} */ (this.content_)),

diff --git a/test/unit/mdc-dialog/foundation.test.js b/test/unit/mdc-dialog/foundation.test.js
@@ -61,7 +61,7 @@ test('default adapter returns a complete adapter implementation', () => {
   verifyDefaultAdapter(MDCDialogFoundation, [
     'addClass', 'removeClass', 'hasClass',
     'addBodyClass', 'removeBodyClass', 'eventTargetMatches',
-    'computeBoundingRect', 'trapFocus', 'releaseFocus',
+    'trapFocus', 'releaseFocus',
     'isContentScrollable', 'areButtonsStacked', 'getActionFromEvent', 'clickDefaultButton', 'reverseButtons',
     'notifyOpening', 'notifyOpened', 'notifyClosing', 'notifyClosed',
   ]);
@@ -106,13 +106,38 @@ test('#destroy removes animating classes if called when the dialog is animating'
   td.verify(mockAdapter.removeClass(cssClasses.CLOSING));
 });
 
+test('#destroy cancels layout handling if called on same frame as layout', () => {
+  const {foundation, mockAdapter} = setupTest();
+  const mockRaf = createMockRaf();
+
+  foundation.layout();
+  foundation.destroy();
+  mockRaf.flush();
+
+  try {
+    td.verify(mockAdapter.areButtonsStacked(), {times: 0});
+    td.verify(mockAdapter.isContentScrollable(), {times: 0});
+  } finally {
+    mockRaf.restore();
+  }
+});
+
 test('#open adds CSS classes', () => {
   const {foundation, mockAdapter} = setupTest();
+  const mockRaf = createMockRaf();
+  const clock = lolex.install();
 
   foundation.open();
+  mockRaf.flush();
+  clock.tick(0);
 
-  td.verify(mockAdapter.addClass(cssClasses.OPEN));
-  td.verify(mockAdapter.addBodyClass(cssClasses.SCROLL_LOCK));
+  try {
+    td.verify(mockAdapter.addClass(cssClasses.OPEN));
+    td.verify(mockAdapter.addBodyClass(cssClasses.SCROLL_LOCK));
+  } finally {
+    mockRaf.restore();
+    clock.uninstall();
+  }
 });
 
 test('#close removes CSS classes', () => {
@@ -128,16 +153,20 @@ test('#close removes CSS classes', () => {
 
 test('#open adds the opening class to start an animation, and removes it after the animation is done', () => {
   const {foundation, mockAdapter} = setupTest();
+  const mockRaf = createMockRaf();
   const clock = lolex.install();
 
   foundation.open();
+  mockRaf.flush();
+  clock.tick(0);
 
   try {
     td.verify(mockAdapter.addClass(cssClasses.OPENING));
     td.verify(mockAdapter.removeClass(cssClasses.OPENING), {times: 0});
     clock.tick(numbers.DIALOG_ANIMATION_OPEN_TIME_MS);
     td.verify(mockAdapter.removeClass(cssClasses.OPENING));
   } finally {
+    mockRaf.restore();
     clock.uninstall();
   }
 });
@@ -162,15 +191,20 @@ test('#close adds the closing class to start an animation, and removes it after
 
 test('#open activates focus trapping on the dialog surface', () => {
   const {foundation, mockAdapter} = setupTest();
+  const mockRaf = createMockRaf();
   const clock = lolex.install();
 
   foundation.open();
 
+  // Wait for application of opening class and setting of additional timeout prior to full open animation timeout
+  mockRaf.flush();
+  clock.tick(0);
   clock.tick(numbers.DIALOG_ANIMATION_OPEN_TIME_MS);
 
   try {
     td.verify(mockAdapter.trapFocus());
   } finally {
+    mockRaf.restore();
     clock.uninstall();
   }
 });
@@ -187,15 +221,19 @@ test('#close deactivates focus trapping on the dialog surface', () => {
 
 test('#open emits "opening" and "opened" events', () => {
   const {foundation, mockAdapter} = setupTest();
+  const mockRaf = createMockRaf();
   const clock = lolex.install();
 
   foundation.open();
+  mockRaf.flush();
+  clock.tick(0);
 
   try {
     td.verify(mockAdapter.notifyOpening(), {times: 1});
     clock.tick(numbers.DIALOG_ANIMATION_OPEN_TIME_MS);
     td.verify(mockAdapter.notifyOpened(), {times: 1});
   } finally {
+    mockRaf.restore();
     clock.uninstall();
   }
 });
@@ -261,11 +299,21 @@ test('#isOpen returns false when the dialog is closed after being open', () => {
 
 test('#open recalculates layout', () => {
   const {foundation} = setupTest();
+  const mockRaf = createMockRaf();
+  const clock = lolex.install();
+
   foundation.layout = td.func('layout');
 
   foundation.open();
+  mockRaf.flush();
+  clock.tick(0);
 
-  td.verify(foundation.layout());
+  try {
+    td.verify(foundation.layout());
+  } finally {
+    mockRaf.restore();
+    clock.uninstall();
+  }
 });
 
 test(`#layout removes ${cssClasses.STACKED} class, detects stacked buttons, and adds class`, () => {

diff --git a/test/unit/mdc-dialog/mdc-dialog.test.js b/test/unit/mdc-dialog/mdc-dialog.test.js
@@ -302,17 +302,6 @@ test('adapter#eventTargetMatches returns whether or not the target matches the s
   assert.isFalse(adapter.eventTargetMatches(target, '.non-existent-class'));
 });
 
-test('adapter#computeBoundingRect calls getBoundingClientRect() on root', () => {
-  const {root, component} = setupTest();
-  document.body.appendChild(root);
-
-  try {
-    assert.deepEqual(component.getDefaultFoundation().adapter_.computeBoundingRect(), root.getBoundingClientRect());
-  } finally {
-    document.body.removeChild(root);
-  }
-});
-
 test(`adapter#notifyOpening emits ${strings.OPENING_EVENT}`, () => {
   const {component} = setupTest();