From 0db70a442ca59f0e77118ecb5b4d460780c4c15a Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 21 Nov 2016 21:33:02 +0100 Subject: [PATCH] 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. --- src/lib/core/overlay/_overlay.scss | 17 ++- src/lib/core/overlay/overlay-ref.ts | 6 + .../position/global-position-strategy.spec.ts | 113 ++++++++++++------ .../position/global-position-strategy.ts | 111 ++++++++--------- .../overlay/position/position-strategy.ts | 3 + src/lib/snack-bar/snack-bar.ts | 1 - 6 files changed, 152 insertions(+), 99 deletions(-) diff --git a/src/lib/core/overlay/_overlay.scss b/src/lib/core/overlay/_overlay.scss index 5e7fbdab3cf2..9e23def56150 100644 --- 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,21 @@ 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; } + // Wrapper element for the panes that use the GlobalPositionStrategy. + .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 index f2cd0e828451..bfefda7433ce 100644 --- a/src/lib/core/overlay/overlay-ref.ts +++ b/src/lib/core/overlay/overlay-ref.ts @@ -38,6 +38,12 @@ export class OverlayRef implements PortalHost { } dispose(): void { + let strategy = this._state.positionStrategy; + + if (strategy && typeof strategy.dispose === 'function') { + strategy.dispose(); + } + this._detachBackdrop(); this._portalHost.dispose(); } diff --git a/src/lib/core/overlay/position/global-position-strategy.spec.ts b/src/lib/core/overlay/position/global-position-strategy.spec.ts index 2f7f0924dffe..9165e5e1cc88 100644 --- 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,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 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); })); }); diff --git a/src/lib/core/overlay/position/global-position-strategy.ts b/src/lib/core/overlay/position/global-position-strategy.ts index bf0c377abde6..dd5b4a86863d 100644 --- a/src/lib/core/overlay/position/global-position-strategy.ts +++ b/src/lib/core/overlay/position/global-position-strategy.ts @@ -1,65 +1,51 @@ -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 = ''; - - /** 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; - } + private _cssPosition: string = 'static'; + private _topOffset: string = ''; + private _bottomOffset: string = ''; + private _leftOffset: string = ''; + private _rightOffset: string = ''; + private _alignItems: string = ''; + private _justifyContent: string = ''; + 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; } @@ -67,10 +53,9 @@ export class GlobalPositionStrategy implements PositionStrategy { * Centers the overlay horizontally with an optional offset. * Clears any previously set horizontal position. */ - centerHorizontally(offset = '0px') { - this._left = '50%'; - this._right = ''; - this._translateX = ['-50%', offset]; + centerHorizontally(offset = '') { + this.left(offset); + this._justifyContent = 'center'; return this; } @@ -78,10 +63,9 @@ export class GlobalPositionStrategy implements PositionStrategy { * Centers the overlay vertically with an optional offset. * Clears any previously set vertical position. */ - centerVertically(offset = '0px') { - this._top = '50%'; - this._bottom = ''; - this._translateY = ['-50%', offset]; + centerVertically(offset = '') { + this.top(offset); + this._alignItems = 'center'; return this; } @@ -90,24 +74,35 @@ export class GlobalPositionStrategy implements PositionStrategy { * TODO: internal */ apply(element: HTMLElement): Promise { - 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; + 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); + } + + let styles = element.style; + let parentStyles = (element.parentNode as HTMLElement).style; - // TODO(jelbourn): we don't want to always overwrite the transform property here, - // because it will need to be used for animations. - let tranlateX = this._reduceTranslateValues('translateX', this._translateX); - let translateY = this._reduceTranslateValues('translateY', this._translateY); + styles.position = this._cssPosition; + styles.marginTop = this._topOffset; + styles.marginLeft = this._leftOffset; + styles.marginBottom = this._bottomOffset; + styles.marginRight = this._rightOffset; - applyCssTransform(element, `${tranlateX} ${translateY}`); + 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; + } } } diff --git a/src/lib/core/overlay/position/position-strategy.ts b/src/lib/core/overlay/position/position-strategy.ts index 297f526921fe..b5a116239778 100644 --- a/src/lib/core/overlay/position/position-strategy.ts +++ b/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; + + /** Cleans up any DOM modifications made by the position strategy, if necessary. */ + dispose?(): void; } diff --git a/src/lib/snack-bar/snack-bar.ts b/src/lib/snack-bar/snack-bar.ts index db823a81f9d1..bf78a14c5841 100644 --- a/src/lib/snack-bar/snack-bar.ts +++ b/src/lib/snack-bar/snack-bar.ts @@ -120,7 +120,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);