JENKINS-65033 Run / Project summary - tables to divs #5351

timja · 2021-03-10T21:14:52Z

This required some minor adaption in a few plugins that had a block layout (should have been inline) next to the icon, see jenkinsci/warnings-ng-plugin#841, jenkinsci/code-coverage-api-plugin#185

This is a lot safer than forms as it won't break anything, possibly minor changes in some plugins, I've tested all the ones we use on ci.jenkins.io and https://plugins.jenkins.io/sauce-ondemand which was broken without this change due to tables nesting

Proposed changelog entries

Entry 1: JENKINS-65033, Run summary page now uses div for layout
Entry 2: Developer: divBasedRunLayout jelly variable available for detecting if Jenkins uses divs for run summary page
...

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

timja · 2021-03-10T21:53:37Z

core/src/main/java/hudson/Functions.java

@@ -304,6 +304,7 @@ public static void initPageVariables(JellyContext context) {
        context.setVariable("resURL",rootURL+getResourcePath());
        context.setVariable("imagesURL",rootURL+getResourcePath()+"/images");
        context.setVariable("divBasedFormLayout", true);
+        context.setVariable("divBasedRunLayout", true);


used in jenkinsci/workflow-job-plugin#180 to adapt to this change without requiring a core bump

uhafner

Since most of those forms (summary.jelly) start with a copy and paste operation I assume that there are a lot of additional plugins that need to be changed.

daniel-beck · 2021-03-10T22:14:58Z

Could we hold this a bit until we've closed out the previous t2d migration? It's barely in LTS, so I expect further regression reports etc.

res0nance · 2021-03-11T16:59:01Z

Could we hold this a bit until we've closed out the previous t2d migration? It's barely in LTS, so I expect further regression reports etc.

I agree with Daniel, there is likely to be more regressions.

Also there seems to be a failing test.

timja · 2021-03-11T17:07:33Z

Could we hold this a bit until we've closed out the previous t2d migration? It's barely in LTS, so I expect further regression reports etc.

I agree with Daniel, there is likely to be more regressions.

the scale for this is far smaller, we can wait a bit but doesn't need a year like the other one

Also there seems to be a failing test.

Yes aware, my IDE doesn't want to run the test without throwing nullpointers in unrelated places -.-

timja · 2021-03-11T20:47:50Z

core/src/main/resources/hudson/model/CauseAction/summary.jelly

@@ -25,13 +25,16 @@ THE SOFTWARE.
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
  <t:summary icon="orange-square.png">
+      ${%Caused by}


needed to add something to go next to the icon to make it looks okay,

any better wording or design thoughts?

Maybe a more explicit Build started by or Build caused by? Caused by seems fine as well

Not sure whats a good wording for it is.

Personally I'm not a fan of the icon but that's outside the scope of this PR and with the new icons coming in this might be updated at some point.

Looks fine to me.

fqueiruga

Code looks good. Same as above, I'd still suggest having a LTS line stable before going ahead with this.

rsandell · 2021-03-15T09:31:48Z

core/src/main/resources/hudson/model/AbstractBuild/index.jelly

@@ -107,9 +107,11 @@ THE SOFTWARE.

        <!-- give actions a chance to contribute summary item -->
        <j:forEach var="a" items="${it.allActions}">
-          <st:include page="summary.jelly" from="${a}" optional="true" it="${a}" />
+          <div style="margin-top: 1em;">


I'm guessing quite a lot more of breaking in plugins.
Ex: https://github.com/jenkinsci/build-failure-analyzer-plugin/blob/master/src/main/resources/com/sonyericsson/jenkins/plugins/bfa/model/FailureCauseBuildAction/summary.groovy

Pretty sure I tested BFA and it rendered fine, can double check later

Job page, browser allows the table and it looks the same as before:

Run page, browser throws out the table tags, it looks fine except it's using a heading tag which is defined as a block element which moves it down a line

We could maybe set an adjacent sibling selector to make headings display inline if after an img tag or svg, any thoughts @fqueiruga ?

Doesn't look like a blocker anyway to me, thoughts?

Nah, that looks OK. Hopefully it won't break things rendered after it, though I don't think it will since it closes it's table tags.

jglick · 2021-03-15T12:22:54Z

core/src/main/resources/hudson/model/CauseAction/summary.jelly

@@ -25,13 +25,16 @@ THE SOFTWARE.
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
  <t:summary icon="orange-square.png">
+      ${%Caused by}


Looks fine to me.

oleg-nenashev

It will likely lead to some UI regressions, and it is highly likely we will need upgrade guidelines for that. Besides that, I think this is ready to go.

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

rsandell · 2021-03-23T08:56:42Z

core/src/main/resources/hudson/model/AbstractBuild/index.jelly

@@ -56,7 +56,7 @@ THE SOFTWARE.
        <t:editableDescription permission="${it.UPDATE}" />
      </div>

-      <table style="margin-top: 1em; margin-left:1em;">
+      <div>


Not sure how things are done with styling etc now in the modern times. But I think it would be nice if this div had an id for page decorators et.al. if they need to manipulate it somehow. Themes would also have it easier to style it I guess?

not really sure why you would need them.

themes should be able to use selectors and if they are finding a need for it they can add them at the time.

rsandell · 2021-03-23T08:57:34Z

core/src/main/resources/hudson/model/AbstractProject/main.jelly

@@ -30,17 +30,21 @@ THE SOFTWARE.

  <p:projectActionFloatingBox />

-  <table style="margin-top: 1em; margin-left:1em;">
+  <div style="margin-top: 1em; margin-left:1em;">


maybe this one too?

daniel-beck

A justification for a potential breaking change should be provided. None is given in this PR, or at least it's unclear: The description indicates that this fixes a plugin's UI. Was it always broken and this just makes it work (at what cost?)? Did we cause a regression (where?) and this is a regression fix?

Needs corresponding developer documentation.

I would also like to see evidence of a more exhaustive ecosystem search for incompatible plugins before this is merged, or an explanation why this would be an excessively rare occurrence.

Maybe I'm overly cautious here but we've had many regressions from config form t2d, some right here in core and in extremely popular plugins that were only discovered after merging, and I would not want to see this again.

daniel-beck · 2021-03-25T07:36:21Z

I found a few plugins that show a <t:pane>, which is a <div>, inside a <t:summary>. Is that a problem? I can't tell from the description of this PR and the examples provided.

timja · 2021-03-25T07:52:27Z

A justification for a potential breaking change should be provided.

Uses proper layout techniques, so browsers don't try fix markup if anyone does anything it doesn't like, table has very specific elements that are allowed as children and browsers will 'correct' your mistake often changing the pages in ways you don't want.
Fixed the sauce-ondemand plugin action which looks ok if it's the only action which is what the authors will have tested, but as you soon as you attach another action, say the test results from junit, and the page is wildly broken. Likely always broken or changed in browsers ages ago.

I found a few plugins that show a <t:pane>, which is a <div>, inside a <t:summary>. Is that a problem? I can't tell from the description of this PR and the examples provided.

I would expect those plugins to be troublesome already, they could already break the layout as that sounds like the scenario this PR is trying to fix.

Most likely they will behave fine after this as table is a valid child of div, whereas nested tables is not always valid.

What plugins?

daniel-beck · 2021-03-25T08:25:29Z

I would expect those plugins to be troublesome already, they could already break the layout as that sounds like the scenario this PR is trying to fix. … What plugins?

https://plugins.jenkins.io/groovy-label-assignment/ and https://plugins.jenkins.io/show-build-parameters/ have :panes.

https://plugins.jenkins.io/violations/ has both a <table> and a <div> inside a t:summary. https://plugins.jenkins.io/allure-jenkins-plugin/ has a <table>. https://plugins.jenkins.io/appetize/ has a <div>.

If <ul> is also a problem, then there's https://plugins.jenkins.io/cccc/ (see screenshot provided) and https://plugins.jenkins.io/buildcontext-capture/

timja · 2021-03-25T08:44:15Z

ul is fine, (it's used in this PR)

Markup here looks really weird, the same outcome would've been done with no table or anything =/, unless i'm missing something: https://github.com/jenkinsci/allure-plugin/blob/master/src/main/resources/ru/yandex/qatools/allure/jenkins/AllureBuildAction/summary.jelly#L3-L7, it would need checking, I can't tell from looking at it if it would render ok.

the pane ones are worth checking, the title will likely be moved onto a new line rather than adjacent to the image

Other than misunderstanding of html layout this will work: https://plugins.jenkins.io/appetize/, instead of br, div or p should be used to move it on the next line.

alecharp · 2022-03-29T16:20:41Z

@timja do you wish and have time to revive this PR as you said on #6307? At this point, the PR seems stalled or could be close. Sorry if I'm wrong.

timja · 2022-03-29T16:23:20Z

@timja do you wish and have time to revive this PR as you said on #6307? At this point, the PR seems stalled or could be close. Sorry if I'm wrong.

it will get done at some point, please leave it alone for now

timja · 2022-05-07T11:27:16Z

No plans to work on this atm

JENKINS-65033 Run summary - tables to divs

df91053

timja marked this pull request as ready for review March 10, 2021 21:44

timja mentioned this pull request Mar 10, 2021

JENKINS-65033 Run summary - tables to divs jenkinsci/workflow-job-plugin#180

Merged

6 tasks

timja commented Mar 10, 2021

View reviewed changes

timja 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 labels Mar 10, 2021

timja requested review from a team, fqueiruga and uhafner March 10, 2021 21:54

uhafner approved these changes Mar 10, 2021

View reviewed changes

Tweak CauseAction layout to render better (and fix test)

c9bf785

timja commented Mar 11, 2021

View reviewed changes

Extend to Project as well

a1b2e97

timja changed the title ~~JENKINS-65033 Run summary - tables to divs~~ JENKINS-65033 Run / Project summary - tables to divs Mar 12, 2021

fqueiruga approved these changes Mar 15, 2021

View reviewed changes

rsandell reviewed Mar 15, 2021

View reviewed changes

jglick approved these changes Mar 15, 2021

View reviewed changes

oleg-nenashev approved these changes Mar 23, 2021

View reviewed changes

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed labels Mar 23, 2021

oleg-nenashev requested a review from rsandell March 23, 2021 08:26

rsandell reviewed Mar 23, 2021

View reviewed changes

rsandell approved these changes Mar 23, 2021

View reviewed changes

daniel-beck requested changes Mar 23, 2021

View reviewed changes

daniel-beck added needs-docs and removed ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Mar 23, 2021

daniel-beck added the needs-justification This PR lacks a motivation for the proposed change. label Mar 23, 2021

timja removed the needs-justification This PR lacks a motivation for the proposed change. label Feb 25, 2022

timja mentioned this pull request Mar 15, 2022

Replace Tango icons with Jenkins Symbols #6307

Merged

11 tasks

alecharp added the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 29, 2022

timja closed this May 7, 2022

timja mentioned this pull request May 30, 2024

[JENKINS-73225] - Restore linking when multiple axes jenkinsci/matrix-project-plugin#199

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-65033 Run / Project summary - tables to divs #5351

JENKINS-65033 Run / Project summary - tables to divs #5351

timja commented Mar 10, 2021 •

edited

Loading

timja Mar 10, 2021

uhafner left a comment

daniel-beck commented Mar 10, 2021

res0nance commented Mar 11, 2021

timja commented Mar 11, 2021

timja Mar 11, 2021 •

edited

Loading

res0nance Mar 11, 2021

jglick Mar 15, 2021

fqueiruga left a comment

rsandell Mar 15, 2021

timja Mar 15, 2021

timja Mar 19, 2021 •

edited

Loading

rsandell Mar 23, 2021

jglick Mar 15, 2021

oleg-nenashev left a comment

rsandell Mar 23, 2021

timja Mar 23, 2021

rsandell Mar 23, 2021

daniel-beck left a comment •

edited

Loading

daniel-beck commented Mar 25, 2021 •

edited

Loading

timja commented Mar 25, 2021 •

edited

Loading

daniel-beck commented Mar 25, 2021

timja commented Mar 25, 2021

alecharp commented Mar 29, 2022

timja commented Mar 29, 2022

timja commented May 7, 2022

JENKINS-65033 Run / Project summary - tables to divs #5351

JENKINS-65033 Run / Project summary - tables to divs #5351

Conversation

timja commented Mar 10, 2021 • edited Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Choose a reason for hiding this comment

uhafner left a comment

Choose a reason for hiding this comment

daniel-beck commented Mar 10, 2021

res0nance commented Mar 11, 2021

timja commented Mar 11, 2021

timja Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqueiruga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck left a comment • edited Loading

Choose a reason for hiding this comment

daniel-beck commented Mar 25, 2021 • edited Loading

timja commented Mar 25, 2021 • edited Loading

daniel-beck commented Mar 25, 2021

timja commented Mar 25, 2021

alecharp commented Mar 29, 2022

timja commented Mar 29, 2022

timja commented May 7, 2022

timja commented Mar 10, 2021 •

edited

Loading

timja Mar 11, 2021 •

edited

Loading

timja Mar 19, 2021 •

edited

Loading

daniel-beck left a comment •

edited

Loading

daniel-beck commented Mar 25, 2021 •

edited

Loading

timja commented Mar 25, 2021 •

edited

Loading