fix(dialog): avoid subpixel rendering issues and refactor GlobalPosit…

…ionStrategy (#1962) * fix(dialog): avoid subpixel rendering issues and refactor GlobalPositionStrategy Refactors the `GlobalPositionStrategy` to use flexbox for positioning. This avoids the subpixel rendering issues that come with using transforms for centering. Fixes #932. * Make the `dispose` method required. * Add extra comments regarding the flex wrapper. * Fix failing unit tests. * Fix the unit tests after the recent changes.
angular · Dec 2, 2016 · 1ea6d34 · 1ea6d34
1 parent deb940f
commit 1ea6d34
Show file tree

Hide file tree

Showing 10 changed files with 172 additions and 116 deletions.
diff --git a/src/lib/core/overlay/_overlay.scss b/src/lib/core/overlay/_overlay.scss
@@ -8,10 +8,7 @@
 
   // TODO(jelbourn): change from the `md` prefix to something else for everything in the toolkit.
 
-  // The overlay-container is an invisible element which contains all individual overlays.
-  .md-overlay-container {
-    position: fixed;
-
+  .md-overlay-container, .md-global-overlay-wrapper {
     // Disable events from being captured on the overlay container.
     pointer-events: none;
 
@@ -20,9 +17,24 @@
     left: 0;
     height: 100%;
     width: 100%;
+  }
+
+  // The overlay-container is an invisible element which contains all individual overlays.
+  .md-overlay-container {
+    position: fixed;
     z-index: $md-z-index-overlay-container;
   }
 
+  // We use an extra wrapper element in order to use make the overlay itself a flex item.
+  // This makes centering the overlay easy without running into the subpixel rendering
+  // problems tied to using `transform` and without interfering with the other position
+  // strategies.
+  .md-global-overlay-wrapper {
+    display: flex;
+    position: absolute;
+    z-index: $md-z-index-overlay;
+  }
+
   // A single overlay pane.
   .md-overlay-pane {
     position: absolute;

diff --git a/src/lib/core/overlay/overlay-ref.ts b/src/lib/core/overlay/overlay-ref.ts
@@ -43,6 +43,10 @@ export class OverlayRef implements PortalHost {
   }
 
   dispose(): void {
+    if (this._state.positionStrategy) {
+      this._state.positionStrategy.dispose();
+    }
+
     this._detachBackdrop();
     this._portalHost.dispose();
   }

diff --git a/src/lib/core/overlay/overlay.spec.ts b/src/lib/core/overlay/overlay.spec.ts
@@ -251,5 +251,6 @@ class FakePositionStrategy implements PositionStrategy {
     return Promise.resolve();
   }
 
+  dispose() {}
 }
 
diff --git a/src/lib/core/overlay/position/connected-position-strategy.ts b/src/lib/core/overlay/position/connected-position-strategy.ts
@@ -59,6 +59,11 @@ export class ConnectedPositionStrategy implements PositionStrategy {
     return this._preferredPositions;
   }
 
+  /**
+   * To be used to for any cleanup after the element gets destroyed.
+   */
+  dispose() { }
+
   /**
    * Updates the position of the overlay element, using whichever preferred position relative
    * to the origin fits on-screen.

diff --git a/src/lib/core/overlay/position/global-position-strategy.spec.ts b/src/lib/core/overlay/position/global-position-strategy.spec.ts
@@ -13,28 +13,45 @@ describe('GlobalPositonStrategy', () => {
   beforeEach(() => {
     element = document.createElement('div');
     strategy = new GlobalPositionStrategy();
+    document.body.appendChild(element);
   });
 
-  it('should set explicit (top, left) position to the element', fakeAsyncTest(() => {
-    strategy.top('10px').left('40%').apply(element);
+  afterEach(() => {
+    strategy.dispose();
+  });
+
+  it('should position the element to the (top, left) with an offset', fakeAsyncTest(() => {
+    strategy.top('10px').left('40px').apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('10px');
-    expect(element.style.left).toBe('40%');
-    expect(element.style.bottom).toBe('');
-    expect(element.style.right).toBe('');
+    let elementStyle = element.style;
+    let parentStyle = (element.parentNode as HTMLElement).style;
+
+    expect(elementStyle.marginTop).toBe('10px');
+    expect(elementStyle.marginLeft).toBe('40px');
+    expect(elementStyle.marginBottom).toBe('');
+    expect(elementStyle.marginRight).toBe('');
+
+    expect(parentStyle.justifyContent).toBe('flex-start');
+    expect(parentStyle.alignItems).toBe('flex-start');
   }));
 
-  it('should set explicit (bottom, right) position to the element', fakeAsyncTest(() => {
+  it('should position the element to the (bottom, right) with an offset', fakeAsyncTest(() => {
     strategy.bottom('70px').right('15em').apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('');
-    expect(element.style.left).toBe('');
-    expect(element.style.bottom).toBe('70px');
-    expect(element.style.right).toBe('15em');
+    let elementStyle = element.style;
+    let parentStyle = (element.parentNode as HTMLElement).style;
+
+    expect(elementStyle.marginTop).toBe('');
+    expect(elementStyle.marginLeft).toBe('');
+    expect(elementStyle.marginBottom).toBe('70px');
+    expect(elementStyle.marginRight).toBe('15em');
+
+    expect(parentStyle.justifyContent).toBe('flex-end');
+    expect(parentStyle.alignItems).toBe('flex-end');
   }));
 
   it('should overwrite previously applied positioning', fakeAsyncTest(() => {
@@ -44,61 +61,85 @@ describe('GlobalPositonStrategy', () => {
     strategy.top('10px').left('40%').apply(element);
     flushMicrotasks();
 
-    expect(element.style.top).toBe('10px');
-    expect(element.style.left).toBe('40%');
-    expect(element.style.bottom).toBe('');
-    expect(element.style.right).toBe('');
-    expect(element.style.transform).not.toContain('translate');
+    let elementStyle = element.style;
+    let parentStyle = (element.parentNode as HTMLElement).style;
+
+    expect(elementStyle.marginTop).toBe('10px');
+    expect(elementStyle.marginLeft).toBe('40%');
+    expect(elementStyle.marginBottom).toBe('');
+    expect(elementStyle.marginRight).toBe('');
+
+    expect(parentStyle.justifyContent).toBe('flex-start');
+    expect(parentStyle.alignItems).toBe('flex-start');
 
     strategy.bottom('70px').right('15em').apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('');
-    expect(element.style.left).toBe('');
-    expect(element.style.bottom).toBe('70px');
-    expect(element.style.right).toBe('15em');
-    expect(element.style.transform).not.toContain('translate');
+    expect(element.style.marginTop).toBe('');
+    expect(element.style.marginLeft).toBe('');
+    expect(element.style.marginBottom).toBe('70px');
+    expect(element.style.marginRight).toBe('15em');
+
+    expect(parentStyle.justifyContent).toBe('flex-end');
+    expect(parentStyle.alignItems).toBe('flex-end');
   }));
 
   it('should center the element', fakeAsyncTest(() => {
     strategy.centerHorizontally().centerVertically().apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('50%');
-    expect(element.style.left).toBe('50%');
-    expect(element.style.transform).toContain('translateX(-50%)');
-    expect(element.style.transform).toContain('translateY(-50%)');
+    let parentStyle = (element.parentNode as HTMLElement).style;
+
+    expect(parentStyle.justifyContent).toBe('center');
+    expect(parentStyle.alignItems).toBe('center');
   }));
 
   it('should center the element with an offset', fakeAsyncTest(() => {
     strategy.centerHorizontally('10px').centerVertically('15px').apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('50%');
-    expect(element.style.left).toBe('50%');
-    expect(element.style.transform).toContain('translateX(-50%)');
-    expect(element.style.transform).toContain('translateX(10px)');
-    expect(element.style.transform).toContain('translateY(-50%)');
-    expect(element.style.transform).toContain('translateY(15px)');
+    let elementStyle = element.style;
+    let parentStyle = (element.parentNode as HTMLElement).style;
+
+    expect(elementStyle.marginLeft).toBe('10px');
+    expect(elementStyle.marginTop).toBe('15px');
+
+    expect(parentStyle.justifyContent).toBe('center');
+    expect(parentStyle.alignItems).toBe('center');
+  }));
+
+  it('should make the element position: static', fakeAsyncTest(() => {
+    strategy.apply(element);
+
+    flushMicrotasks();
+
+    expect(element.style.position).toBe('static');
   }));
 
-  it('should default the element to position: absolute', fakeAsyncTest(() => {
+  it('should wrap the element in a `md-global-overlay-wrapper`', fakeAsyncTest(() => {
     strategy.apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.position).toBe('absolute');
+    let parent = element.parentNode as HTMLElement;
+
+    expect(parent.classList.contains('md-global-overlay-wrapper')).toBe(true);
   }));
 
-  it('should make the element position: fixed', fakeAsyncTest(() => {
-    strategy.fixed().apply(element);
+
+  it('should remove the parent wrapper from the DOM', fakeAsync(() => {
+    strategy.apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.position).toBe('fixed');
+    expect(document.body.contains(element.parentNode)).toBe(true);
+
+    strategy.dispose();
+
+    expect(document.body.contains(element.parentNode)).toBe(false);
   }));
 
   it('should set the element width', fakeAsync(() => {
@@ -122,17 +163,17 @@ describe('GlobalPositonStrategy', () => {
 
     flushMicrotasks();
 
-    expect(element.style.left).toBe('0px');
-    expect(element.style.transform).toBe('');
+    expect(element.style.marginLeft).toBe('0px');
+    expect((element.parentNode as HTMLElement).style.justifyContent).toBe('flex-start');
   }));
 
   it('should reset the vertical position and offset when the height is 100%', fakeAsync(() => {
     strategy.centerVertically().height('100%').apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.top).toBe('0px');
-    expect(element.style.transform).toBe('');
+    expect(element.style.marginTop).toBe('0px');
+    expect((element.parentNode as HTMLElement).style.alignItems).toBe('flex-start');
   }));
 });