Force tooltip and popover to recreate content every time it opens

[GeoSot]

Closes #35029
refers to #35677

This was done during an optimization and to reduce tooltip lifecycle load and maybe it should be better to be optional on v6

@blaskognia can you test it and confirm you are ok with this change?

• fix tests

[blaskognia]

@GeoSot this produce another issues.

You said that this is changed by performance reason. But in orginal Bootstrap 5.1.3 function that is put as content is RUNNING always when popover is opened. So function always do some staff but result of this function is never put into popover. Where is perfomance in this case? Function still running.

Even more! On the first time (when popover is showing for the first time) function is running twice not once.

When i put fragment of your code into original code then function is running three times on the first time.

You understand? If this functions not be runned again and again when popover is shoving, then we can said that performance increase. But when function still runnning, nothing change.

[blaskognia]

@GeoSot it will work truly properly onluy when we dispose popover after click and creat it again. THIS is bad for pertformance reasons but works for me.

[GeoSot]

@blaskognia please try to be more descriptive, in order to come up together with a solution

But in orginal Bootstrap 5.1.3 function that is put as content is RUNNING always when popover is opened. So function always do some staff but result of this function is never put into popover. Where is perfomance in this case? Function still running.

which exact function does staff but its result is never used?

Even more! On the first time (when popover is showing for the first time) function is running twice not once.

Which function is running twice , in order to check it?

Try to use names, to help identify this issue

[blaskognia]

@GeoSot - ok i'm sorry for too small amount of informations.

So... sorry for language...

Link to tests: https://codepen.io/szymon-koronkiewicz/pen/OJxaPym

We have a button. This button DON'T HAVE title. Why? Because our popover have individualy generatet content, and this content is based not on title of element but on result of function that we put as content when we initiate popover.

We have bootstrap 5.1.3 with jquery. Code in link is not beatufiul but tell as many things about our problem.

Function that i gave as content into popover do three things. First increase by one variable named iteration. Second put actual iteration value into html code. Third is return actual date (this date should be insert into popover every time when popover opens).

Corect behavior (first click): we click on button. Iteration increase to one, one is put into html (we should see one on the bottom of button) and popover should open with actual date.
Cirect behavior (next click): we click on button. Iteration increace to two, two is put into html (we should see two on the bottom of button) and popover should open with actual (NEW) date different thant first one date (becacues some time was remain).

Actual behavior (first click): we click on button. Iteration increase to two (function was runned twice - i don't know why), two is put into html (we see two on the bottom of button), and popover open with actual date. Incorrect is that function is running twice, rest is ok.
Actual behavior (next click): we click on button. Iteration increase by one to three (correct because before we had two), three is put into html (correct), and popover open BUT not with new date, but with old date generated before.

==========

So you see? Function that i put as content is running every time when i click on button. I don't know why this function is running twice for first time. So in this situation there is no performance PLUS because function is running all the time (plus one on the begginning).

==========

!!!

Interesting is that when we insert attribute title (this is important) into button, then when we click for first time on button function insterted as content is running only one time and return actual date that is put into popover. BUT when we click again variable iteration no change and that is why we know, that function is running only one time when we first time click on the button.

Conclusion is that you can see this problem only when button dosn't have title attributte and behavior of module is almost correct because it seems to know that function must be running again and again but not using result of this function (function that is put into content of popover).

So i think that this wasn't a performance solution but regular bug.

[GeoSot]

Although I appreciate the example you made, it is a bit difficult for me to follow the code

I 'll ask again, replace the bootstrap script with this https://deploy-preview-35679--twbs-bootstrap.netlify.app/docs/5.1/dist/js/bootstrap.min.js and tell me if you are ok with it

[blaskognia]

@GeoSot thanks but this not working.

First click: no reaction

Second click error in console:

tooltip.js:223 Uncaught TypeError: Cannot read properties of null (reading 'remove')

[blaskognia]

I was wrong - first click also generate this error.

[blaskognia]

@GeoSot

Error was generated by my fragment of code that is no compatibile with your JS file.

I was update pen to be simplest as possible. Now you have easy way to analyze.

https://codepen.io/szymon-koronkiewicz/pen/OJxaPym

This is with your link to bootstrap not original.

Behavior is the same. In first click we have 2 not 1 iteration, we have date but on the next click date still be the same.

[GeoSot]

tooltip.js:223 Uncaught TypeError: Cannot read properties of null (reading 'remove')

forgot to check for existence ;)

[blaskognia]

@GeoSot yes but behavior is the same so you version of bootstrap nothing changes.

[arcanisgk]

i have tested it on: link and work fine

[louismaximepiton]

I think that the main issue for performance in here is that there isn't any title on buttons so the initialization is done twice in a row. I think this might be another bug or at least a performance issue to be cleared. Here is the associated CodePen. TBH, I don't really know exactly where the issue comes from but the increment on DOM is done by l.215 (this._getTipElement()) and the other increment is done between l.197 and l.201.

I don't know if you want me to try some other things ?

[louismaximepiton]

Do we need those two lines (l.347 and l.348) since show does it on its own ?

[GeoSot]

I am afraid in case we do not remove tip and set null into the variable, tooltip will not be re-created

[louismaximepiton]

I see your point, but my point is that the show function already does it l.211 and l.212 and the function is called every time we get in this case. I hope I didn't miss anything but this.tip isn't called before those lines. Nothing changed when I removed those on my side.

[GeoSot]

I don't know if you want me to try some other things ?

Yes, please feel free to try. Any new eye may see something different than mine 😉

[louismaximepiton]

For what I tried on my side, with the previous CodePen from @blaskognia, the content is played twice when there's no title. Here are my concerns about it:

First of all, a title could be added directly to the object passed to Popover (or the attribute title), it'd remove the call to content in this case. But I don't think that this is the main issue in here:

• If we set a title with a function, it'll be called twice.
• For me the real problem comes from the _isWithContent() function itself (resp. overridden in popover). Indeed, this function calls _getTitle() (resp. _getTitle() || _getContent()) that calls itself (resp. themselves) _resolvePossibleFunction(arg). So to check existence, we do the same process as if we would build it. IMO, the best way to handle it is to remove these calls. For example: _isWithContent() { return this._getTitle() || this._config.content } in popover file remove the second call to content.

I don't know if you want me to commit/PR on this branch @GeoSot?

[GeoSot]

If we set a title with a function, it'll be called twice.

because it tries to resolve it every time we call it. It was what I was trying to avoid there, but seems other devs need it

I don't know if you want me to commit/PR on this branch @GeoSot?

You are right. Probably is better to open a pr, so we can check the differences

[louismaximepiton]

Here is the working PR I get: #36585
Preview doesn't seem to be released so the link of the example I made is: http://localhost:9001/docs/5.2/examples/headers/

Please note that I added an example at a random place so please don't merge this.

[julien-deramond]

My analysis shows the same result mentioned by @louismaximepiton in #35679 (comment) regarding this._isWithContent().

With @louismaximepiton's CodePen in mind, if we add several this._isWithContent() in the show() method we can observe that the content function is called multiple times and so the increment variable is incremented multiple times:

show() {
     if (this._element.style.display === 'none') {
       throw new Error('Please use show on visible elements')
     }

     // Try this
     this._isWithContent()
     this._isWithContent()
     this._isWithContent()
     // End
 
     if (!(this._isWithContent() && this._isEnabled)) {
       return
     }
}

We expect this._isWithContent() to check some values and return a boolean, not to call functions.

---

Note: I haven't checked the proposal in #36585 yet

[GeoSot]

@julien-deramond as #36585 is on top of this, how do you suggest proceeding here?
(may we merge this and continue with the next, or something else?)

[julien-deramond]

@julien-deramond as #36585 is on top of this, how do you suggest proceeding here? (may we merge this and continue with the next, or something else?)

The topic is complicated with a lot of edge cases. IMO we could merge this one that already fixes some cases and let's continue to work on #36585 or on another PR to try to improve the global behavior.
Let's keep in mind the comments in this PR and maybe let's gather all the weird use cases in one CodePen as a reference (and then if possible as unit tests).

[julien-deramond]

I approve it as a first version needing more work to be done to tackle all edge cases (see #35679 (comment))