Skip to content

Commit

Permalink
Fix external styles and script not hoisted by externals (#2414)
Browse files Browse the repository at this point in the history
Also add warning in documentation that styles in hoisted
external will also affect the rest of the page
  • Loading branch information
yiwen101 committed Feb 25, 2024
1 parent db27d8a commit ef502db
Show file tree
Hide file tree
Showing 14 changed files with 369 additions and 11 deletions.
2 changes: 2 additions & 0 deletions docs/userGuide/syntax/includes.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ When setting the `id` of a fragment, be careful not to clash with heading anchor

</box>

<include src="panels.md#script_and_styles_warning"></include>

<include src="tip.md" boilerplate >
<span id="tip_body">
The `<include>` mechanism can be used inside any MarkBind source file (even inside the _frontmatter_ section) but it will not work inside some _special_ files such as the `_markbind/variables.md`.
Expand Down
11 changes: 11 additions & 0 deletions docs/userGuide/syntax/panels.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,17 @@ plain text ...
</variable>
</include>

<div id = "script_and_styles_warning">
<box type = "warning" header = "#### Global Effects of the Script and Styles from the Imported Externals">

Importing external resources that contains `script` or `styles` can inadvertently take global effects on your MarkBind website. Due to hoisting during processing, imported scripts and stylesheets affect the entire page. This could potentially alter its appearance and behavior beyond the intended scope.

For example, if a CSS file imported via such means styles headings to be red, this change will be reflected page-wide.

To safeguard against unintended consequences, consider directly incorporating the code or customizing styles to target specific elements or classes not used universally. This approach grants more precise control over your website's presentation and reduces the risk of unexpected changes.
</box>
</div>

**If `popup-url` attribute is provided, a popup button will be shown. If clicked, it opens the specified url in a new window.**

<include src="codeAndOutput.md" boilerplate >
Expand Down
9 changes: 9 additions & 0 deletions packages/cli/test/functional/test_site/expected/siteData.json
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,15 @@
"title": "glyphicon & octicon icon in page, only glyphicon & octicon stylesheets should be loaded",
"headings": {},
"headingKeywords": {}
},
{
"src": "testSourceContainScript.md",
"title": "Test: If source contains script or css, when included, the script or css should be included",
"headings": {
"panel-with-src-that-contains-css-and-script-header": "Panel with src that contains css and script header",
"h1-text": "\n\n\n h1 text\n \n"
},
"headingKeywords": {}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<h1 id="h1-text">


h1 text
<a class="fa fa-anchor" href="#h1-text" onclick="event.stopPropagation()"></a>
</h1>

Large diffs are not rendered by default.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/cli/test/functional/test_site/site.json
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@
{
"src": "testIconsInSiteLayout.md",
"title": "glyphicon & octicon icon in page, only glyphicon & octicon stylesheets should be loaded"
},
{
"src": "testSourceContainScript.md",
"title": "Test: If source contains script or css, when included, the script or css should be included"
}
],
"pagesExclude": ["**/*-fragment.md"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<h1>
<script>
// JavaScript code specific to this component
console.log("Inline script executed! 35");
</script>
<style>
/* CSS styles specific to this component */
h1 {
color: red;
}
</style>
h1 text
</h1>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<panel header="## Panel with src that contains css and script header" src="testPanels/PanelSourceContainsScript.md" expanded></panel>
10 changes: 7 additions & 3 deletions packages/core/src/External/External.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@ export class External {
externalManager: ExternalManager;
sourceFilePath: string;
includedFiles: Set<string>;
userScriptsAndStyles: string[];

constructor(em: ExternalManager, srcFilePath: string) {
constructor(em: ExternalManager, srcFilePath: string, userScriptsAndStyles: string[]) {
this.externalManager = em;
this.sourceFilePath = srcFilePath;
this.includedFiles = new Set([srcFilePath]);
this.userScriptsAndStyles = userScriptsAndStyles;
}

/**
Expand All @@ -42,7 +44,7 @@ export class External {
const pageSources = new PageSources();
const docId = `ext-${fsUtil.removeExtension(path.basename(asIfAtFilePath))}`;
const nodeProcessor = new NodeProcessor(fileConfig, pageSources, variableProcessor,
pluginManager, siteLinkManager, undefined, docId);
pluginManager, siteLinkManager, this.userScriptsAndStyles, docId);

const nunjucksProcessed = variableProcessor.renderWithSiteVariables(this.sourceFilePath, pageSources);
const mdHtmlProcessed = await nodeProcessor.process(this.sourceFilePath, nunjucksProcessed,
Expand All @@ -57,7 +59,9 @@ export class External {

pageSources.addAllToSet(this.includedFiles);

await this.externalManager.generateDependencies(pageSources.getDynamicIncludeSrc(), this.includedFiles);
await this.externalManager.generateDependencies(pageSources.getDynamicIncludeSrc(),
this.includedFiles,
this.userScriptsAndStyles);

return this;
}
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/External/ExternalManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ export class ExternalManager {
* @param {Set<string>} includedFiles
* @return {Promise<unknown[]>}
*/
async generateDependencies(dependencies: DynamicSrc[], includedFiles: Set<string>) {
async generateDependencies(dependencies: DynamicSrc[],
includedFiles: Set<string>,
userScriptsAndStyles: string[]) {
const resolvingExternals: Promise<External>[] = [];

_.uniqBy(dependencies, d => d.asIfTo).forEach((src) => {
Expand All @@ -55,7 +57,7 @@ export class ExternalManager {
const resultPathWithExternalExt = fsUtil.setExtension(resultPath, '._include_.html');

if (!(resultPathWithExternalExt in this.builtFiles)) {
const external = new External(this, src.to);
const external = new External(this, src.to, userScriptsAndStyles);
this.builtFiles[resultPathWithExternalExt] = external.resolveDependency(src.asIfTo,
resultPathWithExternalExt,
this.config);
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/Page/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,9 @@ export class Page {
};

pageSources.addAllToSet(this.includedFiles);
await externalManager.generateDependencies(pageSources.getDynamicIncludeSrc(), this.includedFiles);
await externalManager.generateDependencies(pageSources.getDynamicIncludeSrc(),
this.includedFiles,
this.pageUserScriptsAndStyles);

this.collectHeadingsAndKeywords(pageContent);

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/html/NodeProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class NodeProcessor {
private variableProcessor: VariableProcessor,
private pluginManager: PluginManager,
private siteLinkManager: SiteLinkManager,
private userScriptsAndStyles: string[] | undefined,
private userScriptsAndStyles: string[],
docId = '',
) {
this.markdownProcessor = new MarkdownProcessor(docId);
Expand Down

0 comments on commit ef502db

Please sign in to comment.