Convert builtin block Sass variables into CSS variables

[farhan]

In this story we have to convert builtin block Sass variables into CSS variables.

For each builtin block modify its Sass to use the CSS variable instead of the Sass variable.

Tip: Start with one block and merge the PR. That way, if it breaks on edX.org or in Tutor, we know sooner than later

Relevan ticket: #35173

Tasks

Beta Give feedback

No tasks being tracked yet.

[farhan]

The implementation PR of this ticket has been reverted because of the issue identified by 2U team.
Issue was notified on the slack channel in thread

Here is the revert PR with details of issue experienced.

[farhan]

Debugging status:
I have tutor based Open edX setup on my machine with Indigo theme. It's working fine on it and reflecting well on the browser (Chrome).

Here are the screenshots:

It might be the theme issue or compilation issue.
I need more details and help to trouble shoot it, which theme are we using it on 2U site and how are we compiling it.
Sandbox of edx.org will be great.

@kdmccormick
cc: @nsprenkle

[kdmccormick]

@farhan Did you see this comment? Can you check the compiled CSS to see if that issue is present or not? Also, are you using tutor-dev or tutor-local?

I wonder if this has to do with a difference between Paver and its replacement npm run build. As far as I know 2U has not migrated off of Paver yet (#34467)

[farhan]

@kdmccormick Yes, I’ve seen it, but the issue is not present on my setup. I’m using tutor-dev. Here’s the relevant portion of the compiled CSS, above screen shots are based on this CSS:

      /* line 273, /openedx/edx-platform/xmodule/assets/video/_display.scss */
      .xmodule_display.xmodule_VideoBlock .video .video-wrapper .closed-captions.is-visible {
        max-height: calc((var(--baseline) * 3));
        border-radius: calc((var(--baseline) / 5));
        padding: 8px calc((var(--baseline) / 2)) 8px calc((var(--baseline) * 1.5));
        background: rgba(0, 0, 0, 0.75);
        color: var(--yellow); }
        /* line 280, /openedx/edx-platform/xmodule/assets/video/_display.scss */
        .xmodule_display.xmodule_VideoBlock .video .video-wrapper .closed-captions.is-visible::before {
          position: absolute;
          display: inline-block;
          top: 50%;
          left: var(--baseline);
          margin-top: -0.6em;
          font-family: 'FontAwesome';
          content: "\f142";
          color: var(--white);
          opacity: 0.5; 
}

I wonder if this has to do with a difference between Paver and its replacement npm run build. As far as I know 2U has not migrated off of Paver yet (#34467)

This might be contributing to the issue.

[kdmccormick]

I wonder if this has to do with a difference between Paver and its replacement npm run build. As far as I know 2U has not migrated off of Paver yet

On second thought, disregard this. The remaining Paver asset commands are just a thin wrapper around the npm commands. It shouldn't make a difference.

[kdmccormick]

@nsprenkle @farhan I was able to reproduce this. It only happens we pass output_style=SASS_STYLE_COMPRESSED to the Sass compiler, which we only do for production builds. It seems that the Sass compressor shipped with our 2015 version of libsass-python turns white into #fff. This isn't suprising, given that the conversion saves one character, and CSS variables weren't commonly supported until at least 2017...

One can see the bug in action using tutor-indigo:

cd path/to/your/edx-platform
git switch farhan/xblock-sass-to-css
tutor mounts add .
tutor plugins enable indigo
tutor images build openedx
tutor local run lms cat /openedx/staticfiles/indigo/css/VideoBlockDisplay.css | grep -- '--#fff'

I think there is an easy workaround, though. In _builtin-block-variables.scss, we just need to rename the --white variable, as well as any other CSS named color which the Sass compressor might mangle. We could just do something simple like --white-1: $white, --gray-1: $grey, etc. As long as we leave the underlying Sass variable name the same, this shouldn't be an issue for backwards compatibility.

[farhan]

@kdmccormick You have spotted the right reason

I have reproduced it on my setup changing following lines and then run npm run compile-sass:

        # output_style: int = SASS_STYLE_NESTED if use_dev_settings else SASS_STYLE_COMPRESSED
        output_style: int = SASS_STYLE_COMPRESSED

I think there is an easy workaround, though. In _builtin-block-variables.scss, we just need to rename the --white variable, as well as any other CSS named color which the Sass compressor might mangle. We could just do something simple like --white-1: $white, --gray-1: $grey, etc. As long as we leave the underlying Sass variable name the same, this shouldn't be an issue for backwards compatibility.

Solution is doable but perhaps not the easiest one.

@kdmccormick @nsprenkle Please skim through this PR

What about do all the following steps in one PR per Block.

1. Convert Sass variable into css variables
2. Compile css and copy the relevant block css into xmodule/assets directory
3. Use css rather than sass as implemented here in this above shared PR
4. Remove the sass files of the block from xmodule/assets directory

[kdmccormick]

@farhan oh, good idea, let's do that! Kindly start with VideoBlock so that we can be sure that the fix works as expected.

That PR is a great start. When you make the VideoBlock PR, can you rebuild the CSS without line-number comments, since we won't need those comments once the Sass is gone? In compile_sass.py, you can just temporarily hard-code in the SOURCE_COMMENTS_NONE option.

[farhan]

Further communication of this story will continue on following story, as we are implementing it per block. Block PR's will be attached as PR's in the tasks list.
#35300

[farhan]

Going to close this story as further progress/implementation could be followed on
#35300