bug(overlay): Push calculation is wrong with FlexibleConnectedPositionStrategy

[csutorasr]

Bug description

Push calculation is wrong in this line. Maybe in the next lines also, but I did not check it yet.

I tried to add an element, that perfectly fits, but I could not. I realized that the double of the original margin is used on the right side. I used LTR.

Input

Take a document, that has 400px width. The margin is 20px on the viewport of the overlay. I tried to add one overlay at (300, 20) with size of (80,40). I will use this in the stackblitz and in the calculations.

Reproduction

https://stackblitz.com/edit/angular-ardamx

As you see the overlay is positioned well at the begining. 100px is the container. 80px is the overlay. 10-80-10 is well positioned.

If the body element is 420px width, the position is right. 0-80-20.

If the body is shrinked more to 400px, the position will not be 0-80-20, however it could fit.

Expected Behavior

pushX to be 0 in this case. The element should fit without pushing it. The margin should be 20px on the right, too.

Actual Behavior

start.x is 300, the width is 80 as these are given. The viewport width is 360px (=400px-2*20px).

pushX will be -20 as overflowRight is 20. This is calculated by 300+80-360=20.

This is wrong. The element can fit without pushing it. The margin is now the double of the originally given, which is 40px in this case.

Environment

• Angular: 10.1.3
• CDK/Material: 10.2.5
• Browser(s): Chrome (Could be any)
• Operating System (e.g. Windows, macOS, Ubuntu): macOS (Could be any)

[csutorasr]

This should work. As the width contains the margin 2 times.

const overflowRight = Math.max(start.x + overlay.width - viewport.width - this._viewportMargin, 0);
const overflowBottom = Math.max(start.y + overlay.height - viewport.height - this._viewportMargin, 0);

However I found that, the vertical push is not working until horizontal push appears.

I have made some modifications on the stackblitz.

The code now is:

const overflowTop = Math.max(viewport.top - scrollPosition.top - start.y, 0);
const overflowLeft = Math.max(viewport.left - scrollPosition.left - start.x, 0);

{
  top:    scrollPosition.top + this._viewportMargin,
  left:   scrollPosition.left + this._viewportMargin,
  right:  scrollPosition.left + width - this._viewportMargin,
  bottom: scrollPosition.top + height - this._viewportMargin,
  width:  width  - (2 * this._viewportMargin),
  height: height - (2 * this._viewportMargin),
}

So, the overflowTop is this._viewportMargin - start.y. Same goes for Left.

start is a global coordinate. The scroll position should not be removed from viewport, as making it local to the screen. The code should be the following:

const overflowTop = Math.max(viewport.top - start.y, 0);
const overflowLeft = Math.max(viewport.left - start.x, 0);

The margin is added one here in top and left. That's why it should work correctly.

May I create a pull request or two for this? I will make the changes if you approve.

P. S.
It would be a good first issue for me. This would be my first contribution to @angular/components.

[csutorasr]

if (overlay.width <= viewport.width) {
  pushX = overflowLeft || -overflowRight;
} else {
  pushX = start.x < this._viewportMargin ? (viewport.left - scrollPosition.left) - start.x : 0;
}

if (overlay.height <= viewport.height) {
  pushY = overflowTop || -overflowBottom;
} else {
  pushY = start.y < this._viewportMargin ? (viewport.top - scrollPosition.top) - start.y : 0;
}

At this part the (viewport.top - scrollPosition.top) can be simplified as this._viewportMargin.

I cannot find it out why it is not pushed vertically, when there is no horizontal push. That could be a new pull request.

[csutorasr]

I also found the problem for the pushing vertically. _pushOverlayOnScreen is not called so the problem was not there. The reason is this._isPushed was not true. It is set when the overlay not fits.

I have found that the _getOverlayFit function does not take the margins and the scroll into count.

The code should be:

let leftOverflow = viewport.left - x;
let rightOverflow = (x + overlay.width) - (viewport.left + viewport.width);
let topOverflow = viewport.top - y;
let bottomOverflow = (y + overlay.height) - (viewport.top + viewport.height);

Viewport contains the margin and the currect scroll state. This should work.