feat(overlay): CoverPositionStrategy #15691

kseamon · 2019-04-02T18:30:39Z

Adds a new position strategy that allows attaching to the edges of one or more origins while supporting a subset of the functionality in FlexibleConnectedPositionStrategy.

This will be the default positioning strategy for the upcoming MatPopoverEdit.

… or bring back.

jelbourn · 2019-04-02T18:36:09Z

src/cdk/overlay/position/cover-position-strategy-factory.ts

+import {FlexibleConnectedPositionStrategyOrigin} from './flexible-positioning';
+import {CoverPositionStrategy} from './cover-position-strategy';
+
+@Injectable({providedIn: 'root'})


Add JsDoc description

I'd also add a file-level comment that explains that this class exists so that people can instantiate a CoverPositionStrategy without manually passing in its numerous injected deps

Will do - IMO all of the factories for position strategies should move to something like this rather than baking them into Overlay (and defeating tree-shaking of them in the process).

jelbourn · 2019-04-02T18:37:38Z

src/cdk/overlay/position/cover-position-strategy.ts

+ * implicit position relative some origin element. The relative position is defined in terms of
+ * a point on the origin element that is connected to a point on the overlay element. For example,
+ * a basic dropdown is connecting the bottom-left corner of the origin to the top-left corner
+ * of the overlay.


Could you include the word "cover" somewhere in this description? I'd expect to see something like

Strategy for positioning an overlay such that it covers one or more other elements. You can define the covered region by...

jelbourn · 2019-04-02T18:40:39Z

src/cdk/overlay/position/cover-position-strategy.ts

+  private _isInitialRender: boolean;
+
+  /** Whether the overlay can be pushed on-screen on the initial open. */
+  private _canPush = true;


Is the pushing behavior desirable for the cover strategy? I could see the rationale for not doing it.

It seems reasonable to me - basically it lets you push off of screen edges but in concert with the flexible flag lets you decide whether to do just moving or also shrinking.

jelbourn · 2019-04-02T18:41:43Z

src/cdk/overlay/position/cover-position-strategy.ts

+  private _canPush = true;
+
+  /** Whether the overlay can grow via flexible width/height after the initial open. */
+  private _growAfterOpen = false;


Does growing make sense for the cover strategy? We added this feature to the connected strategy for cases like menu where you may want the overlay to grow based on the content, but I don't really know what that would be for cover.

Note that when you don't anchor all four sides, what flows out can grow freely based on the content.

For instance, in the table edit I'll be anchoring on the top, left, and right sides but leaving the bottom unbounded so that the popup grows as tall as needed to fit the lens content.

jelbourn · 2019-04-02T18:45:00Z

src/cdk/overlay/position/cover-position-strategy.ts

+  private _lastBoundingBox: Partial<BoundingBoxRect>|null;
+
+  /** The last computed bounding box adjustments. */
+  private _lastAdjustments: BoundingBox|null;


It's not fully clear from the name what "adjustments" means. I would expect something like _last____Offset, where the blank is whatever the offset is necessary for (like lastScrollOffset or lastResizeOffset etc.)

crisbeto

I haven't gone too much in-depth with the logic, but here are some concerns after a first pass:

It seems like this has a lot of the logic from the FlexibleConnectedPositionStrategy. This logic is a little convoluted in places and having to maintain it in two places would be a lot.
I'm not sure that we can always guarantee that the overlay is going to cover the trigger. E.g. there's no performant way to react if the element's size changes as a result of its content changing.
From what I can tell, this position strategy is catered for only one component and we should be able to achieve the same with the FlexibleConnectedPositionStrategy. Here's some pseudo code that's based on MatAutocompleteTrigger that would cover the trigger with a connected overlay:

const overlayRef = overlay.create({
  positionStrategy: overlay.position()
    .flexibleConnectedTo(triggerEl)
    .withPositions([{
      originX: 'start',
      originY: 'top',
      overlayX: 'start',
      overlayY: 'top'
    }])
});

merge(scrollDispatcher.scrolled(), viewportRuler.change()).pipe(startWith(null)).subscribe(() => {
  const triggerRect = triggerEl.getBoundingClientRect();
  
  overlayRef.updateSize({
    width: triggerRect.width, 
    height: triggerRect.height
  });
});

crisbeto · 2019-04-02T18:35:16Z

src/cdk/overlay/position/cover-position-strategy-factory.ts

+import {CoverPositionStrategy} from './cover-position-strategy';
+
+@Injectable({providedIn: 'root'})
+export class CoverPositionStrategyFactory {


Why do we need a dedicated factory for it? All of the other position strategies are created through the OverlayPositionBuilder.

IMO all of the strategies should move towards a separate factory approach for better tree shakability. In this case, I did not want to add something new that would not be tree-shaken away by default.

I've had that on my list for a while, but the problem is that we can't do it easily without either a breaking change or having to maintain two approaches for a while. It would also be tricky to migrate people once we remove the deprecated approach. Having only this strategy with a dedicated factory is inconsistent.

Probably you'd add individual factories, have the existing combined factory use them for now, mark it as deprecated and remove it after a couple releases? I'll defer to whatever Jeremy prefers though.

kseamon · 2019-04-02T19:16:02Z

There is a lot in common with FlexibleConnectedPositionStrategy. I've pulled out some parts to common utility functions but a lot of the particulars of positioning are different. I could pull some more parts into a common superclass, but I wasn't sure if you all would care for that.
That's probably true until resize observers are a thing.
Covering one element (left to right but not top to bottom) is the common case for the MatPopoverEdit but it also needs the ability to span across multiple table cells in the same row. Also, it should anchor to the top but grow freely from the bottom and be able to push and shrink if the window is too small. If there's strong opposition to adding a new strategy for general use, I could strip this down to just that set of inputs which would probably simplify it a decent amount.

crisbeto · 2019-04-02T19:21:02Z

We can deal with 1 and 2 later, but my concern with 3 is that I don't see very many cases, aside from MatPopoverEdit, where a position strategy like this would be used. Alternatively, we can see how far we can get using the connected position strategy plus some custom logic inside MatPopoverEdit to match the width/height and then if another use-case comes along or somebody requests it, we can pull it out into a position strategy. WDYT @jelbourn?

jelbourn · 2019-04-02T19:56:12Z

In general, my preference would be to introduce fewer new APIs, so if there is a way to accomplish the same thing with the connected strategy that would be nice. That said, I don't have a clear idea of what the actual differences are between the two strategies (without really diving into the code, which will take a while). @kseamon could you give a brief breakdown of the differences?

I also wouldn't mind if the cover strategy has a very niche use case. If we don't include it in OverlayPositionBuilder, it shouldn't be included unless someone is using it.

As for the CoverPositionStrategyFactory- it's not exactly my favorite API, but I didn't have a better alternative. Having people do new CoverPositionStrategy with like 8 different injected args just sucks.

kseamon · 2019-04-02T20:09:55Z

The gist of the differences is that this one is geared around anchoring to multiple edges of one or more elements while still being flexible (eg able to nudge or shrink based on the constraints of the viewport).

I'd need to do a quick prototype with crisbeto's suggestion to see if it covers my use case for the table edit (it looks like it could be made to span properly if enough logic was added to the popover edit directive but it's not clear to me that the nudging/shrinking would work as desired).

Also it makes the popover edit less flexible as we'd no longer be able to specify custom positioning strategies for it (and this strategy would not be extractable from it). I think for the customizing reason alone I'd prefer to at least keep a subset of this positioning strategy implemented that's needed for popover edit and if need be have it live in the popover edit directory rather than overlay/position.

In either case, this presents a much easier api for the "covering" use-case. I guess the question for whether it belongs in cdk/overlay is if it's a broadly desirable use-case.

kseamon · 2019-04-03T15:08:47Z

Tried out @crisbeto's suggestion. It works pretty well, although flexible dimensions don't seem to work (instead of shrinking, the overlay vanishes when the viewport narrows too much). Any suggestions on that?

I can probably move forward with this as the default strategy for popover edit for now (I think I can resolve the spanning multiple cells issue with some more work), but I'll keep this branch around in case I do end up needing something more versatile.

crisbeto · 2019-04-03T20:46:14Z

If I remember correctly, we had some issues a while ago where flexible positioning wasn't working very well when it has to happen on two axis. It hasn't really been a problem since with the current overlays we only need it to flex in one direction, but it's possible that it's what you're hitting.

jelbourn · 2020-10-01T21:25:50Z

It sounds like we had a solution for the underlying problem without this new strategy, so going to close the PR for now. Let me know if we need to revisit.

angular-automatic-lock-bot · 2020-11-01T00:08:06Z

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.}

kseamon added 11 commits March 21, 2019 16:46

feat(overlay): Add CoverPositionStrategy

e422f3b

almost builds

407c641

it now works at a very basic level

58627de

A decent amount of stuff passes now.

cca144b

right about to switch stuff up

9189217

At a steady state before we try adding persistent height into the mix

6da9ab7

wip

382f330

Getting close - now looking into what other features I need to delete…

f43e32c

… or bring back.

really close now

984eec9

Much cleanup - and stuff works!

e3134d3

more cleansing

8432c25

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 2, 2019

jelbourn requested a review from crisbeto April 2, 2019 18:31

jelbourn reviewed Apr 2, 2019

View reviewed changes

crisbeto reviewed Apr 2, 2019

View reviewed changes

kseamon mentioned this pull request Apr 4, 2019

feat(popover-edit): Adds support for spanning multiple columns and se… #15724

Merged

jelbourn added ༼ つ ◕_◕ ༽つ and removed ༼ つ ◕_◕ ༽つ labels Apr 25, 2019

mmalerba added aaa and removed aaa labels Apr 25, 2019

jelbourn closed this Oct 1, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 1, 2020

feat(overlay): CoverPositionStrategy #15691

feat(overlay): CoverPositionStrategy #15691

Uh oh!

Conversation

kseamon commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kseamon Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kseamon commented Apr 2, 2019

Uh oh!

crisbeto commented Apr 2, 2019

Uh oh!

jelbourn commented Apr 2, 2019

Uh oh!

kseamon commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kseamon commented Apr 3, 2019

Uh oh!

crisbeto commented Apr 3, 2019

Uh oh!

jelbourn commented Oct 1, 2020

Uh oh!

angular-automatic-lock-bot bot commented Nov 1, 2020

Uh oh!

Uh oh!

kseamon Apr 2, 2019 •

edited

Loading

crisbeto Apr 2, 2019 •

edited

Loading

kseamon commented Apr 2, 2019 •

edited

Loading