Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-72845]: Fix status icon animation display on Safari #9123

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

Lmh-java
Copy link
Contributor

@Lmh-java Lmh-java commented Apr 4, 2024

See JENKINS-72845.

The status icon animations are not properly shown on Safari if the user zooms in/out (Command + "+" or "-"). Some bugs related to the Safari browser were reported a few years ago, and it seems this is still not fixed. The main issue is that Safari cannot properly calculate the center value of the property transform-origin, it behaves differently between Safari and Chrome.

By refactoring the structure of SVG icons and using SVG animations instead of CSS animations, this display problem is solved.
I re-made these icon SVGs and centerized every SVG component to an enlarged view window. Therefore, we can directly set transform-origin: 0 0; to make sure the origin pivot is not changing in any cases, so Safari can properly locate the animation.

PS: This is my first time contributing to Jenkins. If I am doing something wrong, please bear with me. Any advice is truly appreciated. :)

Screenshot before:

Before

This is just an example of one status icon. All other animations are similar.
It also happens in the build history section and the timeline section.

Screenshot after:

After

All occurrences of this are fixed.

Testing done

  1. Manual testing on Safari 16.5.1, Chrome 123.0.6312.106.
  2. Lightmode unit tests

Proposed changelog entries

  • Fix status icon animation display on Safari.

Proposed upgrade guidelines

N/A

Submitter checklist

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

Maintainer checklist

Copy link

welcome bot commented Apr 4, 2024

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

@daniel-beck
Copy link
Member

Thanks!

Does this mean

/* Unknown */
[data-symbol-animation] {
animation: spin 1s linear infinite;
transform-origin: center;
@media (prefers-reduced-motion) {
animation-duration: 3s;
}
}
@keyframes spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}
@keyframes spin-reverse {
from {
transform: rotate(360deg);
}
to {
transform: rotate(0deg);
}
}
could be deleted? That also references reduced motion, is this something that is being removed with this change?

@daniel-beck daniel-beck added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Apr 4, 2024
@NotMyFault NotMyFault requested a review from janfaracik April 4, 2024 16:34
@Lmh-java
Copy link
Contributor Author

Lmh-java commented Apr 4, 2024

Thanks!

Does this mean

/* Unknown */
[data-symbol-animation] {
animation: spin 1s linear infinite;
transform-origin: center;
@media (prefers-reduced-motion) {
animation-duration: 3s;
}
}
@keyframes spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}
@keyframes spin-reverse {
from {
transform: rotate(360deg);
}
to {
transform: rotate(0deg);
}
}

could be deleted? That also references reduced motion, is this something that is being removed with this change?

Hi! I didn't notice prefers-reduced-motion at first glance, I will aim to remove this with this PR as well :).

@Lmh-java
Copy link
Contributor Author

Lmh-java commented Apr 5, 2024

Thanks!

Does this mean

/* Unknown */
[data-symbol-animation] {
animation: spin 1s linear infinite;
transform-origin: center;
@media (prefers-reduced-motion) {
animation-duration: 3s;
}
}
@keyframes spin {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}
@keyframes spin-reverse {
from {
transform: rotate(360deg);
}
to {
transform: rotate(0deg);
}
}

could be deleted? That also references reduced motion, is this something that is being removed with this change?

Solved by a new method, please take a look :). In the new solution, I am still using the css properties to animate, so no need to remove the original css. The new solution also supports reduced motion mode.

@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Apr 6, 2024
@NotMyFault NotMyFault requested review from a team and removed request for janfaracik April 7, 2024 08:52
@timja timja changed the title [Jenkins-72845]: Fix status icon animation display on Safari [JENKINS-72845]: Fix status icon animation display on Safari Apr 7, 2024
@Lmh-java
Copy link
Contributor Author

@timja @daniel-beck Please take a look when you have time.

@daniel-beck daniel-beck self-requested a review April 20, 2024 22:20
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Looking at the "Icon legend" view/popup, this seems to work well in Safari, Chrome and Firefox now. I don't know enough about SVG to judge the technical quality of the fix or predict other problems this might cause.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

#9123 (review)
Same, but this fix has been sitting here long enough and if it works lets go for it.


/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 22, 2024
@daniel-beck daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Apr 22, 2024
@Lmh-java
Copy link
Contributor Author

Thank you for both of your responses.

Here is a little more explanation of this change.

This animation is a rotating around one pivot. Specifically, a part of the svg image is rotating around that pivot. I noticed that if we zoom in/out in safari, the new pivot position will not be re-calculated. Therefore, it will still rotate around the old position. This seems to be a bug with Safari since many years ago. To fix this problem, I mapped all the coordinates of the original svg to a new coordinate system, which centered at (0, 0). Therefore, if we zoom in/out, the position of the pivot will not be changed. Without re-calculating the new coordinate, this animation will not shift.

@NotMyFault NotMyFault merged commit 5d05f1e into jenkinsci:master Apr 25, 2024
17 checks passed
Copy link

welcome bot commented Apr 25, 2024

Congratulations on getting your very first Jenkins core pull request merged 🎉🥳

This is a fantastic achievement, and we're thrilled to have you as part of our community! Thank you for your valuable input, and we look forward to seeing more of your contributions in the future!

We would like to invite you to join the community chats and forums to meet other Jenkins contributors 😊
Don't forget to check out the participation page to learn more about how to contribute to Jenkins.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases squash-merge-me Unclean or useless commit history, should be merged only with squash-merge web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants