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-73563] Create a jenkins-button instead of a YUI button in makeButton #9604

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Aug 12, 2024

This reverts the revert of #9511
It avoids the usage of classList.forEach which is not yet support in htmlunit and causes pct to fail

See JENKINS-73563

makeButton is one of the last places in core were YUI is used. This method is called by a few plugins directly but also when plugins create inputs of type button or submit with class submit-button or yui-button. Frequently they then also attach an onclick handler.

This change will replace the input with a button with class jenkins-button on the fly.
Looking at the code of several plugins that rely on the behaviour it seems they will not be affected by this change (tested with shelve-plugin)

credentials plugin works with the returned YUI object and try to set the button to disabled or get the associated form, for this case a small wrapper is added that implements the 2 methods and credentials plugin still works without problems.
There is PR open for credentials that cleans up the code and removes yui usage
(Another usage affects the multi-slave-config plugin which is deprecated and not usable anyway due to prototype usage)

See https://docs.google.com/spreadsheets/d/1UjvtFmNmEdjMN5DUoFxJfBryA8q-E5_HwOzVKbVG9b0/edit?usp=sharing for a complete list of plugins that use makeButton directly or indirectly.

Need to test if ath needs to be adjusted

Testing done

Manual testing

Proposed changelog entries

  • makeButton creates jenkins-buttons on the fly instead of using YUI.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

N/A

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

Maintainer checklist

mawinter69 and others added 8 commits July 30, 2024 19:09
makeButton is one of the last places in core were YUI is used.
This method is called by a few plugins directly but also when plugins
create inputs of type button with class `submit-button` or `yui-button`.
Frequently they then also attach an onclick handler.

This change will replace the input with a button with class
`jenkins-button` on the fly.
Looking at the code of several plugins that rely on the behaviour it
seems they will not be affected by this change.
Co-authored-by: Kevin Guerroudj <[email protected]>
forEach on classList is not supported yet by htmlunit
@mawinter69 mawinter69 changed the title Make button [JENKINS-73563] create a jenkins-button instead of a yui button in makeButton Aug 12, 2024
@mawinter69
Copy link
Contributor Author

mawinter69 commented Aug 12, 2024

Tested locally that

  • hudson.security.LDAPEmbeddedTest#validateUI
  • org.jenkinsci.plugins.docker.commons.credentials.DockerServerDomainSpecificationTest#configRoundTrip

succeed now

@MarkEWaite MarkEWaite added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Aug 12, 2024
@basil basil removed the needs-security-review Awaiting review by a security team member label Aug 13, 2024
@basil basil changed the title [JENKINS-73563] create a jenkins-button instead of a yui button in makeButton [JENKINS-73563] Create a jenkins-button instead of a YUI button in makeButton Aug 13, 2024
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 quick fix, @mawinter69! I verified that the two previously failing tests are now passing in BOM.

Since the only difference between this and #9511 is ca6462c and since #9511 had security approval, I see no need for a second security review. Let's get this integrated toward 2.472 as originally planned.

@basil basil merged commit f615612 into jenkinsci:master Aug 13, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted 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.

3 participants