Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(overlay): prevent blurry connected overlays #1784

Merged
merged 4 commits into from
Nov 17, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions e2e/components/menu/menu-page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,10 @@ export class MenuPage {

expectMenuLocation(el: ElementFinder, {x,y}: {x: number, y: number}) {
el.getLocation().then((loc) => {
expect(loc.x).toEqual(x);
expect(loc.y).toEqual(y);
// 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));
});
}

Expand Down
5 changes: 3 additions & 2 deletions src/lib/core/overlay/position/connected-position-strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,9 @@ export class ConnectedPositionStrategy implements PositionStrategy {
* @param overlayPoint
*/
private _setElementPosition(element: HTMLElement, overlayPoint: Point) {
let x = overlayPoint.x;
let y = overlayPoint.y;
// 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.
Expand Down
32 changes: 16 additions & 16 deletions src/lib/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,13 @@ describe('MdMenu', () => {
// 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.toFixed(2))
.toEqual(expectedLeft.toFixed(2),
expect(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.toFixed(2))
.toEqual(triggerRect.top.toFixed(2),
expect(overlayRect.top)
.toBe(Math.round(triggerRect.top),
`Expected menu top position to be unchanged if it can fit in the viewport.`);
});

Expand All @@ -184,13 +184,13 @@ describe('MdMenu', () => {
// 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.toFixed(2))
.toEqual(expectedTop.toFixed(2),
expect(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.toFixed(2))
.toEqual(triggerRect.left.toFixed(2),
expect(overlayRect.left)
.toBe(Math.round(triggerRect.left),
`Expected menu x position to be unchanged if it can fit in the viewport.`);
});

Expand All @@ -214,12 +214,12 @@ describe('MdMenu', () => {
const expectedLeft = triggerRect.right - overlayRect.width;
const expectedTop = triggerRect.bottom - overlayRect.height;

expect(overlayRect.left.toFixed(2))
.toEqual(expectedLeft.toFixed(2),
expect(overlayRect.left)
.toBe(Math.round(expectedLeft),
`Expected menu to open in "before" position if "after" position wouldn't fit.`);

expect(overlayRect.top.toFixed(2))
.toEqual(expectedTop.toFixed(2),
expect(overlayRect.top)
.toBe(Math.round(expectedTop),
`Expected menu to open in "above" position if "below" position wouldn't fit.`);
});

Expand All @@ -236,14 +236,14 @@ describe('MdMenu', () => {

// 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.toFixed(2))
.toEqual(triggerRect.left.toFixed(2),
expect(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.toFixed(2))
.toEqual(triggerRect.top.toFixed(2),
expect(overlayRect.top)
.toBe(Math.round(triggerRect.top),
`Expected menu to open in "below" position if "above" position wouldn't fit.`);
});

Expand Down