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

Adjuncts do not include CSS correctly for removable dom fragments #23

Merged
merged 2 commits into from
Jan 26, 2015

Conversation

stephenc
Copy link
Member

  • I think it only works by accident with script tags
  • When the adjunct is initially included the script and css elements are injected at the point where the st:adjunct
    tag is first referenced.
  • Thus these tags form part of the DOM that can be removed (e.g. when part of a repeatable / hetero list)
  • Because the script is executed, removing the DOM node cannot un-execute the script, so script tags are safe
  • Removing the css's link tag, however, removes the styling... and the adjunct manager doesn't know.
  • What this hack does is inject the CSS at the end of the DOM's head tag (yes triggering a reflow) but only
    if the CSS link is not present already. The JavaScript tag (because we are bypassing st:adjunct) is conditionally
    appended to the DOM's body so that it can be fetched while other scripts continue executing.

To see the bug this fixes:

  • Create a job with the GIT SCM and a couple of SCM locations and some credential parameters
  • Save the job
  • Load the job
  • Remove the first credential parameter, now all the credential tags loose the styling (width:auto on the drop-down) and the Add button overflows to the next line

- I think it only works by accident with script tags
- When the adjunct is initially included the script and css elements are injected at the point where the st:adjunct
  tag is first referenced.
- Thus these tags form part of the DOM that can be removed (e.g. when part of a repeatable / hetero list)
- Because the script is executed, removing the DOM node cannot un-execute the script, so script tags are safe
- Removing the css's link tag, however, removes the styling... and the adjunct manager doesn't know.
- What this hack does is inject the CSS at the end of the DOM's head tag (yes triggering a reflow) but only
  if the CSS link is not present already. The JavaScript tag (because we are bypassing st:adjunct) is conditionally
  appended to the DOM's body so that it can be fetched while other scripts continue executing.

To see the bug this fixes:

- Create a job with the GIT SCM and a couple of SCM locations and some credential parameters
- Save the job
- Load the job
- Remove the first credential parameter, now all the credential tags loose the styling (width:auto on the drop-down) and the Add button overflows to the next line
@stephenc
Copy link
Member Author

@kohsuke here's a strange one for you!

@stephenc
Copy link
Member Author

@reviewbybees

@jglick
Copy link
Member

jglick commented Jan 22, 2015

Ideally this could be proven with a functional test, but I am not sure if HtmlUnit is powerful enough to deal with the dynamic DOM changes.

This should be fixed upstream in Stapler.

@kohsuke
Copy link
Member

kohsuke commented Jan 23, 2015

Created JENKINS-26578 to track the fix in stapler. A comment should be added that this fix is a temporary work around until that gets resolved.

I'm OK with pushing this fix in, until core picks up JENKINS-26578 whenever that happens.

@rsandell
Copy link
Member

+1

stephenc added a commit that referenced this pull request Jan 26, 2015
Adjuncts do not include CSS correctly for removable dom fragments
@stephenc stephenc merged commit 08c37f7 into jenkinsci:master Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants