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

[JENKINS-72136] Remove inline onclick handlers for generate directive button #678

Merged
merged 1 commit into from
Oct 27, 2023
Merged

[JENKINS-72136] Remove inline onclick handlers for generate directive button #678

merged 1 commit into from
Oct 27, 2023

Conversation

NeetigyaPod
Copy link
Contributor

  • JENKINS issue(s):
  • Description:
    • Inline handlers were showing in console. Used the recommendations given in the issue.
  • Documentation changes:
    • There is no feature change but just a minor change for security reasons.
  • Users/aliases to notify:
    • n/a

Testing:
Screenshots:
Before:
Screenshot from 2023-10-12 11-16-04
After:

Screenshot from 2023-10-12 11-17-57

mvn tests run and changes appear in mvn:hpi run

Kevin-CB
Kevin-CB reviewed Oct 16, 2023
View reviewed changes
Copy link

@Kevin-CB Kevin-CB 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 your changes locally, with the CSP plugins, and it works fine!
I'm not able to see the previous CSP violations!

...rces/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/index.jelly Outdated Show resolved Hide resolved
...rces/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/index.jelly Outdated Show resolved Hide resolved
...rces/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/index.jelly Outdated Show resolved Hide resolved
...s/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/indexScript.js Outdated Show resolved Hide resolved
@NeetigyaPod NeetigyaPod force-pushed the feat/remove-inline-directivegenerator branch from 13f816c to f18663b Compare October 16, 2023 11:30
@NeetigyaPod
Copy link
Contributor Author

Hi, that's great. Sorry for the extra spaces. Will take care in future contributions.

@NeetigyaPod
Copy link
Contributor Author

Hi @Kevin-CB can we merge this before end of October? I would like it to be a part of my hacktoberfest contribution.

@Kevin-CB
Copy link

Hi @NeetigyaPod, even though I approved this PR, I can't merge it as I'm not a maintainer of this plugin.

@NeetigyaPod
Copy link
Contributor Author

@jglick Can you please help me in this? Would appreciate a lot

@jglick jglick added the bug label Oct 25, 2023
jglick
jglick reviewed Oct 25, 2023
View reviewed changes
...s/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/indexScript.js
Comment on lines +13 to +14
// TODO JSON.stringify fails in some circumstances: https://gist.github.com/jglick/70ec4b15c1f628fdf2e9 due to Array.prototype.toJSON
// TODO simplify when Prototype.js is removed
Copy link
Member

Choose a reason for hiding this comment

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

likely needs to be revisited in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be done. Will work on it after this one

...rces/org/jenkinsci/plugins/pipeline/modeldefinition/generator/DirectiveGenerator/index.jelly
@@ -78,34 +78,10 @@
</f:dropdownList>
<j:set var="id" value="${h.generateId()}"/>
Copy link
Member

Choose a reason for hiding this comment

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

Could probably get rid of the generateId trick now, by having the button look for a sibling textarea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sibling in the sense- I can allocate a similar class name to the text area and remove all the mentions of the id variable?

Copy link
Member

Choose a reason for hiding this comment

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

Would have to look up some example, since I do not normally work on JS at all, but IIRC the point is that now that the behavior function is passed an element it would be able to use that argument to perform operations without a (unique) ID—either directly on the element in the argument, or on some sibling in the DOM.

In other words: when using inline <script> with a Jelly facet that might be included multiple times on a page, like the config snippet for a build step in a freestyle project config screen, you have the problem that

<body>
  <!-- first occurrence -->
  <div>
    <button onclick="handle()"/>
    <textarea id="spot"/>
    <script>
      function handle() {
        document.getElementById('spot').text = 'handled';
      }
    </script>
  </div>
</body>

will not work as soon as you add a second <div> with the same content: clicking either button will update the same textarea, perhaps the wrong one. generateId used to be used as a workaround:

<body>
  <!-- first occurrence -->
  <div>
    <j:set var="id" value="${h.generateId()}"/>
    <button onclick="handle()"/>
    <textarea id="spot-${id}"/>
    <script>
      function handle() {
        document.getElementById('spot-${id}').text = 'handled';
      }
    </script>
  </div>
</body>

When switching to a proper adjunct, you do not need this trick since

<body>
  <!-- first occurrence -->
  <div>
    <button class="mybutton"/>
    <textarea/>
    <st:adjunct …/>
  </div>
</body>
function handle(button) {
  button.getSiblingByType('textarea').text = 'handled';
}

will work regardless of how many copies there are. (All code above is pseudocode, actual method names etc. are going to be a bit different.)

Of course in many cases the Jelly fragment could never be included more than once in a page because of what it is for, so whether a unique id was even needed here to begin with, I am unsure; it may have been included “just in case”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhhh makes sense. Thank you, will treat it as nit for future contributions in Jenkins.

@jglick jglick changed the title feat:remove-inline onclick handlers for generate directive button Remove-inline onclick handlers for generate directive button Oct 25, 2023
@jglick jglick changed the title Remove-inline onclick handlers for generate directive button Remove inline onclick handlers for generate directive button Oct 25, 2023
@jglick jglick changed the title Remove inline onclick handlers for generate directive button [JENKINS-72136] Remove inline onclick handlers for generate directive button Oct 25, 2023
@jglick
Copy link
Member

jglick commented Oct 25, 2023

@Kevin-CB have you retested after the latest changes?

@Kevin-CB
Copy link

have you retested after the latest changes?

Yes, last time I tested, it was on f18663b

jglick
jglick approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@jglick jglick 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 at a glance (relying on other reviewers for details & testing).

@jglick jglick merged commit 4cfd891 into jenkinsci:master Oct 27, 2023
15 checks passed
@NeetigyaPod NeetigyaPod deleted the feat/remove-inline-directivegenerator branch October 28, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick approved these changes

@Kevin-CB Kevin-CB Kevin-CB approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
3 participants
@NeetigyaPod @Kevin-CB @jglick