fix tooltips disappearing after trying to interact during their fade out animation

[RyanBerliner]

Resolves #32372, resolves #31646

This points out that components that transition may have a blind spot in testing. Since transition duration & delay is computed using getTransitionDurationFromElement(element), which gets transitions from css, its always going to be returning 0 in the unit tests unless otherwise mocked or styled inline. That means that any components that use this transition method will have tests that mostly ignore them... which may not actually matter (likely true), but at least in this case it does matter.

Ideal in testing you'd mock the actual getTransitionDurationFromElement(element) imo, and just return the duration, however I was having some trouble getting jasmine spys to properly do this. I went for the next best thing in this case, which is mocking computed styles to get a non-zero return from that function. Happy to change it if anyones got ideas.

Preview: https://deploy-preview-33289--twbs-bootstrap.netlify.app/docs/5.0/components/tooltips/

[XhmikosR]

This fixes the issue, but doesn't fix the issue with the tooltip randomly showing up in the wrong position :/

2021-03-06_20-07-58.mp4

It probably is a separate issue, though.

But yeah, feel free to do what you think it's best with mocking since we have been already bitten by this issue.

[RyanBerliner]

Yeah - I figured I'd leave that to #31646 as the positioning issues seem to have everything to do with mouseout and mouseover behavior being investigated there already. You can actually very consistently get the position of the tooltip to mess up if you slowly move your mouse onto the button from where the tooltip is going to appear... because it triggers the following events

• mouse hovers over trigger, to start showing tooltip
• the tooltip shows, but in a position that the mouse is on, which is currently registers a "leave" because the mouse isn't actually in trigger anymore. So, the tooltip hides
• with the tooltip hidden, the cursor is back on the trigger... showing the tooltip again... and the cycle continues until positioning fails

Screen.Recording.2021-03-06.at.1.20.37.PM.mov

Which is of course why adding pointer-events:none; to the tooltip works as mentioned earlier.

Long story short, that seemed separate from the already being dealt with #31646 so I've ignored it for this. It does though seem that the code included in this pr makes solving the other issue less complicated. I'll leave this pr as is, lmk if you'd like anything changed here or a hand on the other issue

[XhmikosR]

If you can help us with the other issue, please go ahead :)

Lack of tests caused these issues in the first place. Currently the tooltip and event handling issues are blocking us.

[RyanBerliner]

This should now resolve #31646 as well. My initial thinking was a bit off. The issue with tooltips ending up in weird locations was due to trying to create a NEW tooltip before the other one was destroyed. I also decided NOT to handle tooltip/trigger overlapping, because since it looks like tooltip offset is allowed to be customized by users easily, in the edge cases that it matters users can just make the offset larger.

Lastly, I changed the default ordering of fallbackPlacements to be ['top', 'bottom', 'left', 'right'] since it seemed strange to me that if "top" tooltips where scrolled to the top of the viewport they would do anything but go to the bottom.

[rohit2sharma95]

Thanks for the PR @RyanBerliner

Change in the fallbackPlacements placement seems unrelated here 🙂
The default value was added here to override the default value of Popper because Popper does not flip the element if there is not enough space on the opposite side. And it was kept clockwise (starting from the top) as that is the usual pattern (like giving CSS padding and margin). Your opinion is also good but that is just for the top and bottom placement if the default placement is on the right side and the user scrolls horizontally, the tooltip will move to top => bottom => left (that is neither the opposite of right not clockwise 🙂).

That default pattern may not be preferred by everyone but that can be overridden by the configuration of the tooltip.

Ref: #32843 and https://popper.js.org/docs/v2/modifiers/flip/#fallbackplacements

[rohit2sharma95]

Can you please add tests for the second issue as well 🙂

[RyanBerliner]

1. fallbackPlacement - I've reverted this to the clockwise rotation it was before
2. Testing - I already added a tested for the "strange popper placement" bug. You'll see in this previous commit I add a check for consistent tooltip placement. Without the fix in place this test fails identically to how you'd see it in the browser, jumping from 'top' to 'right'. If you'd rather I split it out into its own test, or spy on its implementation more I can do that... not sure how far into the weeds to go testing implementation vs outcome. happy to change it.

[XhmikosR]

@RyanBerliner I haven't looked at your changes yet, but I wanted to say a huge thank you for jumping into these issues! ❤️

The tooltip and event issues are the ones blocking as AFAICT, so this would unblock beta3 and generally v5.

[RyanBerliner]

Also, please consider this INSERTED event. Depending on how you anticipate this event to be used by people, it may make most sense to only trigger it on true, new popper creation, otherwise it will be triggered every time someone re-enters a tooltip trigger while its transitioning to hidden.

[rohit2sharma95]

True, it should be triggered only when the tooltip is inserted in the DOM.

bootstrap/js/src/tooltip.js

Line 280 in 548be2e

if (!this._element.ownerDocument.documentElement.contains(this.tip)) {

[XhmikosR]

@rohit2sharma95 @GeoSot LGTY? I think it's a clean way to fix the issues we are hitting.

[GeoSot]

Maybe I am wrong and maybe it not the proper place to ask for this, but I think
_activeTrigger ""reason" is not used somewhere except line 198.

context._activeTrigger.click = !context._activeTrigger.click

Wouldn't be easier to keep only _activeTrigger as boolean value?
I see much 'noise' without purpose

[alpadev]

Maybe I am wrong and maybe it not the proper place to ask for this, but I think
_activeTrigger ""reason" is not used somewhere except line 198.

context._activeTrigger.click = !context._activeTrigger.click

Wouldn't be easier to keep only _activeTrigger as boolean value?
I see much 'noise' without purpose

While I think this could be refactored to be a boolean it also keeps track if something is focused while not being hovered for example.

[alpadev]

@RyanBerliner added a draft PR #33310 that would address what you mentioned earlier (some little revenge for you being faster with this one 😁)

[GeoSot]

@RyanBerliner can you leave a comment on top of first time-out explaining its purpose with zero delay? Or replace zero timeout with 1milisec

I suppose you wrote it like this to give a minimum time to Tooltip to be rendered

[RyanBerliner]

tbh, I know it's not a good reason, but I did it this way because it matches styling from this existing tooltip test. And given that the following is true:

setTimout(() => console.log('here 1'), 0)
console.log('here 2')

------ Outputs ------

"here 2"
"here 1"

I figured that it's the preferred syntax in this case. I can change it, but if I do it should probably be changed in the other tests as well for consistency?

[GeoSot]

Good point.
Personally I would prefer at least the zero to be 1 but I think,cc/ @XhmikosR the monster of code consistency, will give us his opinion

[XhmikosR]

Would be nice to streamline these but in a separate PR after this. For now, I just want us to merge this and any other beta3 PRs so that we release beta3 next week :)

[XhmikosR]

Thanks @RyanBerliner! If you have time, feel free to help us with any other JS-related PRs :)