Skip to content

MudToolTip: Move Duration Delay#11051

Merged
ScarletKuro merged 11 commits into
MudBlazor:devfrom
versile2:fix/tooltipdelayduration
Mar 22, 2025
Merged

MudToolTip: Move Duration Delay#11051
ScarletKuro merged 11 commits into
MudBlazor:devfrom
versile2:fix/tooltipdelayduration

Conversation

@versile2

Copy link
Copy Markdown
Contributor

Description

Regression from #10853
Combines with #11038 to fix all problems with tooltips that I'm aware of by moving Duration/Delay to the tooltip instead of Popover and implementing DebounceDispatcher.
Resolves #11045 and #11046

How Has This Been Tested?

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions Bot added the bug Unexpected behavior or functionality not working as intended label Mar 18, 2025
@codecov

codecov Bot commented Mar 18, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.06%. Comparing base (0943a12) to head (5f36917).
Report is 4 commits behind head on dev.

Files with missing lines Patch % Lines
...c/MudBlazor/Components/Tooltip/MudTooltip.razor.cs 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #11051      +/-   ##
==========================================
+ Coverage   91.05%   91.06%   +0.01%     
==========================================
  Files         429      429              
  Lines       13964    13986      +22     
  Branches     2698     2701       +3     
==========================================
+ Hits        12715    12737      +22     
- Misses        646      647       +1     
+ Partials      603      602       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@versile2 versile2 marked this pull request as ready for review March 18, 2025 21:22
@versile2 versile2 requested a review from danielchalmers March 19, 2025 13:06

@danielchalmers danielchalmers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that it relies on timing as a bandaid. Can the popover predict this is going to happen and prevent it, instead of handling it afterwards in the consumer?

And should this be a native part of the popover so it applies to all controls equally? It would be good if the tooltip can stay minimal and easy to recreate and all the popover stuff goes in the popover implementation.

Comment thread src/MudBlazor/Components/Tooltip/MudTooltip.razor.cs Outdated
@versile2

versile2 commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

I don't like that it relies on timing as a bandaid. Can the popover predict this is going to happen and prevent it, instead of handling it afterwards in the consumer?

And should this be a native part of the popover so it applies to all controls equally? It would be good if the tooltip can stay minimal and easy to recreate and all the popover stuff goes in the popover implementation.

Popover already has a Delay and Duration, if we want it to continue to handle ToolTips then we can undo my PRs to improve performance as they caused these problems to begin with. In my opinion the performance gains those PRs achieved is worth the extra headache but that's an opinion and I won't get my feelings hurt either way.

Every single item you have a tooltip on pre those PRs is 2 items in the dom, if it was a custom tooltip it could be 3+ items. Put a single tooltip on a page of 25 DataGrid results and now you have 50+ dom items in a page. Then add in javascript connection by multiple events to the # of tooltips and it starts to get out of hand really fast.

@danielchalmers

Copy link
Copy Markdown
Member

I don't like that it relies on timing as a bandaid. Can the popover predict this is going to happen and prevent it, instead of handling it afterwards in the consumer?
And should this be a native part of the popover so it applies to all controls equally? It would be good if the tooltip can stay minimal and easy to recreate and all the popover stuff goes in the popover implementation.

Popover already has a Delay and Duration, if we want it to continue to handle ToolTips then we can undo my PRs to improve performance as they caused these problems to begin with. In my opinion the performance gains those PRs achieved is worth the extra headache but that's an opinion and I won't get my feelings hurt either way.

Every single item you have a tooltip on pre those PRs is 2 items in the dom, if it was a custom tooltip it could be 3+ items. Put a single tooltip on a page of 25 DataGrid results and now you have 50+ dom items in a page. Then add in javascript connection by multiple events to the # of tooltips and it starts to get out of hand really fast.

Thank you for the context! I'll try to look into this further, would hate to lose perf benefits but I don't think I was active in that PR.

@ScarletKuro ScarletKuro changed the title MudToolTip Move Duration Delay MudToolTip: Move Duration Delay Mar 21, 2025
@ScarletKuro ScarletKuro linked an issue Mar 21, 2025 that may be closed by this pull request
4 tasks
@ScarletKuro

Copy link
Copy Markdown
Member

@danielchalmers so do I merge this?

@sonarqubecloud

Copy link
Copy Markdown

@danielchalmers

Copy link
Copy Markdown
Member

@ScarletKuro will look more Saturday. I think debounce should be avoided at all cost (if I understand this right)

@danielchalmers

Copy link
Copy Markdown
Member

Is the addition of guarding the MudPopover in MudToolTip with _visibleState.Value the cause of the regression?

@ScarletKuro

ScarletKuro commented Mar 22, 2025

Copy link
Copy Markdown
Member

Is the addition of guarding the MudPopover in MudToolTip with _visibleState.Value the cause of the regression?

It was a regression from one of this PRs: #10853, #10856 didn't investigate which exactly.

@ScarletKuro

Copy link
Copy Markdown
Member

I think debounce should be avoided at all cost (if I understand this right)

I'm fine with the current solution, by the way. There is no reason to spam-update the _visibleState.Value on pointer leave/enter. I'd say this is a very common practice to debounce such spammy events, and you can see this a lot in JS frameworks.

@versile2

versile2 commented Mar 22, 2025

Copy link
Copy Markdown
Contributor Author

Also just fyi, the old way was constantly updating MudPopover who was sending new Delay/Duration updates to CSS. The CSS handled it well but updates also meant js callbacks. I do prefer CSS over the debounce solution but even if we reverted the other two PRs we would still get gains from adding the debounce updates.

@danielchalmers

Copy link
Copy Markdown
Member

Ah, thanks guys (I didn't understand it right). Didn't test the branch but it seems logical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudTooltip not removed when element changes 8.4.0 MudTooltip breaks in MudMenu 8.4.0

3 participants