refactor(connected-position-strategy): use top/left instead of transforms for positioning (#2024)

* refactor(connected-position-strategy): use top/left instead of transforms for positioning

Switches the `ConnectedPositionStrategy` to use `top` and `left` for positioning, instead of `translateX` and `translateY`. This avoids having to do extra work to prevent subpixel rendering issues, while freeing up the transforms to be used on animations or for offsets.

* Fix the failing positioning-related tests.

Author: crisbeto

e2e/components/menu/menu-page.ts

@@ -51,17 +51,15 @@ export class MenuPage {
     });
   }
 
-  expectMenuLocation(el: ElementFinder, {x,y}: {x: number, y: number}) {
-    el.getLocation().then((loc) => {
-      // Round the values because we expect the menu overlay to also have been rounded
-      // to avoid blurriness due to subpixel rendering.
-      expect(loc.x).toEqual(Math.round(x));
-      expect(loc.y).toEqual(Math.round(y));
+  expectMenuLocation(el: ElementFinder, {x, y}: {x: number, y: number}) {
+    el.getLocation().then(loc => {
+      expect(loc.x).toEqual(x);
+      expect(loc.y).toEqual(y);
     });
   }
 
   expectMenuAlignedWith(el: ElementFinder, id: string) {
-    element(by.id(id)).getLocation().then((loc) => {
+    element(by.id(id)).getLocation().then(loc => {
       this.expectMenuLocation(el, {x: loc.x, y: loc.y});
     });
   }

e2e/components/menu/menu.e2e.ts

@@ -12,7 +12,7 @@ describe('menu', () => {
     page.trigger().click();
 
     page.expectMenuPresent(true);
-    expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
+    expect(page.menu().getText()).toEqual('One\nTwo\nThree\nFour');
   });
 
   it('should close menu when menu item is clicked', () => {
@@ -39,7 +39,7 @@ describe('menu', () => {
 
   it('should support multiple triggers opening the same menu', () => {
     page.triggerTwo().click();
-    expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
+    expect(page.menu().getText()).toEqual('One\nTwo\nThree\nFour');
     page.expectMenuAlignedWith(page.menu(), 'trigger-two');
 
     page.backdrop().click();
@@ -48,7 +48,7 @@ describe('menu', () => {
     // TODO(kara): temporary, remove when #1607 is fixed
     browser.sleep(250);
     page.trigger().click();
-    expect(page.menu().getText()).toEqual("One\nTwo\nThree\nFour");
+    expect(page.menu().getText()).toEqual('One\nTwo\nThree\nFour');
     page.expectMenuAlignedWith(page.menu(), 'trigger');
 
     page.backdrop().click();
@@ -57,7 +57,7 @@ describe('menu', () => {
 
   it('should mirror classes on host to menu template in overlay', () => {
     page.trigger().click();
-    page.menu().getAttribute('class').then((classes) => {
+    page.menu().getAttribute('class').then(classes => {
       expect(classes).toContain('md-menu-panel custom');
     });
   });
@@ -157,7 +157,7 @@ describe('menu', () => {
 
     it('should align overlay end to origin end when x-position is "before"', () => {
       page.beforeTrigger().click();
-      page.beforeTrigger().getLocation().then((trigger) => {
+      page.beforeTrigger().getLocation().then(trigger => {
 
         // the menu's right corner must be attached to the trigger's right corner.
         // menu = 112px wide. trigger = 60px wide.  112 - 60 =  52px of menu to the left of trigger.
@@ -169,7 +169,7 @@ describe('menu', () => {
 
     it('should align overlay bottom to origin bottom when y-position is "above"', () => {
       page.aboveTrigger().click();
-      page.aboveTrigger().getLocation().then((trigger) => {
+      page.aboveTrigger().getLocation().then(trigger => {
 
         // the menu's bottom corner must be attached to the trigger's bottom corner.
         // menu.x should equal trigger.x because only y position has changed.
@@ -181,7 +181,7 @@ describe('menu', () => {
 
     it('should align menu to top left of trigger when "below" and "above"', () => {
       page.combinedTrigger().click();
-      page.combinedTrigger().getLocation().then((trigger) => {
+      page.combinedTrigger().getLocation().then(trigger => {
 
         // trigger.x (left corner) - 52px (menu left of trigger) = expected menu.x
         // trigger.y (top corner) - 44px (menu above trigger) = expected menu.y

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

@@ -138,8 +138,9 @@ describe('Overlay directives', () => {
       fixture.detectChanges();
 
       const pane = overlayContainerElement.children[0] as HTMLElement;
-      expect(pane.style.transform)
-          .toContain(`translateX(${startX + 5}px)`,
+
+      expect(pane.style.left)
+          .toBe(startX + 5 + 'px',
               `Expected overlay translateX to equal the original X + the offsetX.`);
 
       fixture.componentInstance.isOpen = false;
@@ -149,8 +150,8 @@ describe('Overlay directives', () => {
       fixture.componentInstance.isOpen = true;
       fixture.detectChanges();
 
-      expect(pane.style.transform)
-          .toContain(`translateX(${startX + 15}px)`,
+      expect(pane.style.left)
+          .toBe(startX + 15 + 'px',
               `Expected overlay directive to reflect new offsetX if it changes.`);
     });
 
@@ -167,19 +168,18 @@ describe('Overlay directives', () => {
       // expected y value is the starting y + trigger height + offset y
       // 30 + 20 + 45 = 95px
       const pane = overlayContainerElement.children[0] as HTMLElement;
-      expect(pane.style.transform)
-          .toContain(`translateY(95px)`,
-              `Expected overlay translateY to equal the start Y + height + offsetY.`);
+
+      expect(pane.style.top)
+          .toBe('95px', `Expected overlay translateY to equal the start Y + height + offsetY.`);
 
       fixture.componentInstance.isOpen = false;
       fixture.detectChanges();
 
       fixture.componentInstance.offsetY = 55;
       fixture.componentInstance.isOpen = true;
       fixture.detectChanges();
-      expect(pane.style.transform)
-          .toContain(`translateY(105px)`,
-              `Expected overlay directive to reflect new offsetY if it changes.`);
+      expect(pane.style.top)
+          .toBe('105px', `Expected overlay directive to reflect new offsetY if it changes.`);
     });
 
   });

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

@@ -1,7 +1,6 @@
 import {PositionStrategy} from './position-strategy';
 import {ElementRef} from '@angular/core';
 import {ViewportRuler} from './viewport-ruler';
-import {applyCssTransform} from '../../style/apply-transform';
 import {
     ConnectionPositionPair,
     OriginConnectionPosition,
@@ -232,18 +231,11 @@ export class ConnectedPositionStrategy implements PositionStrategy {
    * @param overlayPoint
    */
   private _setElementPosition(element: HTMLElement, overlayPoint: Point) {
-    // Round the values to prevent blurry overlays due to subpixel rendering.
-    let x = Math.round(overlayPoint.x);
-    let y = Math.round(overlayPoint.y);
-
-    // TODO(jelbourn): we don't want to always overwrite the transform property here,
-    // because it will need to be used for animations.
-    applyCssTransform(element, `translateX(${x}px) translateY(${y}px)`);
+    element.style.left = overlayPoint.x + 'px';
+    element.style.top = overlayPoint.y + 'px';
   }
 }
 
 
 /** A simple (x, y) coordinate. */
 type Point = {x: number, y: number};
-
-

src/lib/menu/menu-trigger.ts

@@ -24,7 +24,7 @@ import {
     HorizontalConnectionPos,
     VerticalConnectionPos,
 } from '../core';
-import { Subscription } from 'rxjs/Subscription';
+import {Subscription} from 'rxjs/Subscription';
 import {MenuPositionX, MenuPositionY} from './menu-positions';
 
 /**

src/lib/menu/menu.spec.ts

@@ -148,19 +148,19 @@ describe('MdMenu', () => {
 
       fixture.componentInstance.trigger.openMenu();
       fixture.detectChanges();
-      const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+      const overlayPane = getOverlayPane();
       const triggerRect = trigger.getBoundingClientRect();
       const overlayRect = overlayPane.getBoundingClientRect();
 
       // In "before" position, the right sides of the overlay and the origin are aligned.
       // To find the overlay left, subtract the menu width from the origin's right side.
       const expectedLeft = triggerRect.right - overlayRect.width;
-      expect(overlayRect.left)
+      expect(Math.round(overlayRect.left))
           .toBe(Math.round(expectedLeft),
               `Expected menu to open in "before" position if "after" position wouldn't fit.`);
 
       // The y-position of the overlay should be unaffected, as it can already fit vertically
-      expect(overlayRect.top)
+      expect(Math.round(overlayRect.top))
           .toBe(Math.round(triggerRect.top),
               `Expected menu top position to be unchanged if it can fit in the viewport.`);
     });
@@ -177,19 +177,19 @@ describe('MdMenu', () => {
 
       fixture.componentInstance.trigger.openMenu();
       fixture.detectChanges();
-      const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+      const overlayPane = getOverlayPane();
       const triggerRect = trigger.getBoundingClientRect();
       const overlayRect = overlayPane.getBoundingClientRect();
 
       // In "above" position, the bottom edges of the overlay and the origin are aligned.
       // To find the overlay top, subtract the menu height from the origin's bottom edge.
       const expectedTop = triggerRect.bottom - overlayRect.height;
-      expect(overlayRect.top)
+      expect(Math.round(overlayRect.top))
           .toBe(Math.round(expectedTop),
               `Expected menu to open in "above" position if "below" position wouldn't fit.`);
 
       // The x-position of the overlay should be unaffected, as it can already fit horizontally
-      expect(overlayRect.left)
+      expect(Math.round(overlayRect.left))
           .toBe(Math.round(triggerRect.left),
               `Expected menu x position to be unchanged if it can fit in the viewport.`);
     });
@@ -207,18 +207,18 @@ describe('MdMenu', () => {
 
       fixture.componentInstance.trigger.openMenu();
       fixture.detectChanges();
-      const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+      const overlayPane = getOverlayPane();
       const triggerRect = trigger.getBoundingClientRect();
       const overlayRect = overlayPane.getBoundingClientRect();
 
       const expectedLeft = triggerRect.right - overlayRect.width;
       const expectedTop = triggerRect.bottom - overlayRect.height;
 
-      expect(overlayRect.left)
+      expect(Math.round(overlayRect.left))
           .toBe(Math.round(expectedLeft),
               `Expected menu to open in "before" position if "after" position wouldn't fit.`);
 
-      expect(overlayRect.top)
+      expect(Math.round(overlayRect.top))
           .toBe(Math.round(expectedTop),
               `Expected menu to open in "above" position if "below" position wouldn't fit.`);
     });
@@ -230,23 +230,28 @@ describe('MdMenu', () => {
 
       fixture.componentInstance.trigger.openMenu();
       fixture.detectChanges();
-      const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+      const overlayPane = getOverlayPane();
       const triggerRect = trigger.getBoundingClientRect();
       const overlayRect = overlayPane.getBoundingClientRect();
 
       // As designated "before" position won't fit on screen, the menu should fall back
       // to "after" mode, where the left sides of the overlay and trigger are aligned.
-      expect(overlayRect.left)
+      expect(Math.round(overlayRect.left))
           .toBe(Math.round(triggerRect.left),
               `Expected menu to open in "after" position if "before" position wouldn't fit.`);
 
       // As designated "above" position won't fit on screen, the menu should fall back
       // to "below" mode, where the top edges of the overlay and trigger are aligned.
-      expect(overlayRect.top)
+      expect(Math.round(overlayRect.top))
           .toBe(Math.round(triggerRect.top),
               `Expected menu to open in "below" position if "above" position wouldn't fit.`);
     });
 
+    function getOverlayPane(): HTMLElement {
+      let pane = overlayContainerElement.children[0] as HTMLElement;
+      pane.style.position = 'absolute';
+      return pane;
+    }
   });
 
   describe('animations', () => {

src/lib/select/select.spec.ts

@@ -515,6 +515,11 @@ describe('MdSelect', () => {
      */
     function checkTriggerAlignedWithOption(index: number): void {
       const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+
+      // We need to set the position to absolute, because the top/left positioning won't work
+      // since the component CSS isn't included in the tests.
+      overlayPane.style.position = 'absolute';
+
       const triggerTop = trigger.getBoundingClientRect().top;
       const overlayTop = overlayPane.getBoundingClientRect().top;
       const options = overlayPane.querySelectorAll('md-option');
@@ -685,6 +690,11 @@ describe('MdSelect', () => {
         fixture.detectChanges();
 
         const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+
+        // We need to set the position to absolute, because the top/left positioning won't work
+        // since the component CSS isn't included in the tests.
+        overlayPane.style.position = 'absolute';
+
         const triggerBottom = trigger.getBoundingClientRect().bottom;
         const overlayBottom = overlayPane.getBoundingClientRect().bottom;
         const scrollContainer = overlayPane.querySelector('.md-select-panel');
@@ -712,6 +722,11 @@ describe('MdSelect', () => {
         fixture.detectChanges();
 
         const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+
+        // We need to set the position to absolute, because the top/left positioning won't work
+        // since the component CSS isn't included in the tests.
+        overlayPane.style.position = 'absolute';
+
         const triggerTop = trigger.getBoundingClientRect().top;
         const overlayTop = overlayPane.getBoundingClientRect().top;
         const scrollContainer = overlayPane.querySelector('.md-select-panel');
@@ -740,6 +755,11 @@ describe('MdSelect', () => {
         fixture.detectChanges();
 
         const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+
+        // We need to set the position to absolute, because the top/left positioning won't work
+        // since the component CSS isn't included in the tests.
+        overlayPane.style.position = 'absolute';
+
         const triggerLeft = trigger.getBoundingClientRect().left;
         const firstOptionLeft =
             overlayPane.querySelector('md-option').getBoundingClientRect().left;
@@ -758,6 +778,11 @@ describe('MdSelect', () => {
         fixture.detectChanges();
 
         const overlayPane = overlayContainerElement.children[0] as HTMLElement;
+
+        // We need to set the position to absolute, because the top/left positioning won't work
+        // since the component CSS isn't included in the tests.
+        overlayPane.style.position = 'absolute';
+
         const triggerRight = trigger.getBoundingClientRect().right;
         const firstOptionRight =
             overlayPane.querySelector('md-option').getBoundingClientRect().right;
@@ -768,7 +793,6 @@ describe('MdSelect', () => {
             .toEqual((triggerRight + 16).toFixed(2),
                 `Expected trigger to align with the selected option on the x-axis in RTL.`);
       });
-
     });
 
   });