fix(positioning): correctly consider the viewport

[burnedram]

Fixes #1898, fixes #1876.
I'm not sure about #1896, as I have not tested this specific use case.

[pkozlowski-opensource]

@burnedram thnx!
@mschoudry would you mind reviewing?

[mschoudry]

@burnedram I was not able to follow the main issue you are targeting. I have tested your PR and it does not seems to be resolving the issue you demonstrated in plunker. Or is it that I misunderstood something?
Regards,

[burnedram]

The issue I am trying to solve is how "auto" placement is resolved. As seen in the plunkr, the button labelled "auto" (which has the attribute "placement='auto'") is placed above the button, even though there is not enough (horizontal) space for the popover.

Another bug can be seen in the plunkr as well. The "left" button has a proper popover, because there is enough horizontal and vertical space. "top" and "bottom" however gets squished by the browser as it is too close to the edge of the viewport. It seems because the popover gets squished, the calculations done in order to horizontally center the popover is no longer correct. It is not my intent to fix this bug, as I don't really know how one should react to there not being enough space in a position strictly chosen by the user.

Edit: Clarification: When the popover is squished both horizontal and vertical alignment is incorrect in the case of "top", exaggerating the problem.

However, one can mitigate the centering bug by using "auto", but as I've described here, the "auto" algorithm is broken.

[burnedram]

Here is an example of what my pull requests fixes.

Current ng-bootstrap, popover is (almost) always positioned above.
With this pull request applied, popover is positioned where there is enough space.

Edit: position is "auto" in both GIFs

[burnedram]

@pkozlowski-opensource @mschoudry

Sorry for bumping, just wondering if there is anything else I should change or clarify 😃

[mschoudry]

@burnedram : thnx for the updates
I understand now, so you are basically targeting the issue when the popup width is bigger than the host element width. Which is not handled in the case of top/bottom and should have been handled here and here .

wouldn't it be better to resolve only those, rather than refactoring everything?

Regards,

[burnedram]

@mschoudry If you meant "when the popup width is bigger than the viewport width", then yes, that is the issue I'm trying to solve. (By "width" I mean both horizontal position and width)

The reasoning behind refactoring the code was to improve readability. I have essentially moved all conditions inside if-statements and given them a name. This also has a bonus in that if-statements that share conditions doesn't need code repetition, which reduces the chance of missing to implement a change in all if-statements.

Now each placement's reasoning can be understood by reading the code, instead of having to think about what all the expressions means.

I also opted to remove/rewrite/relocate the setSecondaryPlacementFor as it was broken.

Ofcourse, the bug could probably be solved by just changing the code around those two lines, but I can't help to think that the bug occured due to the code being difficult to follow.

[burnedram]

I realise that refactoring isn't always the best option in open-source projects, as it might break other pull requests and general code history, and it is of course up to you if you believe it is acceptable in this case.

[pkozlowski-opensource]

@burnedram seems like some of the issues referenced in this PR are already fixed:

• Typeahead availability of placement 'bottom-left' or 'top-left' is dismissed by height, not width #1876 was fixed by 0df398c
• ngbTooltip placement issue when inside other text #1896 is confirmed as fixed in the latest version: http://plnkr.co/edit/3dUpcB0IWH9wBEBUaAEX?p=preview

Could you please:

• check if your fix is still relevant / valid?
• rebase your PR?

Thnx!

[bluecaret]

What's the status on this? Still waiting for this to make it in to fix #1898. This is crucial for my app.

[bluecaret]

Anyone still working on this? This is still very much a bug.

[burnedram]

@pkozlowski-opensource
I have update the issue, and also rebased the pull-request.
I decided to forgo the entire rewrite of the affected function, and instead only add the missing checks.

I also changed "auto" to include all secondary placements, as there are situations where no primary placement is available. I also think that "auto" should consider all possible placements.

New plnkr that shows how broken "auto" is
With this pull request, all popovers behaves nicely

[burnedram]

Note on the failed CI build: master is failing
Seems to be because of saucelabs.

I've run the tests locally, and they passed (except for two relating to "ngbFocusTrap" and "ngb-typeahead", which also fails on master)

[burnedram]

@pkozlowski-opensource
Rebased onto lastest master commit, now tests are okay

[bluecaret]

@pkozlowski-opensource Any chance this can be added to the next release? Seems like it's been fixed for a while now...

[pkozlowski-opensource]

Merged. Thnx @burnedram for the PR and rebasing it, much appreciated!