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

XModule Style Decoupling: Follow-up fixes & refactoring #32592

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jun 28, 2023

tl;dr

Comprehensive theming for built-in XBlocks was subtly broken since we decoupled their Sass from LMS/CMS Sass. Specifically, XBlock Sass from all themes was being compiled to the same CSS file, with each theme clobbering the previous one.

This PR fixes that issue, plus some cleanup that'll help with the final phase of #31624

Description and Supporting Info

Two commits, starting with the more interesting & significant one:

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. Decouple XModule styles from LMS/Studio styles #32018
  2. Stop dynamically generating XModule SCSS #32292
  3. Image:
    image
  4. Image:
    image
  5. https://github.com/kdmccormick/edx-platform/tree/kdmccormick/xmodule-static-css-2/xmodule/assets#readme

Part of: #32292

fix: flow ?site_theme down through Studio container preview

In ~Palm and earlier, all built-in XBlock Sass was included into CMS (and LMS) styles before being compiled. So, if a site theme was meant to affect built-in XBlock styling, those changes would be manifested directly in the base CMS CSS that is included into every single Studio page. When the user provided the ?site_theme querystring parameter, which is intended to allow devs & admins to view Studio through a given theme, CMS would look up the given theme and serve the corresponding base CMS CSS, which would affect the built-in XBlocks views (as expected).

After ~Palm, built-in XBlocks styles are handled more similarly to to pure XBlock styles, in that they are only requested when CMS tries to render the block. In Studio, blocks are not rendered by the original request, but by a subsequent AJAX request to the /container_preview enpoint. Thus, passing the ?site_theme query parameter to the original request will apply the given theme to Studio's chrome, but the theme will not apply to built-in XBlock views, whose CSS is now loaded via async request.

To fix this, we simply pass Studio's querystring parameters (including ?site_theme) along to the /container_view AJAX request. This will cause CMS to correctly serve the built-in XBlock CSS from the theme specified by ?site_theme, rather than whatever the current theme is.

Part of: #32292

Testing

Setup

First: Set up Tutor Nightly if you haven't already, with at least one user created. Ensure images are recently pulled.

Then:

tutor images pull all
tutor local do importdemocourse

# Set up Tutor's indigo theme, but set the primary color to an obnoxious orange so it's easy to spot.
tutor plugins install indigo
tutor plugins enable indigo
tutor config save --set 'INDIGO_PRIMARY_COLOR="#ff8844"'

# Add the Red Theme from edx-platform. Adjust with your own edx-platform path.
cp -r ~/openedx/edx-platform/themes/red-theme "$(tutor config printroot)/env/build/openedx/themes"

# Rebuild the image (so themes are compiled) and set the active theme to our orange-ified indigo.
tutor images build openedx
tutor local do settheme indigo

Bug Reproduction

tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/openedx/edx-platform
tutor config save --set EDX_PLATFORM_VERSION=master
tutor images build openedx
tutor local start -d

Then, navigate to this problem in the demo course, log in.

"Show Answer" is nice and orange, as specified by our god awful theming choices:

image

But when you hover over "Show Answer", notice that when you hover, it turns RED. As in, red-theme red! 😲

image

Open the network inspector and filter for CSS. Refresh the page. Notice where ProblemBlockPreview.css is coming from: it's http://local.overhang.io/static/css/xmodule/ProblemBlockPreview.css. Notice that there's no /indigo anywhere in that path. Both indigo and red-theme have compiled to that same location, and because red-theme is second alphabetically, it won.

image

This problem is barely noticeable, because the ProblemBlock mostly just inherits cascading CMS CSS rules... but link hover is one thing that for which it does look at the compiled XBlock CSS.

Fix Reproduction

tutor local stop
tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/kdmccormick/edx-platform
tutor config save --set EDX_PLATFORM_VERSION=kdmccormick/xmodule-static-scss-2
tutor images build openedx
tutor local start -d

Again, navigate to this same problem in the demo course. Hover over "Show Answer". No red, just orange 😌

image

Open the network inspector and filter for CSS. Refresh the page. Now, notice where ProblemBlockDisplay.css is coming from: it's http://local.overhang.io/static/indigo/css/xmodule/ProblemBlockDisplay.ee14053c39ab.css, which is specific to indigo.

image

Don't worry, if we run tutor local do settheme red-theme, refresh, and check the network log again, we see red-theme's version of the file is safe over here: http://local.overhang.io/static/red-theme/css/xmodule/ProblemBlockDisplay.bdc1f9f16129.css, yielding:

image

@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch 6 times, most recently from 9e5edf0 to 59115e1 Compare June 29, 2023 22:15
@kdmccormick kdmccormick changed the title build: xmodule scss fixes (WIP) fix: rework XModule SCSS compilation to be theme-aware Jun 29, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch 5 times, most recently from bd1933a to 6954a3b Compare June 30, 2023 20:25
@kdmccormick kdmccormick changed the title fix: rework XModule SCSS compilation to be theme-aware fix: make builtin xblock sass theme-aware again Jun 30, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch 4 times, most recently from 8c2a57a to d90ae76 Compare July 1, 2023 00:32
@kdmccormick kdmccormick changed the title fix: make builtin xblock sass theme-aware again XModule Style Decoupling: Follow-up fixes & refactoring Jul 5, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch 5 times, most recently from 46384b3 to 5f8db1b Compare July 6, 2023 00:51
@kdmccormick kdmccormick self-assigned this Jul 6, 2023
@kdmccormick kdmccormick marked this pull request as ready for review July 6, 2023 02:19
@kdmccormick kdmccormick requested a review from ormsbee July 6, 2023 02:19
@kdmccormick
Copy link
Member Author

FYI @andrey-canon -- here's some more XModule assets insanity if you are interested :)

Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Just a couple of tiny nits in the docs portion.

openedx/core/lib/xblock_utils/__init__.py Outdated Show resolved Hide resolved
xmodule/assets/README.rst Outdated Show resolved Hide resolved
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch from 934ab21 to 25c8c69 Compare July 6, 2023 14:38
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
In ~Palm and earlier, all built-in XBlock Sass was included into CMS
(and LMS) styles before being compiled. So, if a site theme was meant to
affect built-in XBlock styling, those changes would be manifested
directly in the base CMS CSS that is included into every single Studio
page. When the user provided the `?site_theme` querystring parameter,
which is intended to allow devs & admins to view Studio through a given
theme, CMS would look up the given theme and serve the corresponding
base CMS CSS, which would affect the built-in XBlocks views (as
expected).

After ~Palm, built-in XBlocks styles are handled more similarly to to
pure XBlock styles, in that they are only requested when CMS tries to
render the block. In Studio, blocks are not rendered by the original
request, but by a subsequent AJAX request to the `/container_preview`
enpoint. Thus, passing the `?site_theme` query parameter to the original
request will apply the given theme to Studio's chrome, but the theme
will _not_ apply to built-in XBlock views, whose CSS is now loaded via
async request.

To fix this, we simply pass Studio's querystring parameters (including
`?site_theme`) along to the `/container_view` AJAX request. This will
cause CMS to correctly serve the built-in XBlock CSS from the theme
specified by `?site_theme`, rather than whatever the current theme is.

Part of: openedx#32292
@kdmccormick kdmccormick force-pushed the kdmccormick/xmodule-static-css-2 branch from 25c8c69 to b9df5cd Compare July 6, 2023 15:37
@kdmccormick kdmccormick enabled auto-merge (rebase) July 6, 2023 15:38
@kdmccormick kdmccormick merged commit f4540c3 into openedx:master Jul 6, 2023
42 checks passed
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@kdmccormick kdmccormick deleted the kdmccormick/xmodule-static-css-2 branch July 6, 2023 17:03
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

kdmccormick pushed a commit that referenced this pull request Jul 6, 2023
#32592 changed some webpack loading, and renamed the video display bundle from VideoBlockPreview to VideoBlockDisplay but the code for the public view did not include the updated name.
kdmccormick added a commit to kdmccormick/edx-platform that referenced this pull request Sep 1, 2023
lti_block has Sass for its display, but not for its editor.

During the `add_sass_to_fragment` refactoring, I mixed this up:
I added a non-existent scss file to the studio_view but didn't add
the actual scss file to the student_view.

Course authors using the (deprecated) lti_block saw:

    We're having trouble rendering your component
    Students will not be able to access this component. Re-edit your component to fix the error.

    Error: Sass not found: /edx/app/edxapp/edx-platform/xmodule/assets/LTIBlockEditor.scss

as a result of this bug.

Original PR: openedx#32592

Private-ref: https://2u-internal.atlassian.net/browse/TNL-11029
kdmccormick added a commit that referenced this pull request Sep 1, 2023
There exists a Sass file for lti_block's display, but not for its editor.

However, during the `add_sass_to_fragment` refactoring, I mixed that up:
I added a non-existent scss file to the studio_view but didn't add
the actual scss file to the student_view.

Course authors using the (deprecated) lti_block saw:

    We're having trouble rendering your component
    Students will not be able to access this component. Re-edit your component to fix the error.

    Error: Sass not found: /edx/app/edxapp/edx-platform/xmodule/assets/LTIBlockEditor.scss

as a result of this bug.

Original PR: #32592
Private-ref: https://2u-internal.atlassian.net/browse/TNL-11029
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.

3 participants