[AutoComplete] Popover top value inconsistently calculated

[aweary]

This <AutoComplete> instance is inside a <ToolbarGroup>. The top property seems to either be rendered at 70 or 0, depending on its mood I guess.

This is with the latest 0.14.0 release.

[aweary]

It seems to be an issue with how the values from getTargetPosition are calculated and/or parsed in applyAutoPositionIfNeeded in the <Popover/> component.

In the instances where the offset is correct (e.i, not overlapping), getTargetPosition is returning an object that looks like:

{
bottom: 0
center: 0
left: 0
middle: 0
right: 0
top: 0
}

and setPlacement calculates targetPosition as {top: 70, left: 1224}. These values are passed to applyAutoPositionIfNeeded which uses the following logic to check if the top value should be updated:

 if (targetPosition.top < 0 || targetPosition.top + target.bottom > window.innerHeight) {
     // Update logic and stuff
    }

Since targetPosition is 70 and target.bottom is 0 it doesn't recalculate the top value and it renders fine.

But, in the instances where it overlaps, getTargetPosition is returning:

{
bottom: 879
center: 439.5
left: 0
middle: 128
right: 256
top: 0
}

Where bottom is actually the value of window.innerHeight (if you resize the window, the bottom value change accordingly on the next render)

So since the window.innerHeight + 70 is always greater than window.innerHeight its recalculating the top value, which is returning a negative value, and the Math.max(0, newTop) call is returning 0 instead of the 70.

[aweary]

This issue is, of course, resolved if <AutoComplete> sets the canAutoPosition prop on the <Popover> to false

[alitaheri]

This is a very odd and moody bug. Thanks for the detailed report 👍 👍 We'll look into it. although a PR is also welcome 😁 😁

[aweary]

Do you think setting canAutoPosition to false would be acceptable for the <AutoComplete> component, or would that have unexpected consequences? If that would work, I'd be happy to get a PR going now. Otherwise it may take a little while.

[alitaheri]

I'm not sure. I wan't around when this component was being written. @oliviertassinari Can you take the lead on this one?

[oliviertassinari]

@chrismcv Should be the best in place to answers this 😁

[jasonchoimtt]

Hi, I want to point out a related problem. When the keyboard is open on mobile (i.e. screen height is limited), the Popover will also cover the input.

Setting canAutoPosition to false will fix this, but will also cause part of the Popover to go off the screen, since the maxHeight of the Popover content is still determined by window.innerHeight. We might need another way to constrain the height of the Popover content.

[aweary]

@oliviertassinari @chrismcv @alitaheri any word on this?

[aweary]

@oliviertassinari @chrismcv @alitaheri sorry to keep pestering you on this, but we'd like to get this taken care of before pushing our project to production. Can I can an update here? Does my proposed fix sound reasonable?

[alitaheri]

@aweary I'd say make that PR for now so we can evaluate the effects. it it's a breaking change we'll ease into it. otherwise we'll merge. Either way, you can always use your own fork for the production. at least for now.

[aweary]

Thanks @alitaheri I'll get a PR going. Forking would have been a last ditch option, but we'd rather support the project then fork it. Thanks for the response.

[alitaheri]

Your approach is appreciated 👍 Thanks for the PR 😁

[nathanmarks]

fixed by #3620