feat(overlay): add support for flexible connected positioning #9153

crisbeto · 2017-12-28T14:57:25Z

Adds the FlexibleConnectedPositionStrategy that builds on top of the ConnectedPositionStrategy adds the following:
- The ability to have overlays with flexible sizing.
- Being able to push overlays into the viewport if they don't fit.
- Having a margin between the overlay and the viewport edge.
- Being able to assign weights to the overlay positions.
Refactors the ConnectedPositionStrategy to use the FlexibleConnectedPositionStrategy in order to avoid breaking API changes.
Switches all of the components to the new position strategy.
Adds an API to the OverlayRef that allows for the consumer to wrap the pane in a div. This is a common requirement between the GlobalPositionStrategy and the FlexibleConnectedPositionStrategy, and it's easier to keep track of the elements when the attaching and detaching is done in the OverlayRef.

Fixes #6534.
Fixes #2725.
Fixes #5267.

isaacplmann · 2018-01-03T19:36:31Z

I'm excited for this to land! Thanks for the work @crisbeto.

jelbourn · 2018-01-03T01:09:53Z

src/cdk/overlay/_overlay.scss

+
+    // For connected-position overlays, we set `display: flex` in order to force `max-width`
+    // and `max-height` to take effect.
+    // todo: make sure this doesn't break existing stuff


Can this todo be removed?

jelbourn · 2018-01-11T21:51:22Z

src/cdk/overlay/overlay-directives.ts

-      .connectedTo(this.origin.elementRef, originPoint, overlayPoint)
-      .withOffsetX(this.offsetX)
-      .withOffsetY(this.offsetY);
+      .flexibleConnectedTo(this.origin.elementRef)


Add a comment that explains why these features are turned off?

jelbourn · 2018-01-11T21:59:49Z

src/cdk/overlay/overlay-ref.ts

@@ -27,6 +27,7 @@ export type ImmutableObject<T> = {
 */
 export class OverlayRef implements PortalOutlet {
  private _backdropElement: HTMLElement | null = null;
+  private _wrapper: HTMLElement | null;


Can you think of any uses for this wrapper element other than positioning?

It doesn't feel quite right that the OverlayRef has the responsibility for creating the wrapper, since the ref is inherently tied to a specific element at creation. I'm thinking that Overlay.create should create the element and we make it a fixed assumption going forward that the parentNode of the pane is this wrapping element. I'd also like to use a different term than wrapper; what do you think of "host"? If we decide that this really is only meant for positioning, then the structure would be

OverlayContainer holds all overlay content

Each overlay is inside of its own positioningHost

Each overlay still corresponds 1:1 with a panel element inside the positioning host

I'm open to different terms but can't think of a better one presently.

The wrapper is only used for positioning. Initially I wanted to keep it inside the position strategy (the same way we had it for the GlobalPositionStrategy), but it becomes tricky when we start detaching and re-attaching the same overlay. I made it opt-in to avoid changing the current behavior too much, but it should be fine since the wrapper won't do anything if it doesn't have any associated CSS.

jelbourn · 2018-01-11T22:00:40Z

src/cdk/overlay/overlay-ref.ts

@@ -51,6 +53,11 @@ export class OverlayRef implements PortalOutlet {
    return this._pane;
  }

+  /** Wrapper around the panel element. */
+  get overlayWrapper(): HTMLElement | null {


wrapperElement?

It would also be good if the API docs gave some indication as to what the wrapper should generally be used for.

jelbourn · 2018-01-11T22:39:20Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+// TODO: refactor clipping detection into a separate thing (part of scrolling module)
+// TODO: attribute selector to specify the transform-origin inside the overlay content
+// TODO: flexible position + centering doesn't work on IE11 (works on Edge).
+// TODO: doesn't handle both flexible width and height when it has to scroll along both axis.


Are these TODOs still relevant?

jelbourn · 2018-01-12T00:32:49Z

src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts

+
+/** Creates an absolutely positioned, display: block element with a default size. */
+function createPositionedBlockElement() {
+  let element = createBlockElement();


const here and in the other helper functions?

jelbourn · 2018-01-12T00:34:46Z

src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts

+
+      const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+      expect(Math.floor(overlayRect.height)).toBe(OVERLAY_HEIGHT - 10);
+      expect(Math.floor(overlayRect.bottom)).toBe(viewport.getViewportSize().height);


Should this test also check scrollHeight?

I don't think so, we're positioning relative to the viewport, not the page.

jelbourn · 2018-01-12T18:02:37Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+  private _resizeSubscription = Subscription.EMPTY;
+
+  /** Observable sequence of position changes. */
+  positionChange: Observable<ConnectedOverlayPositionChange> = this._positionChange.asObservable();


positionChanges?

crisbeto

Reworked it based on the feedback @jelbourn.

crisbeto · 2018-01-13T11:43:05Z

src/cdk/overlay/overlay-ref.ts

@@ -27,6 +27,7 @@ export type ImmutableObject<T> = {
 */
 export class OverlayRef implements PortalOutlet {
  private _backdropElement: HTMLElement | null = null;
+  private _wrapper: HTMLElement | null;


The wrapper is only used for positioning. Initially I wanted to keep it inside the position strategy (the same way we had it for the GlobalPositionStrategy), but it becomes tricky when we start detaching and re-attaching the same overlay. I made it opt-in to avoid changing the current behavior too much, but it should be fine since the wrapper won't do anything if it doesn't have any associated CSS.

mmalerba · 2018-01-16T22:46:00Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+    // If there are any positions where the overlay would fit with flexible dimensions, choose the
+    // one that has the greatest area available modified by the position's weight
+    if (flexibleFits.length) {
+      let bestFit: FlexibleFit | null = null;


just set to flexibleFits[0] initially? that way we don't need bestFit!

I'm not sure that it's the same functionality. If we did that, the first fit would have a score of -1 (unless we also moved out the score calculation) which would make it impossible to select.

you'd obviously need to set the initial score correctly as well

That means moving that long line outside the loop though.

mmalerba · 2018-01-16T22:46:36Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+    this._boundingBox = overlayRef.hostElement!;
+    this._pane = overlayRef.overlayElement;
+    this._resizeSubscription.unsubscribe();
+    this._resizeSubscription = this._viewportRuler.change().subscribe(() => this.apply());


nit/optional: could use takeUntil(this._detached)

We need to do it inside the disposed and I feel like introducing another Subject for this one case would be overkill.

mmalerba · 2018-01-16T22:48:09Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+  private _isInitialRender = true;
+
+  /** Last height used for the bounding box. Used to avoid resizing the overlay after open. */
+  private _lastBoundingBoxHeight = 0;


nit/optional: _lastBoundingBoxSize: {width: number, height: number} then you can directly do
= ...getBoundingClientRect()

mmalerba · 2018-01-16T22:49:24Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+  private _lastPosition: ConnectedPosition;
+
+  /** Subject that emits whenever the position changes. */
+  private _positionChanges = new Subject<ConnectedOverlayPositionChange>();


.complete() this in dispose

mmalerba · 2018-01-16T22:54:15Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+    if (this._canPush) {
+      // TODO(jelbourn): after pushing, the opening "direction" of the overlay might not make sense.
+      this._isPushed = true;
+      this._applyPosition(fallback!.position, fallback!.originPoint);


fallback actually could be undefined at this point, are we sure we want to just throw?

I don't think so. It should've been assigned here, unless we exited already https://github.com/angular/material2/pull/9153/files#diff-be0f6cfbf065a6175e266e5bc8cd5213R219.

It looks like its only set inside that for loop, so if this._perferredPositions == [] it would not be set

mmalerba · 2018-01-16T22:55:15Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+  /** Cleanup after the element gets destroyed. */
+  dispose() {
+    this._boundingBox = null;
+    this._resizeSubscription.unsubscribe();


I would call detach to make sure we do all detaching stuff, whatever that migth be

mmalerba · 2018-01-16T22:56:41Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+
+  /**
+   * Gets the (x, y) coordinate of a connection point on the origin based on a relative position.
+   * @param originRect


param descriptions (or just omit @param)

mmalerba · 2018-01-16T23:05:27Z

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+    let transformString = '';
+
+    if (position.offsetX) {
+      transformString += `translateX(${position.offsetX}px)`;


prefix with ' ' to prevent potential future bugs (the trim will just remove it anyways). or use an array and .join(' ')

crisbeto · 2018-01-17T18:13:54Z

Addressed the feedback @mmalerba @jelbourn.

jelbourn · 2018-01-17T18:23:04Z

I ran an early presubmit on this and it resulted in a fair number of failures. I'll try to look into them today to figure out if it's just their tests or something in the PR.

(I suspect it's their tests depending on a specific DOM structure)

crisbeto · 2018-01-17T18:52:23Z

FWIW, I was waiting for the final review to look into a couple of unit test failures on Safari, but IIRC they were only for the centered position + offset case which isn't super common.

jelbourn · 2018-01-17T23:23:43Z

Presubmit failure number 1: one app is somehow getting into a state where a call to apply is happening after the overlay is disposed (via some kind of async action). This manifests as _boundingBox being null when trying to read style from it.

Adding an _isDisposed property and aborting apply based on that seems to fix it

crisbeto · 2018-01-18T18:31:57Z

Sorted out the apply after dispose issue.

mmalerba · 2018-01-18T23:18:20Z

@crisbeto needs rebase

jelbourn · 2018-01-19T01:35:06Z

Summary of remaining failures:

Screenshot test failures where an autocomplete overlay is a few hundred pixels to the right of where it should be (multiple projects). We should double check the position config for autocomplete
Screenshot test failures where menus are about 10px to the left of where they should be (multiple projects)
Screenshot failures where the entire app UI is missing (not totally sure if this is related, needs more investigation)
Unit test failure where the innerText of the overlay container used to be something but now is empty (possibly a timing issue)
Unit test failure where overlayContainer.querySelector('.mat-menu-panel') is returning null when formerly it didn't (again might be due to timing)

I'll try to debug these tomorrow, but you might have ideas in the meantime.

crisbeto · 2018-01-19T20:19:41Z

The first two sound like something is definitely wrong on our end, but I'm not sure what it would be off the top of my head. As far as I remember the logic for a non-flexible overlay is more or less the same. I've also ported all the unit tests from the old positioning into the new one and they passed with zero changes.

jelbourn · 2018-01-19T23:47:51Z

Turns out that first one was actually a select. It was caused by the old version of select we have in Google (called mat-deprecated-select) that some teams are still using. This was leftover from when we switched select to be inside form-field. I was able to update the old code to resolve the issue (the _onAttached function just needed to be updated to match the current implementation)

jelbourn · 2018-01-20T01:18:12Z

So, for the menu being slightly off, I think I found what's wrong.

The case is where the menu-trigger is close to the right side of the screen.

The overlay-container is set to width: 100% rather than 100vw, with the difference being the width of the scrollbar. When we set right (on the bounding box), it's relative to the viewport without the scrollbar (width: 100%), but viewportRuler uses innerWidth, which includes the scrollbar.

We probably don't want to change overlay-container since it would likely affect downstream apps. We should be able to manually account for the scrollbar in the calculations. We could alternatively use body.getBoundingClientRect().right instead of the value we're currently getting from ViewportRuler (updating ViewportRuler would probably also be risky).

crisbeto · 2018-01-20T09:33:12Z

The 100% vs 100vw thing came up when we first introduced the bottom/right behavior, and if I remember correctly we tried making the container 100vw which ended up breaking a lot of other things. The workaround was to use the documentElement.clientWidth instead. We still have it when assigning the overlay pane styling, but we forgot to account for it when calculating the bounding box styles. I've rebased it and pushed a fix (it's only in the _getNarrowedViewportRect method if you don't want to re-review the whole thing).

jelbourn · 2018-01-22T18:26:02Z

Next failure: since the bounding box starts off with no size, innerText on the overlayContainer will now see nothing for its contents until it's positioned. It looks like just one app ran into this problem, so I can change their tests to use textContent, but if there's an easy way to make the box still "visible" for innerText it would be good to do. I think setting min-width: 1px; min-height: 1px should do it without affecting the rest of the logic.

crisbeto · 2018-01-22T20:32:22Z

Done @jelbourn.

isaacplmann · 2018-02-13T19:27:09Z

What's the status of this? There have been several releases since the last comment. Every time there's a release, I rush to check the changelog for this fix and it still isn't merged in. Are there technical concerns with the implementation?

* Adds the `FlexibleConnectedPositionStrategy` that builds on top of the `ConnectedPositionStrategy` adds the following: * The ability to have overlays with flexible sizing. * Being able to push overlays into the viewport if they don't fit. * Having a margin between the overlay and the viewport edge. * Being able to assign weights to the overlay positions. * Refactors the `ConnectedPositionStrategy` to use the `FlexibleConnectedPositionStrategy` in order to avoid breaking API changes. * Switches all of the components to the new position strategy. * Adds an API to the `OverlayRef` that allows for the consumer to wrap the pane in a div. This is a common requirement between the `GlobalPositionStrategy` and the `FlexibleConnectedPositionStrategy`, and it's easier to keep track of the elements when the attaching and detaching is done in the `OverlayRef`. Fixes angular#6534. Fixes angular#2725. Fixes angular#5267.

Something that was done initially in angular#9153, but ended up being re-added after a large rebase.

…r#9153) * Adds the `FlexibleConnectedPositionStrategy` that builds on top of the `ConnectedPositionStrategy` adds the following: * The ability to have overlays with flexible sizing. * Being able to push overlays into the viewport if they don't fit. * Having a margin between the overlay and the viewport edge. * Being able to assign weights to the overlay positions. * Refactors the `ConnectedPositionStrategy` to use the `FlexibleConnectedPositionStrategy` in order to avoid breaking API changes. * Switches all of the components to the new position strategy. * Adds an API to the `OverlayRef` that allows for the consumer to wrap the pane in a div. This is a common requirement between the `GlobalPositionStrategy` and the `FlexibleConnectedPositionStrategy`, and it's easier to keep track of the elements when the attaching and detaching is done in the `OverlayRef`. Fixes angular#6534. Fixes angular#2725. Fixes angular#5267.

Something that was done initially in #9153, but ended up being re-added after a large rebase.

angular-automatic-lock-bot · 2019-09-08T12:03:07Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

crisbeto added the pr: needs review label Dec 28, 2017

crisbeto assigned jelbourn Dec 28, 2017

crisbeto requested review from andrewseguin, jelbourn, kara and mmalerba as code owners December 28, 2017 14:57

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2017

crisbeto force-pushed the betterer-connected-position branch from 4b04504 to 578f5f4 Compare December 28, 2017 15:14

crisbeto mentioned this pull request Jan 7, 2018

[Tooltip] Overlay gets cut off by left of screen #7126

Closed

willshowell mentioned this pull request Jan 8, 2018

CDK: ConnectedPositionStrategy not open for extension #9206

Open

jelbourn reviewed Jan 12, 2018

View reviewed changes

crisbeto force-pushed the betterer-connected-position branch from 578f5f4 to d70be08 Compare January 13, 2018 12:14

crisbeto commented Jan 13, 2018

View reviewed changes

crisbeto force-pushed the betterer-connected-position branch from d70be08 to ffe7f09 Compare January 13, 2018 12:22

mmalerba reviewed Jan 16, 2018

View reviewed changes

crisbeto force-pushed the betterer-connected-position branch from ffe7f09 to 1b61bd8 Compare January 17, 2018 18:13

crisbeto force-pushed the betterer-connected-position branch from 1b61bd8 to 541684a Compare January 18, 2018 18:31

mmalerba added pr: lgtm and removed pr: needs review labels Jan 18, 2018

crisbeto force-pushed the betterer-connected-position branch from 541684a to 3726d81 Compare January 20, 2018 09:28

crisbeto removed the pr: needs rebase label Jan 20, 2018

crisbeto force-pushed the betterer-connected-position branch from 3726d81 to 63deb95 Compare January 22, 2018 20:32

josephperrott added the P2 The issue is important to a large percentage of users, with a workaround label Mar 7, 2018

crisbeto force-pushed the betterer-connected-position branch from 63deb95 to 65b76cb Compare March 8, 2018 03:34

crisbeto force-pushed the betterer-connected-position branch from 65b76cb to 3a9965c Compare March 14, 2018 23:10

mmalerba merged commit 27e5f6e into angular:master Mar 14, 2018

willshowell mentioned this pull request Mar 15, 2018

refactor: Leverage FlexibleConnectedPositionStrategy ncstate-sat/popover#112

Closed

crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 16, 2018

chore(select): disable position locking

3b4e467

Something that was done initially in angular#9153, but ended up being re-added after a large rebase.

crisbeto mentioned this pull request Mar 16, 2018

chore(select): disable position locking #10457

Closed

andrewseguin pushed a commit that referenced this pull request Mar 20, 2018

chore(select): disable position locking

0d4682a

Something that was done initially in #9153, but ended up being re-added after a large rebase.

benb7760 mentioned this pull request Apr 9, 2018

Tooltip with longer text is cut when displayed on control positioned next to browser window border #10769

Closed

julianobrasil mentioned this pull request May 30, 2018

bug(Datepicker): Calendar's position is being switched to upside even when there's no space to fit it #6965

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

feat(overlay): add support for flexible connected positioning #9153

feat(overlay): add support for flexible connected positioning #9153

Conversation

crisbeto commented Dec 28, 2017

isaacplmann commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jan 17, 2018

jelbourn commented Jan 17, 2018

crisbeto commented Jan 17, 2018

jelbourn commented Jan 17, 2018

crisbeto commented Jan 18, 2018

mmalerba commented Jan 18, 2018

jelbourn commented Jan 19, 2018

crisbeto commented Jan 19, 2018

jelbourn commented Jan 19, 2018

jelbourn commented Jan 20, 2018

crisbeto commented Jan 20, 2018

jelbourn commented Jan 22, 2018

crisbeto commented Jan 22, 2018

isaacplmann commented Feb 13, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019