feat(modifier): detectOverflow implementation

[FezVrasta]

Still very work in progress, but since I'm stuck with this I'm sharing my progress hoping someone can help.

The idea is to provide a list of directions (top/right/bottom/left) set to either true (the popper is overflowing) or false (it's not).

This data is stored into a new modifiersData state object, which is going to be readable by other modifiers to leverage the data exposed by it.

I think it would make sense to expose a stricter contract to disallow extensions from writing to other modifiers namespaces, but for now this gets the job done.

[atomiks]

I have a couple questions:

• So this doesn't actually prevent the overflow, just detects it. Which modifier will prevent the overflow?
• Which part are you stuck on?

[FezVrasta]

Yes this will just detect it, I haven't yet worked on the actual modifier that will alter the computed offsets. (but it should be trivial once this one works properly)

The logic to detect the overflow is not stable, if you open the visual test cases you will notice there are a lot not working.

[mykas]

Hey @FezVrasta , can you explain a bit how the visual test cases are not working with overflow? I can't seem to find it or don't know where to look. I have cloned, checked out to this branch and opened the visual page.

[FezVrasta]

@mykas please add a console.log(state.modifiersData.detectOverflow) at line 42 of modifiers/detectOverflow.js.

Then open the /scrolling/offset-parent visual test, you will notice the bottom property switches from true to false improperly.

There are a lot of other cases like this.

[inomdzhon]

It is should work like this?

[FezVrasta]

@inomdzhon I can't quite understand where a container ends and the other begins from your screenshots, but from what I can see it seems to work correctly in that case.

[inomdzhon]

Sorry for bad example 😞
Ok, if so, then we should compute popper offsets relative closest scroll container.
I will try to implement it soon.

[FezVrasta]

Just a heads up, if you don't know how Flow works, feel free to ignore it and send PRs with broken type checking, I will take care of it.

[inomdzhon]

Actually, I write in TS so there will be no problem, thanks. Nevertheless, maybe I ignore it 🤔

[atomiks]

The WIP implementation has the overflow detection based on the documentRect but I am guessing that it should be based on the scrollParent(s) as mentioned in @inomdzhon's comment, right?

@FezVrasta have you got any rough ideas about what needs to be taken into account to get this to work? I'm guessing you've experimented with this a lot already. I'm assuming the scroll positions of each of the scroll containers is one aspect. I'm currently lost conceptually in what parts need to fit together to get this to work. A deep explanation of the problem and what needs to be achieved would be really helpful.

@inomdzhon what ideas have you experimented with so far?

[FezVrasta]

Basically, the modifier should calculate all the offsets from the popper up to its scrolling container (common to the reference element), and then from that container, down to the reference element.

Once you have these two measures, you should be able to detect if the popper is overflowing the container.

It's been a while since the last time I touched this code so I may easily saying bullshit, sorry 😥

[atomiks]

Is handling nested scroll containers the main difficulty? If so, maybe only handling one is good enough (same as v1), theoretically handling infinitely nested containers is great but it's pretty rare in real UIs, so if it's blocking this it's probably not worth it... assuming that is the reason of course.

Or is it the difficulty in converting the "write" modifier to this "read" version with new architecture? 🤔

[FezVrasta]

The multi scrolling container is the main issue, but I would love to have that.

[atomiks]

Is it possible to work on in a feat release after v2? I don't see a reason it should block since it doesn't exist in the lib yet anyway.

[FezVrasta]

AFAIK there's some basic support for that.

[inomdzhon]

@atomiks sure, it should based on the scrollParent.

For example, we have two scroll parents. We should detect the popper is overflowing relative the closest scroll parent, if it is don't, we should go to the upper scroll parent, but now we should consider the previous scroll parent.

Until I didn't start implementation 😩

[FezVrasta]

I'm experimenting with an alternative approach to the problem, I'm using HTMLElement#getBoundingClientRect() to detect the position of the boundaryElement and of the referenceElement.

I use the referenceElement ClientRect to compute the future popperElement ClientRect values (since we haven't yet altered the DOM at this stage), once we have this, we can do something very simple such as:

  state.modifiersData.detectOverflow = {
    top: boundaryClientRect.top > popperClientRect.top,
    bottom: boundaryClientRect.bottom < popperClientRect.bottom,
    left: boundaryClientRect.left > popperClientRect.left,
    right: boundaryClientRect.right > popperClientRect.right,
  };

to detect the overflow state of each side.

I'd love to have your feedback about this, do you see any obvious problem with this approach?

[atomiks]

Think there's a typo:

- right: boundaryClientRect.right > popperClientRect.right,
+ right: boundaryClientRect.right < popperClientRect.right,

That said, it seems... like a good solution on first intuition? I think I use the same thing in tippy.js for interactivity to see if the cursor left the popper - you don't need to take into account scrolls/nesting since as just "works" based on client coords. I think this concept is similar.

I think I was getting confused with the offsets the popper uses to position itself (which needs to take all of that complexity into account) with detecting "intersection" (which is based on ClientRects), in attempting to solve this problem 😅

[FezVrasta]

Thanks, I fixed the typo.

I pushed a commit to fix the only remaining issue I was experiencing in the scrolling/offset-parent visual test I used to write this solution. Unfortunately it doesn't work, but I don't have any more bandwidth to allocate to this PR for now.

If anyone wants to fix it it'd be great. Also, if someone wants to test this implementation on all the other visual tests and report issues that'd be very helpful.

@atomiks, the positioning logic is already working on this branch, and from what I could see it's very solid.

[atomiks]

Unfortunately it doesn't work

Not sure what you mean, what remaining issue? The margin calculation seems to work from briefly testing it

[FezVrasta]

The bottom boolean is set to true after ~8 pixels more than it should.

[atomiks]

Hmm should it be like

bottom: boundaryClientRect.bottom + boundaryMargins.bottom,

But why is the boundary margin being taken into account anyway?

[FezVrasta]

If you'd like to send a PR targeting this branch please do so 🤗

[atomiks]

I'm not sure I understand the purpose of subtracting the boundary margin in the first place. When I remove the margins, the boolean switches to true correctly (I think, not sure what behavior you're looking for). I added margins to the popper and it switches correctly as well.

I think the margins added to getElementClientRect are correct, you just need to remove the subtraction for the boundary inside the modifier?

• No boundary margin = switches to true right as it hits the bottom of the rect (as expected - the margin pushes it away from the html element)
• Add the boundary margin = switches to true when hitting the bottom ignoring the margin (i.e. the html rather than body..)

[FezVrasta]

I'm not sure, I'm too tired probably 😅 If you can get it to work it's okay to me, I'm not even sure if the margin thing is needed.

[inomdzhon]

I'd love to have your feedback about this, do you see any obvious problem with this approach?

I know one "problem" – node.offsetTop and other same properties guaranteed that number will be integer, on other hand node.getBoundingClientRect() doesn't.

[FezVrasta]

@inomdzhon that's a good point, I haven't clear in my head the implications of this though. 🤔

[FezVrasta]

I changed the default boundaryElement to be documentElement, it's a bit misleading, because actually what it means is that the "viewport" will be used, but I think the fact documentElement can be smaller than the viewport is something very confusing that a lot of people don't fully understand, so this approach should avoid a lot of troubles.

[atomiks]

So the default boundary will no longer be "scrollParent"?

[FezVrasta]

🤦‍♂ yeah right...

About scrollParent, the v1 behavior is quite bad, because not all the scroll parents clip the overflowing content. A better behavior would be to default to a clippingScrollParent.

At the moment I don't remember what qualifies one, but it should be something like overflow !== visible + position !== initial, but I need to check my notes (not really, I don't keep notes....)

edit: here's a few example of clipping containers, I think this kind of containers are the one that should be used by default.

https://twitter.com/FezVrasta/status/1197515785289252864

[FezVrasta]

I was thinking that rather than reporting true|false we could report the offset between the boundary and the popper, it would be a number > 0 if the popper is overflowing. This would allow more flexibility, for example, one could make a modifier that flips the popper only if it's overflowing more than 10px its boundaries.

[FezVrasta]

ok folks, I'm merging this PR.

[atomiks]

Does clippingParent fix this case in CodeSandbox UI?: https://codesandbox.io/s/1vzvoo9mwl (ignore the actual sandbox)

Hover over "Clear Console" icon down the bottom. Notice that the tippy overlaps due to the fact that it's in a scrolling parent. This is a common bug that people kinda struggle to fix (fixed with boundary: "window").

Maybe documentElement is a better default?