Popover expanding height

[MrSuttonmann]

Hi,
I updated to 0.13.2 earlier and it appears to have broken the script to calculate where the top of a popover should be based on the height.

For example, I have a popover open set to "popover-placement="top" and add some content in which will increase the height. Incorrectly, the bottom of the popover moves down, and the top stays static. Prior to 0.13.2, the top of the popover would have moved up as the bottom stayed static.

See images for an example - note the button is covered in the 0.13.2 example:

[wesleycho]

Hm, I wonder if this is related to the changes made in the $tooltip service due to a bug in tooltip positioning found in tooltips.

[wesleycho]

Can you create a reproduction in Plunker?

[RobJacobs]

Here is a plunk reproducing the problem. Open the popover, then start typing in the input until the content wraps. Possibly related to this PR: 632aa82

_EDIT_
The tooltip is relying on the watch for contentExp to call the position logic for the popover-template. The contentExp just holds the template url for the popover so there is nothing watching the actual content of the template to change.

[wesleycho]

Hm, probably related, although that change fixes a positional bug with the tooltip that results from calling it too often.

[wesleycho]

Do we need to just reposition on every digest? I'm not sure how we would be able to reliably position the tooltip otherwise.

[RobJacobs]

@wesleycho I'll play with this for a bit and will have some suggestions tomorrow

[RobJacobs]

Here's what I'm thinking, rather than watching content (type, title, etc..) create a watch for the tooltip element size to trigger a call to position, something like:

tooltipLinkedScope.$watch(function() { 
  return tooltip[0].clientHeight + tooltip[0].clientWidth; }, 
  function() {
     positionTooltip(); 
});

We could potentially remove the other calls to the positionTooltip and just have that one watch responsible for handling when to position the tooltip. We may also want to consider implementing a timeout on the actual positionTooltip function like so:

var positionTimeout;
var positionTooltip = function () {
  if (!tooltip) { return; }

  if (positionTimeout) {
    $timeout.cancel(positionTimeout);
  }
  positionTimeout = $timeout(function() {
    var ttPosition = $position.positionElements(element, tooltip, ttScope.placement, appendToBody);
    ttPosition.top += 'px';
    ttPosition.left += 'px';

    // Now set the calculated positioning.
    tooltip.css( ttPosition );
    positionTimeout = null;
  }, 0, false);
};

[pkozlowski-opensource]

create a watch for the tooltip element size to trigger a call to position

This would be very, very bad idea from the performance point of view. We would be queering DOM and potentially causing reflows on each and every turn of the $digest loop. This is big no-no.

[pkozlowski-opensource]

The potential solution would be to trigger re-position at the end of the digest loop, if we really want to do this that often.

[RobJacobs]

@pkozlowski-opensource Yeah I agree, but I just wasn't sure how else to handle this. I'll try your suggestion, thanks.

[RobJacobs]

@pkozlowski-opensource @wesleycho So I'm stuck on this, the crux of the problem is the popover template content is not bound to the tooltip scope, so there is no elegant way to trigger a position calc. You mention trigger a reposition at the end of the digest loop, how would you implement that? The commit that broke the popover-template positioning behavior was removing the following implementation:

tooltipLinkedScope.$watch(function () {  
  $timeout(positionTooltip, 0, false);  
});  

as it was causing performance concerns.

[wesleycho]

Maybe something like

var repositionScheduled = false;
tooltipLinkedScope.$watch(function() {
  if (!repositionScheduled) {
    repositionScheduled = true;
    tooltipLinkedScope.$$postDigest(repositionTooltip);
  }
});

function repositionTooltip() {
  repositionScheduled = false;
  $timeout(positionTooltip, 0, false);
}

[RobJacobs]

@wesleycho Here is a plunk with the tooltip code extracted into the tooltip.js file with your suggestion so we can play with this scenario easier. The end result seems to be the same thing we started with, a reposition of the tooltip on every digest cycle of the linked scope regardless if the popover content is impacted. I added some console log statements that will show how often this is occurring.

[wesleycho]

My suggestion should be the worst case solution.

There's an important difference with my suggestion and the previous behavior of executing on each watch function evaluation - my suggestion defers all rendering to a single pass after the models on $scope stabilize. I fear we may have to go this route since we do not have access to when the content changes.

[RobJacobs]

I thought your approach would behave like that as well (wait until all digests are complete) but I'm seeing something different in the plunk I created. I added a log statement to show the $rootScope.$$postDigestQueue.length and the first time the popover opens, it hits the repositionTooltip function twice with length of 1, then 0. But I'm not sure if that is an accurate indication of what is truly happening...

[wesleycho]

I think it works as expected - I forked your Plunker and moved your console.log here, since I suspect you were encountering a console.log logging by reference issue of some sorts.

[wesleycho]

Resolved by 895a228