jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Replace YUI tooltips with Tippy.js

Open janfaracik opened this issue 2 years ago • 60 comments

This is the first of two PRs which intend to replace the existing YUI implementation of tooltips and menus with Tippy.js. This PR focuses solely on tooltips and brings native support for Tippy.js to Jenkins.

https://user-images.githubusercontent.com/43062514/160298962-5e39f965-a868-4e0a-bc3a-78bd68fe0481.mov

This brings several advantages:

  • Better experience for users, animations, better placement engine
  • Cleaner and better defined codebase
  • Tippy is far more flexible than YUI, so we can be more creative with our designs
  • Far easier to find support online
  • YUI 2 is also abandoned since 2011

This implementation intends to maintain 100% backwards compatibility with the existing implementation of tooltips, it also introduces a new way to host HTML content in tooltips which is used for the project status tooltip using html-tooltip=" ... "

This change is documented in the updated Jenkins Design Library plugin.

Proposed changelog entries

  • Update appearance and framework for tooltips

For plugin developers

  • For developers that are using Tippy.js as part of their plugin, they can now remove the dependency and instead rely on the Jenkins implementations (tooltip="hello world" or html-tooltip="<p>hello world</p>")

Proposed upgrade guidelines

N/A

Submitter checklist

  • [ ] (If applicable) Jira issue is well described
  • [ ] Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • [ ] Appropriate autotests or explanation to why this change has no tests
  • [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • [ ] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • [ ] There are at least 2 approvals for the pull request and no outstanding requests for change
  • [ ] Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • [ ] Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • [ ] Proper changelog labels are set so that the changelog can be generated automatically
  • [ ] If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

janfaracik avatar Mar 27 '22 20:03 janfaracik

  • HTML doesn't seem to be handled properly here. I'm not sure if this is a special occasion, because the tooltip is provided as translation string in a .properties file, where <br> is not escaped at all:
  • Tooltips are shown twice on this maven project if it's still building
  • The history widget displays an empty tooltip if the job didn't complete yet to show the overall time it took:

This brings several advantages:

Also worth to note that YUI 3 is abandoned since 2014.

Great finds! They should all be fixed now :)

janfaracik avatar Mar 27 '22 22:03 janfaracik

YUI 3 is also abandoned since 2014

Wait, why do we care? We're on 2.x 😎 :trollface:

daniel-beck avatar Mar 28 '22 12:03 daniel-beck

HTML doesn't seem to be handled properly here.

IMO we should consider making all tooltips plain text if we can get away with it. They've been a common source of XSS issues in the past. Of course, some minimal formatting ability may need to remain, like newlines.

CC @Wadeck

daniel-beck avatar Mar 28 '22 12:03 daniel-beck

YUI 3 is also abandoned since 2014

Wait, why do we care? We're on 2.x 😎 :trollface:

From 2011 by the looks of it not that I can find the source of the latest upgrade, 2.9.0 doesn't seem to be in the repo

timja avatar Mar 28 '22 12:03 timja

HTML doesn't seem to be handled properly here.

IMO we should consider making all tooltips plain text if we can get away with it. They've been a common source of XSS issues in the past. Of course, some minimal formatting ability may need to remain, like newlines.

CC @Wadeck

Looking at the code only <br> has been added by default.

You can opt-in to html processing with html-tooltip.

timja avatar Mar 28 '22 12:03 timja

  • I'm not sure if this is intended, but the warnings-ng plugin overview in columns does now unfold into the column itself stretching it a lot, rather than floating around somewhere near the actual issue # it introduces.

Maybe my fix in https://github.com/jenkinsci/warnings-ng-plugin/pull/1206 helps. I did not yet release it because it bumps the LTS version to the latest one but hopefully that should work out.

Additionally: we need to check compatibility with the plugin views that are using Bootstrap since they depend on Popper as well. We need to make sure that Popper is just included once in our pages.

uhafner avatar Mar 28 '22 19:03 uhafner

  • I'm not sure if this is intended, but the warnings-ng plugin overview in columns does now unfold into the column itself stretching it a lot, rather than floating around somewhere near the actual issue # it introduces.

Maybe my fix in jenkinsci/warnings-ng-plugin#1206 helps. I did not yet release it because it bumps the LTS version to the latest one but hopefully that should work out.

It solely utilizes the modernized table view (in a sharp box 👀) but the position didn't change: Yet on the other hand, analyzer tooltips work out fine

NotMyFault avatar Mar 29 '22 20:03 NotMyFault

Repeatable element don't seem to be triggering tooltips:

image

image

timja avatar Mar 30 '22 08:03 timja

YUI currently overrides title to the value of tooltip if it's set on an element, do we want to continue this behaviour going forward?

janfaracik avatar Mar 30 '22 22:03 janfaracik

  • I'm not sure if this is intended, but the warnings-ng plugin overview in columns does now unfold into the column itself stretching it a lot, rather than floating around somewhere near the actual issue # it introduces.

Maybe my fix in jenkinsci/warnings-ng-plugin#1206 helps. I did not yet release it because it bumps the LTS version to the latest one but hopefully that should work out.

Additionally: we need to check compatibility with the plugin views that are using Bootstrap since they depend on Popper as well. We need to make sure that Popper is just included once in our pages.

warnings-ng would need changes like this file https://github.com/jenkinsci/jenkins/blob/68b0aea0e62a1ca16dd88f9585778c25fcdda03d/core/src/main/resources/lib/hudson/buildHealth.jelly - essentially moving the table to its own var then setting html-tooltip to that value.

janfaracik avatar Mar 30 '22 22:03 janfaracik

If no agent is available, tooltips are stacked for every attempt to trigger a build: Though even if they're split into separate lines, it looks odd to have +1 message for every attempt, considering you don't enqueue several builds in that way, but that may be a different issue.

Would you mind rechecking this? I can't seem to reproduce it locally but I have made some changes to that area.

  • Pipeline steps' tooltips are no longer attached to the step

Opened a small PR to address that and a couple other tiny things, cheers 👍

https://github.com/jenkinsci/workflow-support-plugin/pull/158

janfaracik avatar Mar 30 '22 23:03 janfaracik

If no agent is available, tooltips are stacked for every attempt to trigger a build: Though even if they're split into separate lines, it looks odd to have +1 message for every attempt, considering you don't enqueue several builds in that way, but that may be a different issue. Would you mind rechecking this? I can't seem to reproduce it locally but I have made some changes to that area.

New entries are appended properly now, I attached the steps below demonstrating what I mean by "+1 message for every attempt".

https://user-images.githubusercontent.com/13383509/161003876-deebd727-3047-40d0-b403-45debc8dee84.mp4

NotMyFault avatar Mar 31 '22 07:03 NotMyFault

If no agent is available, tooltips are stacked for every attempt to trigger a build: Though even if they're split into separate lines, it looks odd to have +1 message for every attempt, considering you don't enqueue several builds in that way, but that may be a different issue. Would you mind rechecking this? I can't seem to reproduce it locally but I have made some changes to that area.

New entries are appended properly now, I attached the steps below demonstrating what I mean by "+1 message for every attempt".

2022-03-31.09-39-12.mp4

Ah seems like an existing issue - managed to reproduce it on master.

image

:P

janfaracik avatar Mar 31 '22 22:03 janfaracik

I saw that the tooltips have a background slightly transparent. In some cases, it could make it harder to see the content of the tooltip, no?

Screenshot from 2022-04-05 13-49-55

In this example, I had to look a bit more closely than on other places.

alecharp avatar Apr 05 '22 11:04 alecharp

I saw that the tooltips have a background slightly transparent. In some cases, it could make it harder to see the content of the tooltip, no?

Screenshot from 2022-04-05 13-49-55

In this example, I had to look a bit more closely than on other places.

Are you using Firefox by any chance? I'm using backdrop-filter to blur anything behind the tooltips but I don't think Firefox supports it atm, I'll put in a fix for that.

janfaracik avatar Apr 05 '22 13:04 janfaracik

Are you using Firefox by any chance?

yes, Firefox 98.0. Thanks.

alecharp avatar Apr 05 '22 13:04 alecharp

New entries are appended properly now, I attached the steps below demonstrating what I mean by "+1 message for every attempt". 2022-03-31.09-39-12.mp4

Ah seems like an existing issue - managed to reproduce it on master.

Deliberate; a build can have any number of causes. Usually it is more useful than this, e.g. when it's triggered by the cron schedule but while it's in the queue, you click "Build Now", it's triggered both by the schedule and by you (and you probably accelerated its execution, as Build Now does not wait in the queue if it doesn't have to).

daniel-beck avatar Apr 08 '22 09:04 daniel-beck

Considering the PR is in a solid state, I fired up ATH and BOM tests: 🔴 ATH shows various failures related to YUI: https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/PR-759/15/ 🟢 BOM fails too, but Jesse already filed a PR to matrix-auth to take care of that, that's not related to this PR: https://ci.jenkins.io/job/Tools/job/bom/job/PR-992/1/

NotMyFault avatar Apr 11 '22 09:04 NotMyFault

For plugin developers

This should mention the need to register tooltips manually as done in the linked matrix-auth PR.

daniel-beck avatar May 02 '22 19:05 daniel-beck

For plugin developers

This should mention the need to register tooltips manually as done in the linked matrix-auth PR.

Added

timja avatar May 02 '22 19:05 timja

This change is documented in the updated Jenkins Design Library plugin.

html-tooltip is not documented there currently, I think it was backed out of the main PR to only show released content

timja avatar May 02 '22 19:05 timja

This change is documented in the updated Jenkins Design Library plugin.

html-tooltip is not documented there currently, I think it was backed out of the main PR to only show released content

I'll add it back 👍

janfaracik avatar May 02 '22 19:05 janfaracik

This change is documented in the updated Jenkins Design Library plugin.

html-tooltip is not documented there currently, I think it was backed out of the main PR to only show released content

I'll add it back 👍

Could you mention the need to think about the source of the content and the need to potentially sanitize parts (perhaps even with example)? Thanks :)

daniel-beck avatar May 02 '22 20:05 daniel-beck

Could you mention the need to think about the source of the content and the need to potentially sanitize parts (perhaps even with example)? Thanks :)

The warning was left on the page when the docs were removed 😆, see https://weekly.ci.jenkins.io/design-library/Tooltips/

timja avatar May 02 '22 20:05 timja

Tooltips being interactable makes UI like the the permission grid of matrix-auth annoying to use. Move the cursor up a row and instead of being able to interact with checkboxes, you can copy & paste the text of the tooltip. This seems like a niche use case, so could we disable this behavior?

Before

https://user-images.githubusercontent.com/1831569/166409789-08d8d22d-4ce6-413b-abd8-0120f47cc3d5.mov

After

https://user-images.githubusercontent.com/1831569/166409799-b151db79-949d-4816-bbb4-0a04e066fd6a.mov

daniel-beck avatar May 03 '22 05:05 daniel-beck

I cannot get the tooltip on the matrix-auth configuration to work on Firefox 100 now. To be specific, videos from @daniel-beck on https://github.com/jenkinsci/jenkins/pull/6408#issuecomment-1115761460 are not what I have locally. I can see the <td/> have data-tooltip-{enabled,disabled} and the <input/> have a tooltip attribute, but there is no tooltip. There is no errors/warnings in the javascript console.

alecharp avatar May 25 '22 07:05 alecharp

@alecharp are you running with this PR? https://github.com/jenkinsci/matrix-auth-plugin/pull/120

timja avatar May 25 '22 08:05 timja

You are correct @timja, with the correct version of the plugin it's working fine. I missed that we need downstream pull requests.

alecharp avatar May 25 '22 17:05 alecharp

Are there plans to look for plugins expecting to have HTML support in tooltips, and adapting the more popular ones? I know of https://plugins.jenkins.io/compact-columns/ (not that popular but an example of a resulting regression) which relies on this:

Screenshot 2022-05-31 at 11 19 34

Similarly, like in matrix-auth, the opposite situation may exist where content is escaped, expecting it to be interpreted as HTML, and then it's double-escaped.

Non-blocking (splitting HTML and non-HTML explicitly should prevent some problems in the future), just curious.

daniel-beck avatar May 31 '22 09:05 daniel-beck

Are there plans to look for plugins expecting to have HTML support in tooltips, and adapting the more popular ones? I know of https://plugins.jenkins.io/compact-columns/ (not that popular but an example of a resulting regression) which relies on this:

Screenshot 2022-05-31 at 11 19 34

Similarly, like in matrix-auth, the opposite situation may exist where content is escaped, expecting it to be interpreted as HTML, and then it's double-escaped.

Non-blocking (splitting HTML and non-HTML explicitly should prevent some problems in the future), just curious.

There's nothing concrete planned but I'm more than happy to take a look at/fix any issues that arise with plugins. I'll take a look at that one now.


Update: Opened a PR here for that: https://github.com/jenkinsci/compact-columns-plugin/pull/52

janfaracik avatar May 31 '22 09:05 janfaracik