fix(dialog): avoid subpixel rendering issues and refactor GlobalPositionStrategy (#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.

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;

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();
   }

src/lib/core/overlay/overlay.spec.ts

@@ -251,5 +251,6 @@ class FakePositionStrategy implements PositionStrategy {
     return Promise.resolve();
   }
 
+  dispose() {}
 }
 

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.

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,21 +61,28 @@ 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(() => {
@@ -66,10 +90,10 @@ describe('GlobalPositonStrategy', () => {
 
     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(() => {
@@ -77,28 +101,45 @@ describe('GlobalPositonStrategy', () => {
 
     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 default the element to position: absolute', fakeAsyncTest(() => {
+  it('should make the element position: static', fakeAsyncTest(() => {
     strategy.apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.position).toBe('absolute');
+    expect(element.style.position).toBe('static');
   }));
 
-  it('should make the element position: fixed', fakeAsyncTest(() => {
-    strategy.fixed().apply(element);
+  it('should wrap the element in a `md-global-overlay-wrapper`', fakeAsyncTest(() => {
+    strategy.apply(element);
 
     flushMicrotasks();
 
-    expect(element.style.position).toBe('fixed');
+    let parent = element.parentNode as HTMLElement;
+
+    expect(parent.classList.contains('md-global-overlay-wrapper')).toBe(true);
+  }));
+
+
+  it('should remove the parent wrapper from the DOM', fakeAsync(() => {
+    strategy.apply(element);
+
+    flushMicrotasks();
+
+    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,8 +163,8 @@ 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(() => {
@@ -131,8 +172,8 @@ describe('GlobalPositonStrategy', () => {
 
     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');
   }));
 });
 

src/lib/core/overlay/position/global-position-strategy.ts

@@ -1,67 +1,55 @@
-import {applyCssTransform} from '../../style/apply-transform';
 import {PositionStrategy} from './position-strategy';
 
 
 /**
  * A strategy for positioning overlays. Using this strategy, an overlay is given an
- * explicit position relative to the browser's viewport.
+ * explicit position relative to the browser's viewport. We use flexbox, instead of
+ * transforms, in order to avoid issues with subpixel rendering which can cause the
+ * element to become blurry.
  */
 export class GlobalPositionStrategy implements PositionStrategy {
-  private _cssPosition: string = 'absolute';
-  private _top: string = '';
-  private _bottom: string = '';
-  private _left: string = '';
-  private _right: string = '';
+  private _cssPosition: string = 'static';
+  private _topOffset: string = '';
+  private _bottomOffset: string = '';
+  private _leftOffset: string = '';
+  private _rightOffset: string = '';
+  private _alignItems: string = '';
+  private _justifyContent: string = '';
   private _width: string = '';
   private _height: string = '';
 
-  /** Array of individual applications of translateX(). Currently only for centering. */
-  private _translateX: string[] = [];
-
-  /** Array of individual applications of translateY(). Currently only for centering. */
-  private _translateY: string[] = [];
-
-  /** Sets the element to use CSS position: fixed */
-  fixed() {
-    this._cssPosition = 'fixed';
-    return this;
-  }
-
-  /** Sets the element to use CSS position: absolute. This is the default. */
-  absolute() {
-    this._cssPosition = 'absolute';
-    return this;
-  }
+  /* A lazily-created wrapper for the overlay element that is used as a flex container.  */
+  private _wrapper: HTMLElement;
 
   /** Sets the top position of the overlay. Clears any previously set vertical position. */
   top(value: string) {
-    this._bottom = '';
-    this._translateY = [];
-    this._top = value;
+    this._bottomOffset = '';
+    this._topOffset = value;
+    this._alignItems = 'flex-start';
     return this;
   }
 
   /** Sets the left position of the overlay. Clears any previously set horizontal position. */
   left(value: string) {
-    this._right = '';
-    this._translateX = [];
-    this._left = value;
+    this._rightOffset = '';
+    this._leftOffset = value;
+    this._justifyContent = 'flex-start';
     return this;
   }
 
   /** Sets the bottom position of the overlay. Clears any previously set vertical position. */
   bottom(value: string) {
-    this._top = '';
-    this._translateY = [];
-    this._bottom = value;
+    this._topOffset = '';
+    this._bottomOffset = value;
+    this._alignItems = 'flex-end';
     return this;
   }
 
   /** Sets the right position of the overlay. Clears any previously set horizontal position. */
   right(value: string) {
-    this._left = '';
-    this._translateX = [];
-    this._right = value;
+    this._leftOffset = '';
+    this._rightOffset = value;
+    this._justifyContent = 'flex-end';
     return this;
   }
 
@@ -96,14 +84,8 @@ export class GlobalPositionStrategy implements PositionStrategy {
    * Clears any previously set horizontal position.
    */
   centerHorizontally(offset = '') {
-    this._left = '50%';
-    this._right = '';
-    this._translateX = ['-50%'];
-
-    if (offset) {
-      this._translateX.push(offset);
-    }
-
+    this.left(offset);
+    this._justifyContent = 'center';
     return this;
   }
 
@@ -112,14 +94,8 @@ export class GlobalPositionStrategy implements PositionStrategy {
    * Clears any previously set vertical position.
    */
   centerVertically(offset = '') {
-    this._top = '50%';
-    this._bottom = '';
-    this._translateY = ['-50%'];
-
-    if (offset) {
-      this._translateY.push(offset);
-    }
-
+    this.top(offset);
+    this._alignItems = 'center';
     return this;
   }
 
@@ -128,26 +104,37 @@ export class GlobalPositionStrategy implements PositionStrategy {
    * TODO: internal
    */
   apply(element: HTMLElement): Promise<void> {
-    element.style.position = this._cssPosition;
-    element.style.top = this._top;
-    element.style.left = this._left;
-    element.style.bottom = this._bottom;
-    element.style.right = this._right;
-    element.style.width = this._width;
-    element.style.height = this._height;
+    if (!this._wrapper) {
+      this._wrapper = document.createElement('div');
+      this._wrapper.classList.add('md-global-overlay-wrapper');
+      element.parentNode.insertBefore(this._wrapper, element);
+      this._wrapper.appendChild(element);
+    }
 
-    // TODO(jelbourn): we don't want to always overwrite the transform property here,
-    // because it will need to be used for animations.
-    let translateX = this._reduceTranslateValues('translateX', this._translateX);
-    let translateY = this._reduceTranslateValues('translateY', this._translateY);
+    let styles = element.style;
+    let parentStyles = (element.parentNode as HTMLElement).style;
 
-    applyCssTransform(element, `${translateX} ${translateY}`);
+    styles.position = this._cssPosition;
+    styles.marginTop = this._topOffset;
+    styles.marginLeft = this._leftOffset;
+    styles.marginBottom = this._bottomOffset;
+    styles.marginRight = this._rightOffset;
+    styles.width = this._width;
+    styles.height = this._height;
+
+    parentStyles.justifyContent = this._justifyContent;
+    parentStyles.alignItems = this._alignItems;
 
     return Promise.resolve(null);
   }
 
-  /** Reduce a list of translate values to a string that can be used in the transform property */
-  private _reduceTranslateValues(translateFn: string, values: string[]) {
-    return values.map(t => `${translateFn}(${t})`).join(' ');
+  /**
+   * Removes the wrapper element from the DOM.
+   */
+  dispose(): void {
+    if (this._wrapper && this._wrapper.parentNode) {
+      this._wrapper.parentNode.removeChild(this._wrapper);
+      this._wrapper = null;
+    }
   }
 }

src/lib/core/overlay/position/position-strategy.ts

@@ -3,4 +3,7 @@ export interface PositionStrategy {
 
   /** Updates the position of the overlay element. */
   apply(element: Element): Promise<void>;
+
+  /** Cleans up any DOM modifications made by the position strategy, if necessary. */
+  dispose(): void;
 }

src/lib/core/overlay/position/relative-position-strategy.ts

@@ -8,4 +8,8 @@ export class RelativePositionStrategy implements PositionStrategy {
     // Not yet implemented.
     return null;
   }
+
+  dispose() {
+    // Not yet implemented.
+  }
 }

src/lib/dialog/dialog.spec.ts

@@ -169,7 +169,7 @@ describe('MdDialog', () => {
 
     let overlayPane = overlayContainerElement.querySelector('.md-overlay-pane') as HTMLElement;
 
-    expect(overlayPane.style.top).toBe('100px');
+    expect(overlayPane.style.marginTop).toBe('100px');
   });
 
   it('should should override the bottom offset of the overlay pane', () => {
@@ -183,7 +183,7 @@ describe('MdDialog', () => {
 
     let overlayPane = overlayContainerElement.querySelector('.md-overlay-pane') as HTMLElement;
 
-    expect(overlayPane.style.bottom).toBe('200px');
+    expect(overlayPane.style.marginBottom).toBe('200px');
   });
 
   it('should should override the left offset of the overlay pane', () => {
@@ -197,7 +197,7 @@ describe('MdDialog', () => {
 
     let overlayPane = overlayContainerElement.querySelector('.md-overlay-pane') as HTMLElement;
 
-    expect(overlayPane.style.left).toBe('250px');
+    expect(overlayPane.style.marginLeft).toBe('250px');
   });
 
   it('should should override the right offset of the overlay pane', () => {
@@ -211,7 +211,7 @@ describe('MdDialog', () => {
 
     let overlayPane = overlayContainerElement.querySelector('.md-overlay-pane') as HTMLElement;
 
-    expect(overlayPane.style.right).toBe('125px');
+    expect(overlayPane.style.marginRight).toBe('125px');
   });
 
   describe('disableClose option', () => {

src/lib/snack-bar/snack-bar.ts

@@ -121,7 +121,6 @@ export class MdSnackBar {
   private _createOverlay(): OverlayRef {
     let state = new OverlayState();
     state.positionStrategy = this._overlay.position().global()
-        .fixed()
         .centerHorizontally()
         .bottom('0');
     return this._overlay.create(state);