fix: make built-in XBlock Sass theme-aware again

In ~Palm and earlier, all built-in XBlock Sass was included into LMS and CMS
styles before being compiled. The generated CSS was coupled together with
broader LMS/CMS CSS. This means that comprehensive themes have been able to
modify built-in XBlock appearance by setting certain Sass variables. We say that
built-in XBlock Sass was, and is expected to be, "theme-aware".

Shortly after Palm, we decoupled XBlock Sass from LMS and CMS Sass [1]. Each
built-in block's Sass is now compiled into two separate CSS targets, one for
block editing and one for block display. The CSS, now located at
`common/static/css/xmodule`, is injected into the running Webpack context with
the new `XModuleWebpackLoader`. Built-in XBlocks already used
`add_webpack_to_fragment` in order to add JS Webpack bundles to their view
fragments, so when CSS was added to Webpack, it Just Worked.

This unlocked a slieu of simplifications for static asset processing [2];
however, it accidentally made XBlock Sass theme-*unaware*, or perhaps
theme-confused, since the CSS was targeted at `common/static/css/xmodule`
regardless of the theme. The result of this is that **built-in XBlock views will
use CSS based on the Sass variables _last theme to be compiled._** Sass
variables are only used in a handful of places in XBlocks, so the bug is subtle,
but it is there for those running off of master. For example, using edX.org's
theme on master, we can see that there is a default blue underline in the Studio
sequence nav [3]. With this bugfix, it becomes the standard edX.org
greenish-black [4].

This commit makes several changes, firstly to fix the bug, and secondly to leave
ourselves with a more comprehensible asset setup in the `xmodule/` directory.

* We remove the `XModuleWebpackLoader`, thus taking built-in XBlock Sass back
  out of Webpack.

* We compile XBlock Sass not to `common/static/css/xmodule`, but to:

  * `[lms|cms]/static/css` for the default theme, and
  * `<THEME_ROOT>/[lms|cms]/static/css`, for any custom theme.

  This is where the comprehensive theming system expects to find themable
  assets. Unfortunately, this does mean that the Sass is compiled twice, both
  for LMS and CMS. We would have liked to compile it once to somewhere in the
  `common/`, but comprehensive theming does not consider `common/` assets to be
  themable.

* We split `add_webpack_to_fragment` into two more specialized functions:
  * `add_webpack_js_to_fragment` , for adding *just* JS from a Webpack bundle,
    and
  * `add_sass_to_fragment`, for adding static links to CSS compiled themable
    Sass (not Webpack). Both these functions are moved to a new module
    `xmodule/util/builtin_assets.py`, since the original module
    (`xmodule/util/xmodule_django.py`) didn't make a ton of sense.

* In an orthogonal bugfix, we merge Sass `CourseInfoBlock`, `StaticTabBlock`,
  `AboutBlock` into the `HtmlBlock` Sass files. The first three were never used,
  as their styling was handled by `HtmlBlock` (their shared parent class).

* As a refactoring, we change Webpack bundle names and Sass module names to be
  less misleading:
  * student_view, public_view, and author_view: was `<Name>BlockPreview`, is now
    `<Name>BlockDisplay`.
  * studio_view: was `<Name>BlockStudio`, is now `<Name>BlockEditor`.

* As a refactoring, we move the contents of `xmodule/static` into the existing
  `xmodule/assets` directory, and adopt its simper structure. We now have:
  *  `xmodule/assets/*.scss`: Top-level compiled Sass modules. These could be
     collapsed away in a future refactoring.
  * `xmodule/assets/<blocktype>/*`: Resources for each block, including both JS
    modules and Sass includes (underscore-prefixed so that they aren't
    compiled). This structure maps closely with what externally-defined XBlocks
    do.
  * `xmodule/js` still exists, but it will soon be folded into the
    `xmodule/assets`.

* We add a new README [4] to explain the new structure, and also update a
  docstring in `openedx/lib/xblock/utils` which had fallen out of date with
  reality.

* Side note: We avoid the term "XModule" in all of this, because that's
  (thankfully) become a much less useful/accurate way to describe these blocks.
  Instead, we say "built-in XBlocks".

Refs:
1. openedx#32018
2. openedx#32292
3. https://github.com/openedx/edx-platform/assets/3628148/8b44545d-0f71-4357-9385-69d6e1cca86f
4. https://github.com/openedx/edx-platform/assets/3628148/d0b7b309-b8a4-4697-920a-8a520e903e06
5. https://github.com/openedx/edx-platform/tree/master/xmodule/assets#readme

Part of: openedx#32292

cms/djangoapps/contentstore/views/preview.py

@@ -24,7 +24,7 @@
 from xmodule.services import SettingsService, TeamsConfigurationService
 from xmodule.studio_editable import has_author_view
 from xmodule.util.sandboxing import SandboxService
-from xmodule.util.xmodule_django import add_webpack_to_fragment
+from xmodule.util.builtin_assets import add_webpack_js_to_fragment
 from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, XModuleMixin
 from cms.djangoapps.xblock_config.models import StudioConfig
 from cms.djangoapps.contentstore.toggles import individualize_anonymous_user_id, ENABLE_COPY_PASTE_FEATURE
@@ -319,7 +319,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
             'language': getattr(course, 'language', None)
         }
 
-        add_webpack_to_fragment(frag, "js/factories/xblock_validation")
+        add_webpack_js_to_fragment(frag, "js/factories/xblock_validation")
 
         html = render_to_string('studio_xblock_wrapper.html', template_context)
         frag = wrap_fragment(frag, html)

cms/envs/common.py

@@ -1440,7 +1440,6 @@
     'DEFAULT': {
         'BUNDLE_DIR_NAME': 'bundles/',
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'),
-        'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader',
     },
     'WORKERS': {
         'BUNDLE_DIR_NAME': 'bundles/',

lms/envs/common.py

@@ -2715,7 +2715,6 @@ def _make_locale_paths(settings):  # pylint: disable=missing-function-docstring
     'DEFAULT': {
         'BUNDLE_DIR_NAME': 'bundles/',
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'),
-        'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader',
     },
     'WORKERS': {
         'BUNDLE_DIR_NAME': 'bundles/',

openedx/core/djangoapps/xblock/runtime/runtime.py

@@ -334,7 +334,7 @@ def render(self, block, view_name, context=None):
             raise PermissionDenied
 
         # We also need to override this method because some XBlocks in the
-        # edx-platform codebase use methods like add_webpack_to_fragment()
+        # edx-platform codebase use methods from builtin_assets.py,
         # which create relative URLs (/static/studio/bundles/webpack-foo.js).
         # We want all resource URLs to be absolute, such as is done when
         # local_resource_url() is used.

openedx/core/lib/xblock_utils/__init__.py

@@ -11,7 +11,6 @@
 import uuid
 
 import markupsafe
-import webpack_loader.utils
 from django.conf import settings
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.contrib.staticfiles.storage import staticfiles_storage
@@ -29,7 +28,6 @@
 from common.djangoapps import static_replace
 from common.djangoapps.edxmako.shortcuts import render_to_string
 from xmodule.seq_block import SequenceBlock  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.util.xmodule_django import add_webpack_to_fragment  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.vertical_block import VerticalBlock  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.x_module import (  # lint-amnesty, pylint: disable=wrong-import-order
     PREVIEW_VIEWS,
@@ -116,6 +114,9 @@ def wrap_xblock(
     if view == STUDENT_VIEW and getattr(block, 'HIDDEN', False):
         css_classes.append('is-hidden')
 
+    # TODO: This special case will be removed when we update the SCSS under
+    #       xmodule/assets to use the standard XBlock CSS classes.
+    #       See https://github.com/openedx/edx-platform/issues/32617.
     if getattr(block, 'uses_xmodule_styles_setup', False):
         if view in PREVIEW_VIEWS:
             # The block is acting as an XModule
@@ -444,16 +445,22 @@ def xblock_resource_pkg(block):
     openassessment.xblock.openassessmentblock.OpenAssessmentBlock, the value
     returned is 'openassessment.xblock'.
 
-    XModules are special cased because they're local to this repo and they
-    actually don't share their resource files when compiled out as part of the
-    XBlock asset pipeline. This only covers XBlocks and XModules using the
-    XBlock-style of asset specification. If they use the XModule bundling part
-    of the asset pipeline (xmodule_assets), their assets are compiled through an
-    entirely separate mechanism and put into lms-modules.js/css.
+    Built-in edx-platform XBlocks (defined under ./xmodule/) are special cases.
+    They currently use two different mechanisms to load assets:
+    1. The `builtin_assets` utilities, which let the blocks add JS and CSS
+       compiled completely outside of the XBlock pipeline. Used by HtmlBlock,
+       ProblemBlock, and most other built-in blocks currently. Handling for these
+       assets does not interact with this function.
+    2. The (preferred) standard XBlock runtime resource loading system, used by
+       LibrarySourdedBlock. Handling for these assets *does* interact with this
+       function.
+
+    We hope to migrate to (2) eventually, tracked by:
+    https://github.com/openedx/edx-platform/issues/32618.
     """
-    # XModules are a special case because they map to different dirs for
-    # sub-modules.
     module_name = block.__module__
+    # Special handling for case (2) of the built-in XBlocks because they map to different
+    # dirs for sub-modules.
     if module_name.startswith('xmodule.'):
         return module_name
 

pavelib/assets.py

@@ -170,8 +170,7 @@ def get_theme_sass_dirs(system, theme_dir):
     css_dir = theme_dir / system / "static" / "css"
     certs_sass_dir = theme_dir / system / "static" / "certificates" / "sass"
     certs_css_dir = theme_dir / system / "static" / "certificates" / "css"
-    xmodule_sass_dir = path("xmodule") / "static" / "sass" / system
-    xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include"
+    builtin_xblock_sass = path("xmodule") / "assets"
 
     dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, [])
     if sass_dir.isdir():
@@ -199,18 +198,6 @@ def get_theme_sass_dirs(system, theme_dir):
             ],
         })
 
-        dirs.append({
-            "sass_source_dir": xmodule_sass_dir,
-            "css_destination_dir": path("common") / "static" / "css" / "xmodule",
-            "lookup_paths": [
-                xmodule_lookup_dir,
-                *dependencies,
-                sass_dir / "partials",
-                system_sass_dir / "partials",
-                system_sass_dir,
-            ],
-        })
-
         # now compile theme sass files for certificate
         if system == 'lms':
             dirs.append({
@@ -222,6 +209,28 @@ def get_theme_sass_dirs(system, theme_dir):
                 ],
             })
 
+    # Now, finally, compile builtin XBlocks' Sass. Themes cannot override these
+    # Sass files directly, but they *can* modify Sass variables which will affect
+    # the output here. We compile all builtin XBlocks' Sass both for LMS and CMS,
+    # not because we expect the output to be different between LMS and CMS, but
+    # because only LMS/CMS-compiled Sass can be themed; common sass is not themed.
+    dirs.append({
+        "sass_source_dir": builtin_xblock_sass,
+        "css_destination_dir": css_dir,
+        "lookup_paths": [
+            # XBlock editor views may need both LMS and CMS partials.
+            # XBlock display views should only need LMS patials.
+            # In order to keep this build script simpler, though, we just
+            # include everything and compile everything at once.
+            theme_dir / "lms" / "static" / "sass" / "partials",
+            theme_dir / "cms" / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass" / "partials",
+            path("cms") / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass",
+            path("cms") / "static" / "sass",
+        ],
+    })
+
     return dirs
 
 
@@ -237,8 +246,7 @@ def get_system_sass_dirs(system):
     dirs = []
     sass_dir = path(system) / "static" / "sass"
     css_dir = path(system) / "static" / "css"
-    xmodule_sass_dir = path("xmodule") / "static" / "sass" / system
-    xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include"
+    builtin_xblock_sass = path("xmodule") / "assets"
 
     dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, [])
     dirs.append({
@@ -250,17 +258,6 @@ def get_system_sass_dirs(system):
         ],
     })
 
-    dirs.append({
-        "sass_source_dir": xmodule_sass_dir,
-        "css_destination_dir": path("common") / "static" / "css" / "xmodule",
-        "lookup_paths": [
-            xmodule_lookup_dir,
-            *dependencies,
-            sass_dir / "partials",
-            sass_dir,
-        ],
-    })
-
     if system == 'lms':
         dirs.append({
             "sass_source_dir": path(system) / "static" / "certificates" / "sass",
@@ -271,6 +268,18 @@ def get_system_sass_dirs(system):
             ],
         })
 
+    # See builtin_xblock_sass compilation in get_theme_sass_dirs for details.
+    dirs.append({
+        "sass_source_dir": builtin_xblock_sass,
+        "css_destination_dir": css_dir,
+        "lookup_paths": dependencies + [
+            path("lms") / "static" / "sass" / "partials",
+            path("cms") / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass",
+            path("cms") / "static" / "sass",
+        ],
+    })
+
     return dirs
 
 
@@ -577,14 +586,18 @@ def _compile_sass(system, theme, debug, force, timing_info):
             else:
                 sh(f"rm -rf {css_dir}/*.css")
 
+        all_lookup_paths = COMMON_LOOKUP_PATHS + lookup_paths
+        print(f"Compiling Sass: {sass_source_dir} -> {css_dir}")
+        for lookup_path in all_lookup_paths:
+            print(f"    with Sass lookup path: {lookup_path}")
         if dry_run:
             tasks.environment.info("libsass {sass_dir}".format(
                 sass_dir=sass_source_dir,
             ))
         else:
             sass.compile(
                 dirname=(sass_source_dir, css_dir),
-                include_paths=COMMON_LOOKUP_PATHS + lookup_paths,
+                include_paths=all_lookup_paths,
                 source_comments=source_comments,
                 output_style=output_style,
             )

xmodule/README.rst

@@ -6,15 +6,18 @@ The ``xmodule`` folder contains a variety of old-yet-important functionality cor
 * the ModuleStore, edx-platform's "Version 1" learning content storage backend;
 * an XBlock Runtime implementation for ModuleStore-backed content;
 * the "partitions" framework for differentiated XBlock content;
-* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``; and
-* the implementations of several different content-level XBlocks, such as ``problem`` and ``html``.
+* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``;
+* the implementations of several different built-in content-level XBlocks, such as ``problem`` and ``html``; and
+* `assets for those built-in XBlocks`_.
+
+.. _assets for those built-in XBlocks: https://github.com/openedx/edx-platform/tree/master/assets
 
 Historical Context
 ******************
 
 "XModule" was the original content framework for edx-platform, which gives the folder its current name.
 XModules rendered specific course run content types to users for both authoring and learning.
-For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences. 
+For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences.
 
 XModule was succeeded in ~2013 by the "XBlock" framework, which served the same purpose, but put additional focus into flexibility and modularity.
 XBlock allows new content types to be created by anyone, completely external the edx-platform repository.
@@ -47,7 +50,7 @@ To help with this direction, please **do not add new functionality to this direc
 
 .. _Blockstore: https://github.com/openedx/blockstore/
 .. _edx-platform XBlock runtime: https://github.com/openedx/edx-platform/tree/master/openedx/core/djangoapps/xblock
-.. _openedx-learning: https://github.com/openedx/openedx-learning 
+.. _openedx-learning: https://github.com/openedx/openedx-learning
 .. _xblock-drag-and-drop-v2: https://github.com/openedx/xblock-drag-and-drop-v2
 .. _the forums: https://discuss.openedx.org
 

xmodule/annotatable_block.py

@@ -12,7 +12,7 @@
 from openedx.core.djangolib.markup import HTML, Text
 from xmodule.editing_block import EditingMixin
 from xmodule.raw_block import RawMixin
-from xmodule.util.xmodule_django import add_webpack_to_fragment
+from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment
 from xmodule.xml_block import XmlMixin
 from xmodule.x_module import (
     HTMLSnippet,
@@ -199,7 +199,8 @@ def student_view(self, context):  # lint-amnesty, pylint: disable=unused-argumen
         """
         fragment = Fragment()
         fragment.add_content(self.get_html())
-        add_webpack_to_fragment(fragment, 'AnnotatableBlockPreview')
+        add_sass_to_fragment(fragment, 'AnnotatableBlockDisplay.scss')
+        add_webpack_js_to_fragment(fragment, 'AnnotatableBlockDisplay')
         shim_xmodule_js(fragment, 'Annotatable')
 
         return fragment
@@ -211,6 +212,7 @@ def studio_view(self, _context):
         fragment = Fragment(
             self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context())
         )
-        add_webpack_to_fragment(fragment, 'AnnotatableBlockStudio')
+        add_sass_to_fragment(fragment, 'AnnotatableBlockEditor.scss')
+        add_webpack_js_to_fragment(fragment, 'AnnotatableBlockEditor')
         shim_xmodule_js(fragment, self.studio_js_module_name)
         return fragment

xmodule/assets/HtmlBlockDisplay.scss

@@ -0,0 +1,6 @@
+.xmodule_display.xmodule_AboutBlock,
+.xmodule_display.xmodule_CourseInfoBlock,
+.xmodule_display.xmodule_HtmlBlock,
+.xmodule_display.xmodule_StaticTabBlock {
+  @import "html/display.scss";
+}

xmodule/assets/HtmlBlockEditor.scss

@@ -0,0 +1,7 @@
+.xmodule_edit.xmodule_AboutBlock,
+.xmodule_edit.xmodule_CourseInfoBlock,
+.xmodule_edit.xmodule_HtmlBlock,
+.xmodule_edit.xmodule_StaticTabBlock {
+  @import "editor/edit.scss";
+  @import "html/edit.scss";
+}

xmodule/assets/README.rst

@@ -0,0 +1,99 @@
+xmodule/assets: edx-platform XBlock resources
+#############################################
+
+This folder exists to contain resources (ie, static assets) for the XBlocks
+defined in edx-platform.
+
+Concepts
+********
+
+We would like edx-platform XBlock resources to match the standard XBlock
+resource strategy as much as possible, because:
+
+* it'll make it easier to extract the XBlocks into their own packages
+  eventually, and
+* it makes it easier to reason about the system as a whole when
+  internally-defined and externally-defined blocks play by the same rules.
+
+Due to the legacy of the XModule system, we're not quite there yet.
+However, we are proactively working towards a system where:
+
+* Python is not involved in the generation of static assets.
+* We minimze conditionals that differentiate between "older" (aka "XModule-style")
+  XBlocks and newer (aka "pure") XBlocks.
+* Each XBlock's assets are contained within their own folder as much as
+  possible. See ``./vertical`` as an example.
+
+Themable Sass (.scss)
+*********************
+
+XBlock CSS for ``student_view``, ``author_view``, and ``public_view`` is compiled from the various ``./<ClassName>BlockDisplay.scss`` modules, such as `AnnotatableBlockDisplay.scss`_.
+
+XBlock CSS for ``studio_view`` is compiled from the various ``./<ClassName>BlockEditor.scss`` modules, such as `AnnotatableBlockEditor.scss`_.
+
+Those Sass modules are mostly thin wrappers around the underscore-prefixed Sass
+modules within block-type-subdirectories, such as `annotatable/_display.css`. In the
+future, we may `simplify things`_ by collapsing the top-level Sass modules and
+just directly compiling the block-type-subdirectories' Sass.
+
+The CSS is compiled into the static folders of both LMS and CMS, as well as into
+the corresponding folders in any enabled themes, as part of the edx-platform build.
+It is collected into the static root, and then linked to from XBlock fragments by the
+``add_sass_to_fragment`` function in `builtin_assets.py`_.
+
+.. _AnnotatableBlockDisplay: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockDisplay.scss
+.. _AnnotatableBlockEditor: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/AnnotatableBlockEditor.scss
+.. _annotatable/_display.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/annotatable/_display.scss
+.. _simplify things: https://github.com/openedx/edx-platform/issues/32621
+
+Static CSS (.css)
+*****************
+
+Non-themable, ready-to-seve CSS may also be added to the any block type's
+subdirectory. For example, see `library_source_block/style.css`_.
+
+JavaScript (.js)
+****************
+
+Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and outside in `xmodule/js`_. Different JS resources are processed differently:
+
+* For many older blocks, their JS is:
+
+  * copied to ``common/static/xmodule`` by `static_content.py`_ (aka ``xmodule_assets``),
+  * then bundled using a generated Webpack config at ``common/static/xmodule/webpack.xmodule.config.js``,
+  * which is included into `webpack.common.config.js`_,
+  * allowing it to be included into XBlock fragments using ``add_webpack_js_to_fragment`` from `builtin_assets.py`_.
+
+  Example blocks using this setup:
+
+  * `ProblemBlock`_
+  * `HtmlBlock`_
+  * `AnnotatableBlock`_
+
+* For other "purer" blocks, the JS is used as a standard XBlock fragment. Example blocks:
+
+  * `VerticalBlock`_
+  * `LibrarySourcedBlock`_
+
+* Some XBlock JS is also processed through Django Pipeline and used in a couple specific legacy places.
+
+As part of an `active build refactoring`_:
+
+* We update the older builtin XBlocks to reference their JS directly rather than using copies of it.
+* We will move ``webpack.xmodule.config.js`` here instead of generating it.
+* We will consolidate all edx-platform XBlock JS here in `xmodule/assets`_.
+* We will remove XBlock JS from Django Pipeline.
+* We will delete the ``xmodule_assets`` script.
+
+.. _xmodule/assets: https://github.com/openedx/edx-platform/tree/master/xmodule/assets
+.. _xmodule/js: https://github.com/openedx/edx-platform/tree/master/xmodule/js
+.. _ProblemBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/capa_block.py
+.. _HtmlBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/html_block.py
+.. _AnnotatableBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/annotatable_block.py
+.. _VerticalBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/vertical_block.py
+.. _LibrarySourcedBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/library_sourced_block.py
+.. _active build refactoring: https://github.com/openedx/edx-platform/issues/31624
+.. _builtin_assets.py: https://github.com/openedx/edx-platform/tree/master/xmodule/util/builtin_assets.py
+.. _static_content.py: https://github.com/openedx/edx-platform/blob/master/xmodule/static_content.py
+.. _library_source_block/style.css: https://github.com/openedx/edx-platform/blob/master/xmodule/assets/library_source_block/style.css
+.. _webpack.common.config.js: https://github.com/openedx/edx-platform/blob/master/webpack.common.config.js