Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-20772] Properly render response in case Apply results …
…in an error page.
  • Loading branch information
jglick committed Dec 23, 2013
1 parent 9e5817a commit 0e8195c
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions core/src/main/resources/lib/form/apply/apply.js
Expand Up @@ -41,6 +41,8 @@ Behaviour.specify("INPUT.apply-button", 'apply', 0, function (e) {
target.contentWindow.applyCompletionHandler(window);
} else {
// otherwise this is possibly an error from the server, so we need to render the whole content.
var doc = target.contentDocument || target.contentWindow.document;
$(containerId).appendChild(doc.getElementsByTagName('body')[0]);
var r = YAHOO.util.Dom.getClientRegion();
responseDialog.cfg.setProperty("width",r.width*3/4+"px");
responseDialog.cfg.setProperty("height",r.height*3/4+"px");
Expand Down

5 comments on commit 0e8195c

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

Would probably suffice to show just doc.getElementById("main-panel") for regular error dialogs. To make sure it's a regular error dialog, could wrap the error in core/src/main/resources/jenkins/model/Jenkins/oops.jelly in another div with id="error-description" attribute, and show just that using getElementById, but keep getElementsByTagName('body')[0] as fallback if no such element is found?

@jglick
Copy link
Member Author

@jglick jglick commented on 0e8195c Dec 23, 2013

Choose a reason for hiding this comment

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

Could make refinements such as these. My priority was to make sure something descriptive was displayed.

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion implemented in #1076

I don't know how to make the dialog scrollable though (issue already present with just this change as well, Firefox 26 on OS X 10.8.6). Any ideas? The problem is that for the test case you provided (invalid expression in freestyle job's cron trigger), you don't even get to see the wrapped AntlrExceptions that actually explain what's wrong.

@jglick
Copy link
Member Author

@jglick jglick commented on 0e8195c Dec 23, 2013

Choose a reason for hiding this comment

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

I don't know how to make the dialog scrollable though

Nor I. Beyond my knowledge of YUI.

@daniel-beck
Copy link
Member

Choose a reason for hiding this comment

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

Scrolling fixed in #1077. The container div had automatic size, so it was way larger than the dialog box and cut off by some YUI container element with overflow:hidden. This PR fixes that by defining a size slightly smaller than the dialog and an overflow:scroll style attribute.

Please sign in to comment.