Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions xblocks_contrib/html/html.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ class HtmlBlockMixin(LegacyXmlMixin, XBlock):
values=[{"display_name": _("Visual"), "value": "visual"}, {"display_name": _("Raw"), "value": "raw"}],
scope=Scope.settings,
)
# Opt-in styling for this block. When enabled the block renders its HTML in
# a shadow root carrying the MFE theme, so page styles cannot reach the
# content and the content cannot leak styles back into the page.
include_theme = Boolean(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rpenido I'm wondering if something like "use_mfe_theming" or an inverse "use_legacy_theming" might be more appropriate. The more I think about it, the more "include_theme" seems a little vague.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"use_mfe_theme" would match the display name, and I think be more informative. :)

help=_("If enabled, this content is styled with the MFE theme and rendered in isolation."),
display_name=_("Use MFE Theme"),
default=False,
scope=Scope.settings,
)

ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA = "ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA"

Expand All @@ -190,9 +199,22 @@ class HtmlBlockMixin(LegacyXmlMixin, XBlock):
def student_view(self, _context):
"""Return a fragment that contains the html for the student view."""
frag = Fragment(self.get_html())
frag.add_css(resource_loader.load_unicode("static/css/html.css"))
frag.add_javascript("""function HtmlBlock(runtime, element){}""")
frag.initialize_js("HtmlBlock")
# The legacy html.css is not loaded for themed blocks; the theme is
# applied inside the block's shadow root instead.
if not self.include_theme:
frag.add_css(resource_loader.load_unicode("static/css/html.css"))

frag.add_javascript(resource_loader.load_unicode("static/js/html_block.js"))

# The MFE config API is only served by the LMS, so point at it
# explicitly; in Studio this falls back to the CDN defaults.
frag.initialize_js(
"HtmlBlock",
{
"include_theme": self.include_theme,
"mfe_config_api": f"{settings.LMS_ROOT_URL}/api/mfe_config/v1",
},
)
Comment on lines +207 to +217

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rpenido I feel like for best backwards compatibility, it would be good to initialise the non-include-theme variant exactly as before, including the barebones HtmlBlock js implementation. Although I'm not sure about namespacing here - would the HtmlBlock js function conflict with that of other Text blocks on the same page?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am not sure if I agree with you on this one. Since we added more functionality to the init function (using html_block.js instead of just using an empty function like before), I think we should load it for every case and make it handle the parameters there, abstracting its logic from html.py.
In the future if someone needs other JS features for our HTML block, he doesn't need to add a new condition for loading the html_block.js or, in the worst case, be tempted to add a new html_block_for_my_feature.js file.

Does that make sense?

And the new HTMLBlock declared function early returns if we don't have initArgs of the included_theme. I don't think we are at risk to add a regression here.

   if (!initArgs || !initArgs.include_theme) {
      return;
    }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rpenido yep that makes sense, thanks for checking :) I think I'm just aware that this html block is also used for the "raw html" block, which is used for arbitrary interactive (js) features - eg. the demo course feedback buttons:

image

return frag

@XBlock.supports("multi_device")
Expand Down
145 changes: 145 additions & 0 deletions xblocks_contrib/html/static/js/html_block.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/**
* Renders a Text (HTML) XBlock inside a shadow root so that the block is styled
* by the MFE theme without leaking styles in either direction.
*
* The block's HTML is rendered server-side, so there is nothing to "render"
* here: the job is to move the existing children into a shadow root and attach
* the theme stylesheets to it.
*/
(function () {
'use strict';

var CDN_CORE = 'https://cdn.jsdelivr.net/npm/@openedx/paragon@23/dist/core.min.css';

/**
* Fetch the Paragon theme stylesheet URLs from the MFE config API.
*
* Returns `core` and `theme` arrays built from PARAGON_THEME_URLS, falling back
* to the CDN defaults for whatever the deployment does not publish. The
* deployment's own layers are preferred over the CDN ones so there is a single
* source of truth for theme URLs: Studio's editor reads the same key and builds
* its preview from it, so whatever is attached here is what is previewed there.
*
* @param {string} mfeConfigApiUrl - URL of the MFE config API.
* @returns {Promise<{core: string[], theme: string[]}>}
*/
async function getThemes(mfeConfigApiUrl) {
let themeUrls;
try {
var response = await fetch(mfeConfigApiUrl);
var mfeConfig = await response.json();
themeUrls = mfeConfig.PARAGON_THEME_URLS || {};
} catch (error) {
// Not fatal: the block still renders, just with the CDN defaults.
console.error('Text XBlock: failed to fetch theme URLs:', error);
themeUrls = {};
}
var variant = themeUrls.variants && themeUrls.variants[activeVariant(themeUrls)];
return {
core: [pickUrl(themeUrls.core) || CDN_CORE].filter(Boolean),
theme: [pickUrl(variant)].filter(Boolean),
};
}

/**
* Work out which variant is active.
*
* Two shapes are published in practice: frontend-base's `Theme`
* (https://github.com/openedx/frontend-base/blob/main/types.ts) carries an optional
* `defaults` map naming the active light and dark variants, while tutor-indigo
* (https://github.com/overhangio/tutor-indigo) ships only a `variants` map with
* nothing pointing at one. So read `defaults` when it is there, and otherwise
Comment on lines +49 to +51

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rpenido Note that the Theme interface type in frontend-base is very lenient: the defaults map isn't a required key, in fact no keys are required. So the tutor-indigo one may not be a special case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right. For now, nit update to the docs here: 949f631

* take the first variant present rather than dropping a configured theme.
*/
function activeVariant(themeUrls) {
if (themeUrls.defaults && themeUrls.defaults.light) {
return themeUrls.defaults.light;
}
var variants = themeUrls.variants || {};
if (variants.light) {
return 'light';
}
var names = Object.keys(variants);
return names.length ? names[0] : null;
}

/**
* Pick the stylesheet URL out of a `core` or `variants` entry.
*
* Entries appear either nested (`{urls: {default, brandOverride}}`, as
* tutor-indigo publishes) or flat (`{url}`). Prefer `brandOverride` so the
* deployment's theme layers on top of the CDN build, and fall back to
* `default` for configurations that publish only that.
*/
function pickUrl(entry) {
if (!entry) {
return undefined;
}
if (entry.urls) {
return entry.urls.brandOverride || entry.urls.default || entry.url;
}
return entry.url;
}

/**
* Attach a stylesheet inside the shadow root only.
*
* Paragon declares its custom properties on `:root`, which matches nothing
* inside a shadow tree, but they still reach the content by inheritance through
* the host element. Attaching to the document as well would restyle every
* other block sharing the page, since Paragon core carries top-level rules for
* bare element selectors.
*/
function addStylesheet(shadowRoot, url) {
var link = document.createElement('link');
link.rel = 'stylesheet';
link.href = url;
shadowRoot.appendChild(link);
}

/**
* Move the block's server-rendered children into a shadow root.
*/
function sandbox(element) {
if (element.shadowRoot) {
return element.shadowRoot.querySelector('.xblock-root');
}

var shadowRoot = element.attachShadow({ mode: 'open' });
var root = document.createElement('div');
root.classList.add('xblock-root');

// Adopt the rendered content rather than re-rendering it, so anything the
// server produced (images, anchors, embedded markup) is preserved as-is.
while (element.firstChild) {
root.appendChild(element.firstChild);
}

shadowRoot.appendChild(root);
return root;
}

/**
* XBlock view entry point. Invoked by the XBlock JS runtime as
* `HtmlBlock(runtime, element, initArgs)`.
*
* @param {Object} runtime - XBlock runtime (unused).
* @param {Element} element - The block's root element.
* @param {Object} initArgs - Data supplied by `Fragment.initialize_js`.
*/
function HtmlBlock(runtime, element, initArgs) {
if (!initArgs || !initArgs.include_theme) {
return;
}

var root = sandbox(element);

getThemes(initArgs.mfe_config_api).then(function (themes) {
[...themes.core, ...themes.theme].forEach(function (url) {
addStylesheet(root.getRootNode(), url);
});
});
}

window.HtmlBlock = HtmlBlock;
}());
58 changes: 58 additions & 0 deletions xblocks_contrib/html/tests/test_html.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,64 @@ def test_student_preview_view(self, view):
rendered = module_system.render(block, view, {}).content
assert html in rendered

def _rendered_fragment(self, include_theme, view="student_view"):
"""Render the block and return the real Fragment, for inspecting resources.

The block's own `student_view` is called directly: going through
`module_system.render()` returns a Mock, which hides the resources.
"""
block = HtmlBlock(
get_test_system(),
DictFieldData(
{
"data": "<p>This is a test</p>",
"include_theme": include_theme,
}
),
Mock(),
)
return getattr(block, view)({})

def test_default_include_theme_is_false(self):
"""The opt-in defaults to off, so existing blocks render unchanged."""
block = HtmlBlock(get_test_system(), DictFieldData({}), Mock())
assert block.include_theme is False

def test_unthemed_block_loads_legacy_css(self):
fragment = self._rendered_fragment(include_theme=False)
resources = [str(r.data) for r in fragment._resources]
assert any("@import" in r for r in resources), "expected legacy html.css"

def test_block_is_initialized_regardless_of_theme(self):
"""Both variants must be initialized.

initialize_js is what emits `data-init` on the block root, and every rule
in html.css is scoped to it. Gating it on include_theme left unthemed
blocks completely unstyled.
"""
for include_theme in (False, True):
fragment = self._rendered_fragment(include_theme=include_theme)
assert fragment.js_init_fn == "HtmlBlock", include_theme
assert fragment.json_init_args["include_theme"] is include_theme

def test_themed_block_skips_legacy_css(self):
"""The legacy stylesheet must not be loaded for themed blocks."""
fragment = self._rendered_fragment(include_theme=True)
resources = [str(r.data) for r in fragment._resources]
assert not any("@import" in r for r in resources), "legacy html.css should be skipped"

def test_themed_block_passes_theme_config_to_js(self):
"""The view tells the JS to sandbox the content and where to get the theme."""
fragment = self._rendered_fragment(include_theme=True)
assert fragment.json_init_args["include_theme"] is True
assert fragment.json_init_args["mfe_config_api"].endswith("/api/mfe_config/v1")

def test_view_ships_the_sandbox_script(self):
"""The JS that creates the shadow root is attached to the fragment."""
fragment = self._rendered_fragment(include_theme=True)
resources = [str(r.data) for r in fragment._resources]
assert any("attachShadow" in r for r in resources), "expected the shadow DOM script"


class HtmlBlockSubstitutionTestCase(unittest.TestCase):
def test_substitution_user_id(self):
Expand Down
Loading