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-73971 Extract event handlers in NestedViewsSearch/search-results #48

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

judovana
Copy link
Contributor

@judovana judovana requested a review from a team as a code owner October 16, 2024 12:05
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

This is really great, as it gets us halfway to the finish line. To complete this task, we need to remove the onclick= HTML attribute, and instead set it programmatically from searchscripts.js. To do that, you'll need to use either document.addEventListener('DOMContentLoaded', …) or Behaviour#specify (as described in https://www.jenkins.io/doc/developer/security/csp/#inline-event-handlers) to write some code that runs when the page loads. That code should find the <a> elements whose onclick handlers were removed (e.g. through document.querySelector and iterating by class name or ID), then add a new onclick handler with element.addListener("click", ...). For an example, see:

jenkinsci/scriptler-plugin#120

Once that is done and the result is tested, I am happy to approve this PR.

@@ -0,0 +1,17 @@
// <![CDATA[
Copy link
Member

Choose a reason for hiding this comment

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

CDATA is not needed here. or on line 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you insists, I will keep it there. I like when code is somehow wrapped, and on contrary I do nt ot like the JS's myNameSapce = function () { notation... Anyway.. Yah my bad habit:(

Copy link
Member

Choose a reason for hiding this comment

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

OK, no big deal either way.

@judovana
Copy link
Contributor Author

This is really great, as it gets us halfway to the finish line. To complete this task, we need to remove the onclick= HTML attribute, and instead set it programmatically from searchscripts.js. To do that, you'll need to use either document.addEventListener('DOMContentLoaded', …) or Behaviour#specify (as described in https://www.jenkins.io/doc/developer/security/csp/#inline-event-handlers) to write some code that runs when the page loads. That code should find the <a> elements whose onclick handlers were removed (e.g. through document.querySelector and iterating by class name or ID), then add a new onclick handler with element.addListener("click", ...). For an example, see:

jenkinsci/scriptler-plugin#120

Once that is done and the result is tested, I am happy to approve this PR.

Ah sure. Will do.

moving adjunct after creation of elements
@judovana
Copy link
Contributor Author

@basil done sir!

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

This is looking almost done. The only task I think is remaining is to follow this advice from the CSP documentation:

For event handlers like onclick that used to call return false to prevent the usual action (e.g. link navigation) from happening, add a call to Event.preventDefault() in a separate event handler on the provided Event argument.

For an example, see:

jenkinsci/workflow-job-plugin#477

After that is done, I think this change is ready to ship.

@judovana
Copy link
Contributor Author

This is quite educational PR. TY!

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I trust you can confirm this is all working as expected by manually testing the UI.

@judovana
Copy link
Contributor Author

yup, I did. In adition unittests did its job susprisngly well. ty!

@judovana judovana merged commit 1aea594 into master Oct 18, 2024
17 checks passed
@basil basil deleted the jsNewFile branch October 18, 2024 19:40
@basil
Copy link
Member

basil commented Oct 18, 2024

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants