Skip to content

fix(cdk/overlay): sub-pixel deviations throwing off positioning in some cases #21427

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

Merged
merged 1 commit into from
Jan 15, 2021
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
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {ComponentPortal, PortalModule} from '@angular/cdk/portal';
import {CdkScrollable, ScrollingModule, ViewportRuler} from '@angular/cdk/scrolling';
import {MockNgZone} from '@angular/cdk/testing/private';
import {dispatchFakeEvent, MockNgZone} from '@angular/cdk/testing/private';
import {Component, ElementRef, NgModule, NgZone} from '@angular/core';
import {inject, TestBed} from '@angular/core/testing';
import {fakeAsync, inject, TestBed, tick} from '@angular/core/testing';
import {Subscription} from 'rxjs';
import {map} from 'rxjs/operators';
import {
Expand Down Expand Up @@ -2194,7 +2194,51 @@ describe('FlexibleConnectedPositionStrategy', () => {
expect(Math.floor(overlayRect.width)).toBe(rightOffset);
});

it('should account for sub-pixel deviations in the size of the overlay', fakeAsync(() => {
originElement.style.top = '200px';
originElement.style.left = '200px';

positionStrategy
.withFlexibleDimensions()
.withPositions([{
originX: 'start',
originY: 'bottom',
overlayX: 'start',
overlayY: 'top'
}]);

attachOverlay({
positionStrategy,
height: '100%'
});

const originalGetBoundingClientRect = overlayRef.overlayElement.getBoundingClientRect;

// The browser may return a `ClientRect` with sub-pixel deviations if the screen is zoomed in.
// Since there's no way for us to zoom in the screen programmatically, we simulate the effect
// by patching `getBoundingClientRect` to return a slightly different value.
overlayRef.overlayElement.getBoundingClientRect = function() {
const clientRect = originalGetBoundingClientRect.apply(this);
const zoomOffset = 0.1;

return {
top: clientRect.top,
right: clientRect.right + zoomOffset,
bottom: clientRect.bottom + zoomOffset,
left: clientRect.left,
width: clientRect.width + zoomOffset,
height: clientRect.height + zoomOffset
} as any;
};

// Trigger a resize so that the overlay get repositioned from scratch
// and to have it use the patched `getBoundingClientRect`.
dispatchFakeEvent(window, 'resize');
tick(100); // The resize listener is usually debounced.

const overlayRect = originalGetBoundingClientRect.apply(overlayRef.overlayElement);
expect(Math.floor(overlayRect.top)).toBe(0);
}));

});

Expand Down
27 changes: 25 additions & 2 deletions src/cdk/overlay/position/flexible-connected-position-strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -525,9 +525,12 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
}

/** Gets how well an overlay at the given point will fit within the viewport. */
private _getOverlayFit(point: Point, overlay: ClientRect, viewport: ClientRect,
private _getOverlayFit(point: Point, rawOverlayRect: ClientRect, viewport: ClientRect,
position: ConnectedPosition): OverlayFit {

// Round the overlay rect when comparing against the
// viewport, because the viewport is always rounded.
const overlay = getRoundedBoundingClientRect(rawOverlayRect);
let {x, y} = point;
let offsetX = this._getOffset(position, 'x');
let offsetY = this._getOffset(position, 'y');
Expand Down Expand Up @@ -595,7 +598,7 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
* originPoint.
*/
private _pushOverlayOnScreen(start: Point,
overlay: ClientRect,
rawOverlayRect: ClientRect,
scrollPosition: ViewportScrollPosition): Point {
// If the position is locked and we've pushed the overlay already, reuse the previous push
// amount, rather than pushing it again. If we were to continue pushing, the element would
Expand All @@ -607,6 +610,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
};
}

// Round the overlay rect when comparing against the
// viewport, because the viewport is always rounded.
const overlay = getRoundedBoundingClientRect(rawOverlayRect);
const viewport = this._viewportRect;

// Determine how much the overlay goes outside the viewport on each
Expand Down Expand Up @@ -1219,3 +1225,20 @@ function getPixelValue(input: number|string|null|undefined): number|null {

return input || null;
}

/**
* Gets a version of an element's bounding `ClientRect` where all the values are rounded down to
* the nearest pixel. This allows us to account for the cases where there may be sub-pixel
* deviations in the `ClientRect` returned by the browser (e.g. when zoomed in with a percentage
* size, see #21350).
*/
function getRoundedBoundingClientRect(clientRect: ClientRect): ClientRect {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just rolls right off the tongue

return {
top: Math.floor(clientRect.top),
right: Math.floor(clientRect.right),
bottom: Math.floor(clientRect.bottom),
left: Math.floor(clientRect.left),
width: Math.floor(clientRect.width),
height: Math.floor(clientRect.height)
};
}