Migrate inline JavaScript event handlers#418
Conversation
…etoolsstore modules - Add js and css dependencies in the addClientDependencies method - Fixed bug in SignUpAdmin.jsp - group names were not being displayed in the combobox
…de will be removed entirely in the next PR.
…I added to security policy set in the header.
…ent.getElementById("customGctButton")['onclick'] as suggested by Adam. This drops the migrated handlers from text searches looking for typical inline handler patterns ("onclick = ")
| { | ||
| // add a container listener so we'll know when our container is deleted: | ||
| ContainerManager.addContainerListener(new SkylineToolsStoreContainerListener()); | ||
| SecurityManager.registerAllowedConnectionSource("script-src", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); |
There was a problem hiding this comment.
The key is primarily for use cases where the value may change due to config from users. I'll add some JavaDoc separately to help clarify.
| SecurityManager.registerAllowedConnectionSource("script-src", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); | |
| SecurityManager.registerAllowedConnectionSource("jquery-ui", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); |
| dependencies.add("skylinetoolsstore/js/functions.js"); | ||
| } | ||
|
|
||
| public final HtmlString editIconImgHtml = HtmlString.unsafe(IMG(DOM.at(src, getWebappURL("skylinetoolsstore/img/pencil.png")).at(alt, "Pencil")).renderToString()); |
There was a problem hiding this comment.
@labkey-matthewb @labkey-adam this works but routing this through unsafe feels unsatisfying. Do you have a recommendation for a different pattern that works with link()?
There was a problem hiding this comment.
I'm afraid not. LinkBuilder and link() should take Renderable, which would avoid HtmlString entirely. Feel free to open an issue to me so I don't forget...
There was a problem hiding this comment.
Actually, DOM.createHtmlFragment() is your friend here; just don't look at the impl. 😄 I also have a local change to switch html-taking methods to Renderable.
There was a problem hiding this comment.
Thanks. My PR is targeting 24.3 so the changes to builders accepting Renderable will not be available. Should I target "develop" instead? For now, I have replaced HtmlString.unsafe() with DOM.createHtml() to make it look less ugly.
There was a problem hiding this comment.
Whatever's easiest for you... I'd probably stick with 24.3 since that's what you have and my PR isn't merged yet. Though note that develop is fine for all the strict CSP work.
| { | ||
| // add a container listener so we'll know when our container is deleted: | ||
| ContainerManager.addContainerListener(new TestResultsContainerListener()); | ||
| SecurityManager.registerAllowedConnectionSource("script-src", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); |
There was a problem hiding this comment.
| SecurityManager.registerAllowedConnectionSource("script-src", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); | |
| SecurityManager.registerAllowedConnectionSource("jquery-ui", "https://code.jquery.com/ui/1.13.2/jquery-ui.min.js"); |
3 participants
Related Pull Requests
Changes