fix(tooltip): update placement on content change

[bekos]

Since the recompute of position is a heavy calculation and given
that this happens only when tooltip is visible, ie position
is already calculated, we can reuse it to update the position when
widht/height changes.

[bekos]

Added some tests to count on $position calls, in order to make sure that behaves as it should.

@pkozlowski-opensource LGTM :-) Any objections merging?

[pkozlowski-opensource]

@bekos I would love to see if we can avoid using $timeout which will slow things down by putting browser into rendering mode and triggering a digest loop in JS execution context. Why is the $timeout needed here? Is it because size / position is not updated by a browser yet? In this case we can trigger reflow by reading one of the size params.

What I'm trying to say here is that $timeout should be the last resort...

[bekos]

@pkozlowski-opensource When $observe is fired by the dynamic content change of the attribute, the actual content of the tooltip has not changed yet. This will happen on the next digest cycle. That's why I wire the position update, to also be fired in the next digest cycle. I hope this makes sense :-)

[bekos]

@pkozlowski-opensource By reading one of the size params you mean to $watch tooltip's width/height changes?

[bekos]

@pkozlowski-opensource (Too much noise! sorry) What about using the $timeout with invokeApply false?

[bekos]

@pkozlowski-opensource Updated to $timeout that does not invoke a $apply on it's execution. I am not aware of a better solution atm, since there is no event we can bind and no property we can read inside the $observe function.

[chrisirhc]

Another alternative (that I found while working on #1455) is to force one digest cycle on the tooltip's isolate scope by doing scope.$digest(). When this is done, it only runs watchers on this scope and child scopes which is minimal performance impact, and updates the content in the tooltip.

[bekos]

@chrisirhc Good point. I will try to compare them, but I believe that for both ways the performance impact is too small, and is only experienced in edges cases, since in the normal usage there is no performance hit.

We can also fire the content update only once every 100ms, or 200ms if it'too slow, but we are adding too much complexity for an edge case bug :-)

[chrisirhc]

@bekos actually it's quite different because $timeout causes a digest cycle on the $rootScope so all watchers in all scopes will run while doing scope.$digest just causes the watchers in this isolate scope to run.

The difference impact on performance is depends on the number of watchers and the complexity of the rest of the application (out of this directive). Hence, I would advise against using a $timeout.

[bekos]

@chrisirhc I call $timeout with invokeApply = false, so I think that no digest cycle is caused, no?

[chrisirhc]

Oh yes, right.

I just find $timeout logic makes the logic more difficult to test and timers should be avoided if possible especially if the purpose isn't to wait a specific amount of time. In this case, waiting for the content is actually waiting for a $digest cycle, not waiting time to pass.

You're right that the difference probably isn't huge if you don't invoke apply.

[bekos]

@chrisirhc $digest cannot be called inside $observe, since it is already in progress. Workarounds?

[chrisirhc]

Have you tried $evalAsync? Heh, I think that should work in this case.

Update: I thought about this again as I was looking through AngularJS core's $animate codebase. Perhaps $timeout without invoking apply wouldn't be so bad after all. If $evalAsync doesn't work, your $timeout solution is a good idea.

[bekos]

@chrisirhc Nice! 👍

[bekos]

@chrisirhc Just saw your update. $evalAsync works like charm, because DOM is updated and so we can read the new dimensions of the tooltip and position correctly.

[chrisirhc]

Excellent! I wasn't 100% certain it would work as I haven't actually tried it but now we know! 😄

[bekos]

@pkozlowski-opensource Is there something that still bothers you?

[pkozlowski-opensource]

@bekos @chrisirhc yep, in fact yes. In fact I'm pondering bigger rewrite for tooltips / popovers to be able to easily support templates for content and manual triggers. So I would like to just take a bit more time to understand the current implementation and confront it with what I've got on my mind. Give me few more days, it is not forgotten :-)

[bekos]

@pkozlowski-opensource No worries!

[chrisirhc]

After #1455, it's actually okay to update the position on any change of content or even just on any digest call on the tooltip's scope. This is because the tooltip's scope only exists when it's visible. As such, there can only be at most one tooltip visible and only one watcher that's always updating the tooltip to fit the content.

(Adding reference to #96 on this PR for tracking)

[rvanbaalen]

Closing this PR due to no activity.

[chrisirhc]

Resolved via #3435 .