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-71252][JENKINS-70793] Multiple form validation fixes #8422

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Aug 23, 2023

See JENKINS-71252 and JENKINS-70793.

(The fix for JENKINS-70793 only works with the fix for JENKINS-71252.)

JENKINS-71252

Per https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetHeight,

If the element is hidden (for example, by setting style.display on the element or one of its ancestors to "none"), then 0 is returned.

Requesting review by @janfaracik to understand whether there's any reason to use this over auto.

JENKINS-70793

https://github.com/jenkinsci/jenkins/pull/6460/files#diff-10922aee3260853be495a09cf3bf13bf5f931bc128e334807e3008ff00b57651R523-R528 looked sus, so I deleted it.

https://github.com/jenkinsci/jenkins/pull/6460/files#r862952504 looks a little concerning, but I was unable to reproduce that with sample plugins. Requesting @timja review just in case.

Testing done

Followed the steps to reproduce in my Jira comment to JENKINS-71252.

Installed @yaroslavafenkin's demo plugin for JENKINS-71252 and confirmed it behaved correctly.

Patched the demo plugin JENKINS-71252 to have a stack trace like described in JENKINS-70793, and tested.

Proposed changelog entries

  • Show form validation results for form elements that are initially hidden. (regression in 2.355)
  • Remove previous form validation errors when the form validation is updated with new content. (regression in 2.355)

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@daniel-beck daniel-beck added regression-fix Pull request that fixes a regression in one of the previous Jenkins releases bug For changelog: Minor bug. Will be listed after features labels Aug 23, 2023
@daniel-beck daniel-beck requested a review from janfaracik August 23, 2023 11:30
@daniel-beck daniel-beck changed the title [JENKINS-71252] Show form validation for hidden elements [JENKINS-71252][JENKINS-70793] Multiple form validation fixes Aug 23, 2023
validationArea.after(element);
});
}
validationArea.style.height = "auto";

Behaviour.applySubtree(validationArea);
// For errors with additional details, apply the subtree to the expandable details pane
Copy link
Member Author

@daniel-beck daniel-beck Aug 23, 2023

Choose a reason for hiding this comment

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

This line and following are probably obsolete now and can be deleted?

@daniel-beck daniel-beck requested a review from timja August 23, 2023 12:04
@MarkEWaite
Copy link
Contributor

@janfaracik we'll select the next LTS baseline this week. I think this looks like a very promising fix to consider including in Jenkins 2.426 weekly so that it can be included in the Nov 15, 2023 LTS release. Would you be willing to review it?

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.

Looks sensible

@timja
Copy link
Member

timja commented Oct 3, 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 3, 2023
@MarkEWaite MarkEWaite merged commit 012fa00 into jenkinsci:master Oct 5, 2023
MarkEWaite pushed a commit to MarkEWaite/jenkins that referenced this pull request Oct 29, 2023
…sci#8422)

* [JENKINS-71252] Show form validation for hidden elements

* [JENKINS-70793] Clear all validation output on update

---------

Co-authored-by: Daniel Beck <[email protected]>

(cherry picked from commit 012fa00)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants