Use a general approach to show tooltip, fix temporary tooltip bug #23574

wxiaoguang · 2023-03-19T13:47:03Z

TLDR

Improve performance: lazy creating the tippy instances.
Transparently support all "tooltip" elements, no need to call initTooltip again and again.
Fix a temporary tooltip re-entrance bug, which causes showing temp content forever.
Upgrade vue3-calendar-heatmap to 2.0.2 with lazy tippy init (initHeatmap time decreases from 100ms to 50ms)

Details

The performance

Creating a lot of tippy tooltip instances is expensive. This PR doesn't create all tippy tooltip instances, instead, it only adds "mouseover" event listener to necessary elements, and then switches to the tippy tooltip

The general approach for all tooltips

Before, dynamically generated tooltips need to be called with initTooltip.

After, use MutationObserver to:

Attach the event listeners to newly created tooltip elements, work for Vue (easier than before)
Catch changed attributes and update the tooltip content (better than before)

It does help a lot, eg:

gitea/web_src/js/components/PullRequestMergeForm.vue

Lines 33 to 36 in 1a4efa0

    
           <div class="ui label"> 
        
             <!-- TODO: Convert to tooltip once we can use tooltips in Vue templates --> 
        
             {{ mergeForm.textClearMergeMessageHint }} 
        
           </div>

Temporary tooltip re-entrance bug

To reproduce, on try.gitea.io, click the "copy clone url" quickly, then the tooltip will be "Copied!" forever.

After this PR, with the help of attachTippyTooltip, the tooltip content could be reset to the default correctly.

Other changes

data-tooltip-content is preferred from now on, the old data-content may cause conflicts with other modules.
data-placement was only used for tooltip, so it's renamed to data-tooltip-placement, and removed from createTippy.

silverwind · 2023-03-19T15:06:39Z

One change I was planning to to is to remove the tooltip class and have it based purely on a single data-tooltip="content" attribute. Would recommend if we touch tooltips deeply, we do this refactor here now.

I certainly can't approve two ways to init tooltips like this PR does via coexistance of data-tooltip-content and data-content. There should only be one way.

wxiaoguang · 2023-03-19T15:09:27Z

One change I was planning to to is to remove the tooltip class and have it based purely on a single data-tooltip="content" attribute. Would recommend if we touch tooltips deeply, we do this refactor here now.

It would be too much. Could we just have this one, and do the replacing data-content => data-tooltip-content in next PR?

wxiaoguang · 2023-03-19T15:11:27Z

I certainly can't approve two ways to init tooltips like this PR does via coexistance of data-tooltip-content and data-content. There should only be one way.

gitea/docs/content/doc/developers/guidelines-refactoring.en-us.md

Lines 31 to 51 in dbdb5ba

    
           ## Refactoring Suggestions 
        
           * Design more about the future, but not only resolve the current problem. 
        
           * Reduce ambiguity, reduce conflicts, improve maintainability. 
        
           * Describe the refactoring, for example: 
        
             * Why the refactoring is needed. 
        
             * How the legacy problems would be solved. 
        
             * What's the Pros/Cons of the refactoring. 
        
           * Only do necessary changes, keep the old logic as much as possible. 
        
           * Introduce some intermediate steps to make the refactoring easier to review, a complete refactoring plan could be done in several PRs. 
        
           * If there is any divergence, the TOC(Technical Oversight Committee) should be involved to help to make decisions. 
        
           * Add necessary tests to make sure the refactoring is correct. 
        
           * Non-bug refactoring is preferred to be done at the beginning of a milestone, it would be easier to find problems before the release. 
        
           ## Reviewing & Merging Suggestions 
        
           * A refactoring PR shouldn't be kept open for a long time (usually 7 days), it should be reviewed as soon as possible. 
        
           * A refactoring PR should be merged as soon as possible, it should not be blocked by other PRs. 
        
           * If there is no objection from TOC, a refactoring PR could be merged after 7 days with one core member's approval (not the author). 
        
           * Tolerate some dirty/hacky intermediate steps if the final result is good. 
        
           * Tolerate some regression bugs if the refactoring is necessary, fix bugs as soon as possible.

Introduce some intermediate steps to make the refactoring easier to review, a complete refactoring plan could be done in several PRs.
Tolerate some dirty/hacky intermediate steps if the final result is good.

silverwind · 2023-03-19T15:13:49Z

data-tooltip-content

Please just use data-tooltip, less to write and same effect.

Tolerate some dirty/hacky intermediate steps if the final result is good.

I don't know about others but I generally can't accept suboptimal "intermediate" solutions. Just go all the way please.

wxiaoguang · 2023-03-19T15:15:30Z

data-tooltip-content

Please just use data-tooltip, less to write and same effect.

I do like data-tooltip-content, it matches tippy.setContent. Maybe let most maintainers choose.

Tolerate some dirty/hacky intermediate steps if the final result is good.

I don't know about others but I generally can't accept suboptimal "intermediate" solutions. Just go all the way please.

If you do not have strong objection, I will ask other maintainers' opinions.

silverwind · 2023-03-19T15:28:48Z

I don't know but I think I've personally had enough of your "temporary" solutions. I want robust, simple and easy to maintain solutions, not such a mess like the getComputedStyle when toggling a class.

wxiaoguang · 2023-03-19T15:41:56Z

I don't know but I think I've personally had enough of your "temporary" solutions. I want robust, simple and easy to maintain solutions, not such a mess like the getComputedStyle when toggling a class.

I do not know what do you mean.

I have done a lot of refactorings, all of them are stable. And I have used my solutions (which you don't like, for example, catch frontend bugs, improve async calls, use pageData to introduce more Vue componenets, etc) resolved many Gitea bugs.

For the getComputedStyle, it doesn't affect anything, why it would bother you? Actually, you forced to make the show/hide refactoring in one PR, then some regression bugs come. If we could do it step by step, I am sure many regression bugs could be avoided. (Update: if you want to remove the assertion code, you can take a look at #23576)

I always finish my promises, to make the refactorings clear and stable. You can check:

The whole frontend refactoring, Frontend Plan #17149 , a lot of related PRs
Refactor branch/tag selector dropdown (first step) #23394 , then it's completed by Refactor branch/tag selector to Vue SFC #23421
Backend refactoring, Refactor git command arguments and make all arguments to be safe to be used #21535 and Refactor git command package to improve security and maintainability #22678
Backend refactoring, Decouple unit test code from business code #17623 and Decouple unit test, remove intermediate unittestbridge package #17662
And even more, if you are interested, I can find more.

I do not understand what you mean by "mess". If you say mess, the Gitea frontend (before I joint) was totally a mess.

web_src/js/modules/tippy.js

silverwind · 2023-03-19T19:06:08Z

web_src/js/modules/tippy.js

+      if (mutation.type === 'childList') {
+        for (const el of mutation.addedNodes) {
+          // handle all "tooltip" elements in newly added nodes, skip non-related nodes (eg: "#text")
+          if (el.querySelectorAll) {


this line looks broken.

Not broken, just a quick check to see the "node" has querySelectorAll function, which is needed by attachChildrenLazyTippyTooltip below.

Otherwise some nodes (like #text) couldn't be used to attach tippy.

I agree that there would be other approaches to check if the node is non-#text, at the moment, this quick check is clear and fast IMO.

Use node.nodeType please, it's much more clear.

Can you provide a list for the types?

Should be [Node.ELEMENT_NODE,Node.DOCUMENT_FRAGMENT_NODE].includes(el.nodeType).

But that's slower than the if (el.querySelectorAll) check. Do you really want to do so?

Alternatively 'querySelectorAll' in el would also make the indent clear enough for me.

OK, then 'querySelectorAll' in el : ba8ad89

silverwind · 2023-03-19T19:08:36Z

I can accept a few of these changes here but the duality of data-content and data-tooltip-content needs to go. There must only be one attribute. Wich name it is, we can bikeshed later.

HesterG · 2023-03-20T08:03:32Z

DiffFileTreeItem might need :data-content="item.name" as it is also a tooltip?

wxiaoguang · 2023-03-20T08:15:03Z

DiffFileTreeItem might need :data-content="item.name" as it is also a tooltip?

According the comment below that line, the title is used by design. I have reviewed the DiffFileTreeItem PR before, at that time, most people agree the the "tippy tooltip" is too heavy, so the DiffFileTreeItem only used the title. I believe the "tooltip" class on it could be removed (I will remove it in this PR)

…tip" and "tippy"

codecov-commenter · 2023-03-20T10:16:02Z

Codecov Report

Merging #23574 (690ed36) into main (f521e88) will increase coverage by 0.01%.
The diff coverage is 37.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23574      +/-   ##
==========================================
+ Coverage   47.14%   47.16%   +0.01%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152353     +907     
==========================================
+ Hits        71397    71854     +457     
- Misses      71611    72014     +403     
- Partials     8438     8485      +47

Impacted Files	Coverage Δ
cmd/dump.go	`0.67% <0.00%> (ø)`
cmd/web.go	`0.00% <0.00%> (ø)`
models/actions/run.go	`1.64% <0.00%> (-0.08%)`	⬇️
models/actions/runner.go	`1.44% <ø> (ø)`
models/packages/package.go	`45.45% <0.00%> (-1.13%)`	⬇️
models/user/search.go	`77.50% <0.00%> (-6.29%)`	⬇️
modules/actions/github.go	`0.00% <0.00%> (ø)`
modules/actions/workflows.go	`0.00% <0.00%> (ø)`
modules/context/context.go	`64.54% <0.00%> (-3.53%)`	⬇️
modules/doctor/storage.go	`31.93% <0.00%> (ø)`
... and 33 more

... and 34 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

# Conflicts: # web_src/js/index.js

wxiaoguang · 2023-03-23T04:04:16Z

but the duality of data-content and data-tooltip-content needs to go.

There are still many new PRs still using data-content. To avoid conflicts and make things clear, I will fix it in next PR.

To explain why (with details):

If I also refactor data-content, then this PR will get conflicts when other PRs come, I am sure nobody wants to review the conflicts again and again, which are mixed with a lot of other code.

To make things clear, in next PR, we "only" rename the data-content, even if some conflicts come, it's still clear that we can focus on the "name" itself, no need to care about other code.

Please follow #23649 after this gets merged.

wxiaoguang · 2023-03-23T10:09:10Z

but the duality of data-content and data-tooltip-content needs to go.

Let's move to #23649 , it's big enough for reviewing.

* upstream/main: Fix incorrect `HookEventType` of pull request review comments (go-gitea#23650) [skip ci] Updated translations via Crowdin Fix codeblocks in the cheat sheet (go-gitea#23664) Drop migration for ForeignReference (go-gitea#23605) Fix new issue/pull request btn margin when it is next to sort (go-gitea#23647) A tool to help to backport locales, changes source strings to fix other broken translations (go-gitea#23633) Fix incorrect `show-modal` and `show-panel` class (go-gitea#23660) Restructure documentation. Now the documentation has installation, administration, usage, development, contributing the 5 main parts (go-gitea#23629) Check LFS/Packages settings in dump and doctor command (go-gitea#23631) Use a general approach to show tooltip, fix temporary tooltip bug (go-gitea#23574) Improve workflow event triggers (go-gitea#23613) Improve `<SvgIcon>` to make it output `svg` node and optimize performance (go-gitea#23570)

Follow: * #23574 * Remove all ".tooltip[data-content=...]" Major changes: * Remove "tooltip" class, use "[data-tooltip-content=...]" instead of ".tooltip[data-content=...]" * Remove legacy `data-position`, it's dead code since last Fomantic Tooltip -> Tippy Tooltip refactoring * Rename reaction attribute from `data-content` to `data-reaction-content` * Add comments for some `data-content`: `{{/* used by the form */}}` * Remove empty "ui" class * Use "text color" for SVG icons (a few)

fix

d06b47c

wxiaoguang force-pushed the fix-tippy-tooltip branch from 6f2622e to d06b47c Compare March 19, 2023 13:50

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2023

silverwind reviewed Mar 19, 2023

View reviewed changes

web_src/js/modules/tippy.js Show resolved Hide resolved

silverwind reviewed Mar 19, 2023

View reviewed changes

wxiaoguang added 3 commits March 20, 2023 16:15

remove unnecessary "tooltip" class from DiffFileTreeItem

45981f5

remove non-general default value for placement

b0f056f

add comments, rename some names to clarify the relation between "tool…

3e195a2

…tip" and "tippy"

wxiaoguang force-pushed the fix-tippy-tooltip branch from 3f6eb42 to 3e195a2 Compare March 20, 2023 09:59

upgrade vue3-calendar-heatmap to 2.0.2 with lazy-tippy-init

462257d

wxiaoguang mentioned this pull request Mar 20, 2023

[Proposal] Optimize the index.js #23461

Closed

wxiaoguang added 2 commits March 20, 2023 23:27

use mouseover instead of mouseenter

b266575

use: 'querySelectorAll' in el

ba8ad89

silverwind changed the title ~~Use a general apporach to show tooltip, fix temporary tooltip bug~~ Use a general approach to show tooltip, fix temporary tooltip bug Mar 20, 2023

lunny added the topic/ui Change the appearance of the Gitea UI label Mar 21, 2023

lunny added this to the 1.20.0 milestone Mar 21, 2023

wxiaoguang mentioned this pull request Mar 21, 2023

Diff improvements #23553

Merged

wxiaoguang added 2 commits March 22, 2023 10:04

Merge branch 'main' into fix-tippy-tooltip

690ed36

Merge branch 'main' into fix-tippy-tooltip

1175c4f

# Conflicts: # web_src/js/index.js

HesterG approved these changes Mar 23, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 23, 2023

lunny approved these changes Mar 23, 2023

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 23, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023

Merge branch 'main' into fix-tippy-tooltip

0544cd3

Merge branch 'main' into fix-tippy-tooltip

c083b4e

wxiaoguang mentioned this pull request Mar 23, 2023

Use data-tooltip-content for tippy tooltip #23649

Merged

lunny merged commit 9be90a5 into go-gitea:main Mar 23, 2023

lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 23, 2023

wxiaoguang deleted the fix-tippy-tooltip branch March 23, 2023 09:56

wxiaoguang mentioned this pull request Mar 29, 2023

Tooltips or static help texts? #21321

Open

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a general approach to show tooltip, fix temporary tooltip bug #23574

Use a general approach to show tooltip, fix temporary tooltip bug #23574

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind commented Mar 19, 2023 •

edited

Loading

wxiaoguang commented Mar 19, 2023

wxiaoguang commented Mar 19, 2023

silverwind commented Mar 19, 2023

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind commented Mar 19, 2023 •

edited

Loading

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind Mar 19, 2023

wxiaoguang Mar 20, 2023 •

edited

Loading

silverwind Mar 20, 2023

wxiaoguang Mar 20, 2023

silverwind Mar 20, 2023

wxiaoguang Mar 20, 2023

silverwind Mar 20, 2023

wxiaoguang Mar 20, 2023

silverwind Mar 20, 2023

silverwind commented Mar 19, 2023 •

edited

Loading

HesterG commented Mar 20, 2023 •

edited

Loading

wxiaoguang commented Mar 20, 2023

codecov-commenter commented Mar 20, 2023 •

edited

Loading

wxiaoguang commented Mar 23, 2023 •

edited

Loading

wxiaoguang commented Mar 23, 2023

	<div class="ui label">
	<!-- TODO: Convert to tooltip once we can use tooltips in Vue templates -->
	{{ mergeForm.textClearMergeMessageHint }}
	</div>

Use a general approach to show tooltip, fix temporary tooltip bug #23574

Use a general approach to show tooltip, fix temporary tooltip bug #23574

Conversation

wxiaoguang commented Mar 19, 2023 • edited Loading

TLDR

Details

The performance

The general approach for all tooltips

Temporary tooltip re-entrance bug

Other changes

silverwind commented Mar 19, 2023 • edited Loading

wxiaoguang commented Mar 19, 2023

wxiaoguang commented Mar 19, 2023

silverwind commented Mar 19, 2023

wxiaoguang commented Mar 19, 2023 • edited Loading

silverwind commented Mar 19, 2023 • edited Loading

wxiaoguang commented Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Mar 19, 2023 • edited Loading

HesterG commented Mar 20, 2023 • edited Loading

wxiaoguang commented Mar 20, 2023

codecov-commenter commented Mar 20, 2023 • edited Loading

Codecov Report

wxiaoguang commented Mar 23, 2023 • edited Loading

wxiaoguang commented Mar 23, 2023

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind commented Mar 19, 2023 •

edited

Loading

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind commented Mar 19, 2023 •

edited

Loading

wxiaoguang commented Mar 19, 2023 •

edited

Loading

wxiaoguang Mar 20, 2023 •

edited

Loading

silverwind commented Mar 19, 2023 •

edited

Loading

HesterG commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading

wxiaoguang commented Mar 23, 2023 •

edited

Loading