show parameters of a run with readOnlyMode#10941
Conversation
also removes the injected inline javascript for checkboxes with `readonly` attribute set.
timja
left a comment
There was a problem hiding this comment.
LGTM
/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!
| onclick="${attrs.readonly=='true' ? null : attrs.onclick}" id="${attrs.id}" class="${attrs.class} ${attrs.negative!=null ? 'negative' : null} ${attrs.checkUrl!=null?'validated':''}" | ||
| checkUrl="${attrs.checkUrl}" checkDependsOn="${attrs.checkDependsOn}" json="${attrs.json}" | ||
| disabled="${readOnlyMode ? 'true' : null}" | ||
| disabled="${readOnlyMode or attrs.readonly=='true' ? 'true' : null}" |
There was a problem hiding this comment.
#8420 (comment) points out this changes the semantics of form submission. Could this cause trouble?
CC @zbynek as comment author
There was a problem hiding this comment.
I checked where checkboxes with readonly="true" are used. I found 6 plugins:
- artifact-manager-s3: 2 places the underlying fields are final and the value is set from a system property -> no problem
- admin-params: used to show a parameter value in the parameters page of a run -> no problem
- google-compute-egine: the underlying field is static final -> no problem
- agent-maintenance: also sets
readOnlyMode-> no problem - nodejs: used in
show-config.jellythis only shows the config and should not be editable anyway -> no problem - matrix-combinations: used to show a parameter value in the parameters page of a run -> no problem
There was a problem hiding this comment.
Did you check with a screen reader?
There was a problem hiding this comment.
ahh yes that could be a problem.
But then any place where we set readOnlyMode that leads to a disabled input is problematic. E.g. in radio.jelly, radioBlock.jelly toggleSwitch.jelly, enum.jelly
Display the parameters that have been used for a run with
readOnlyModeinstead of using the readonly attribute. This is the recommended way.Also removes the inline javascript when a checkbox sets
readonly="true". This is only used in very few places and should not have an effect on plugins. A f:checkbox that still usesreadonlywill then be rendereddisabledBefore:

After:

Testing done
Interactive testing
Proposed changelog entries
Proposed changelog category
/label rfe,web-ui
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered (see query).