Tooltip/popover: Fix auto placement to use viewport #16152
Conversation
Currently, auto placement is using the container dimensions (if provided) or the element's parent to determine where to open the tooltip: ```javascript var $container = this.options.container ? $(this.options.container) : this.$element.parent() var containerDim = this.getPosition($container) ``` This is quite broken in fact, because the parent element could be just a small div outside the element for example, leading in a totally random placement (placing the tooltip on top even if there's no room). And the container can also be outside of the viewport. This fix simply uses the viewport instead, that's the purpose of the viewport actually, to position the tooltip. So the auto placement should use it to find where there's more room. By default this is body, which is good.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0e8e522 (Please note that this is a fully automated comment.) |
|
If you could add a unit test to prevent future regressions, that would be greatly appreciated. |
|
Yep, I'll try fixing the sauce labs failing tests and add a new one ;) |
|
Ok I've just added a non-regression unit test and fixed another one which was actually not testing anything and passing by pure luck. Also, I see you actually had a lot of specs about the auto-placement feature mentioning the viewport, this is good and comforts me into thinking that the behaviour I fixed was the expected one. The funny part is that all these specs currently passes because the viewport happen to also be the direct parent on these specs. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: df96c3e (Please note that this is a fully automated comment.) |
|
There seem to be some test failures in all versions of IE. |
|
Yep, I'm trying to have a look a this but can't manage to get tests running on IE9 yet. |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 5921724 (Please note that this is a fully automated comment.) |
|
This is now fixed. |
|
This looks safe to try to me, anything else on your end @cvrebert |
|
@fat Nope. |
Tooltip/popover: Fix auto placement to use viewport
Currently, auto placement is using the container dimensions (if provided) or the element's parent to determine where to open the tooltip:
This is quite broken in fact, because the parent element could be just a small div outside the element for example, leading in a totally random placement (placing the tooltip on top even if there's no room). And the container can also be outside of the viewport.
This fix simply uses the viewport instead, that's the purpose of the viewport actually, to position the tooltip.
So the auto placement should use it to find where there's more room.
By default this is body, which is good.