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.

Sign up for GitHub

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

Jump to bottom

Actually adjust copy button logic to new API #8554

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Actually adjust copy button logic to new API #8554

merged 3 commits into from
Oct 6, 2023

Conversation

D3SOX
Copy link
Contributor

@D3SOX D3SOX commented Oct 2, 2023 •
edited by MarkEWaite
Loading

The previous code (introduced in #6698) contained obsolete logic from the deprecated clipboard API, which was removed here.

The Promise returned by navigator.clipboard.writeText is now correctly handled as described per https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText#return_value

I've also removed the check if it's running as test as HTMLUnit 3.1.0 added support for isSecureContext

Testing done

I have overwritten the function with the console to always return a rejecting Promise. Therefore, the new logic is tested
image

Proposed changelog entries

I think this does not need to be mentioned in the changelog

Alternatives I have considered

We could implement the deprecated clipboard API again when we are not in a secure context

Additional testing done

  • @MarkEWaite confirm that the copy button behaves as expected with a build of this pull request

@welcome
Copy link

welcome bot commented Oct 2, 2023

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.

@D3SOX
Actually adjust copy button logic to new API
ee591a3 
Remove the obsolete textarea creation and correctly handle other permission errors
@D3SOX
Remove obsolete check if running as test
e257b59 
HTMLUnit now supports `isSecureContext`
@NotMyFault NotMyFault added the skip-changelog Should not be shown in the changelog label Oct 3, 2023
@NotMyFault NotMyFault requested review from a team October 3, 2023 09:03
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Oct 3, 2023
yaroslavafenkin
yaroslavafenkin reviewed Oct 3, 2023
View reviewed changes
Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Looks OK from security POV.

@yaroslavafenkin yaroslavafenkin added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Oct 3, 2023
@NotMyFault NotMyFault requested a review from a team October 3, 2023 21:02
@MarkEWaite MarkEWaite self-assigned this Oct 4, 2023
MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Oct 4, 2023
@MarkEWaite
Test the copy button logic improvement in core
f12297a 
HTMLUnit 3 should already be implemented in all the plugins that are
included in the bill of materials, but it is better to verify in the
plugin bill of materials now than to discover it with a broken build of
the plugin BOM at release time.

Checking

* jenkinsci/jenkins#8554

Thanks to @D3SOX for the pull request proposing the improvement.
@MarkEWaite MarkEWaite mentioned this pull request Oct 4, 2023
Closed
6 tasks
@MarkEWaite
Copy link
Contributor

Thanks very much @D3SOX . I've submitted a verification test with the plugin bill of materials to confirm that this improvement does not surprise us in the bill of materials release. Pull request is:

MarkEWaite
MarkEWaite approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

I tested the copy button on an SSL enabled Jenkins behind an NGINX reverse proxy and it worked great. Approved. Thanks very much.

Tests also ran as expected on the Jenkins plugin bill of materials. No test failures reported.

@timja
Copy link
Member

timja commented Oct 5, 2023

/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 Oct 5, 2023
@MarkEWaite MarkEWaite merged commit b29b457 into jenkinsci:master Oct 6, 2023
16 checks passed
@welcome
Copy link

welcome bot commented Oct 6, 2023

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.

jglick
jglick reviewed Dec 21, 2023
View reviewed changes
core/src/main/resources/lib/layout/copyButton/copyButton.js
@@ -4,31 +4,20 @@ Behaviour.specify(
0,
function (copyButton) {
copyButton.addEventListener("click", () => {
// HTMLUnit 2.70.0 does not recognize isSecureContext
Copy link
Member

@jglick jglick Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

introduced in #7784 FTR; see HtmlUnit/htmlunit@54d9d30

jtnord
jtnord reviewed Dec 21, 2023
View reviewed changes
core/src/main/resources/lib/layout/copyButton/copyButton.js
hoverNotification(copyButton.getAttribute("message"), copyButton);
if (isSecureContext) {
// Copy the text to the clipboard
navigator.clipboard
Copy link
Member

@jtnord jtnord Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

whilst mainstream browsers support this (according to canIUse) I believe unless you set a handler in HtmlUnit that navigator.clipboard will be undefined when running tests with webclient.

the webclient created in j-t-h is not configured with a handler.

@jglick jglick mentioned this pull request Dec 21, 2023
Merged
@anhcraft anhcraft mentioned this pull request Feb 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaroslavafenkin yaroslavafenkin yaroslavafenkin left review comments

@jtnord jtnord jtnord left review comments

@jglick jglick jglick left review comments

@timja timja timja approved these changes

@MarkEWaite MarkEWaite MarkEWaite approved these changes

Assignees

@MarkEWaite MarkEWaite

Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@D3SOX @MarkEWaite @timja @jglick @jtnord @yaroslavafenkin @NotMyFault