From 691472e475316786ab3d45959c3456327b5bc4db Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 11 Feb 2021 13:31:17 -0500 Subject: [PATCH] [feat]: Don't use Mathjax if an HTMLBlock has no math. (#26478) Mobile apps load HTML (and other) XBlocks individually using the render_xblock endpoint. This is an attmept to reduce the number of requests and JS processing needed to do so by detecting when we have math content in HTMLBlocks and only adding the Mathjax resources when necessary. This is controlled by the "courseware.optimized_render_xblock" CourseWaffleFlag. For maximum safety, we currently only optimize in this way when directly hitting HTMLBlocks, and not for ProblemBlock or VerticalBlock. This was made as part of edX's Hackathon XXV. --- lms/djangoapps/courseware/tests/test_views.py | 104 +++++++++++++++++- lms/djangoapps/courseware/toggles.py | 15 +++ lms/djangoapps/courseware/views/views.py | 85 +++++++++++++- .../courseware/courseware-chromeless.html | 4 +- 4 files changed, 204 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index ec3427c690..b3d1196bb5 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -58,7 +58,8 @@ from lms.djangoapps.courseware.tests.helpers import get_expiration_banner_text from lms.djangoapps.courseware.testutils import RenderXBlockTestMixin from lms.djangoapps.courseware.toggles import ( COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, - REDIRECT_TO_COURSEWARE_MICROFRONTEND + COURSEWARE_OPTIMIZED_RENDER_XBLOCK, + REDIRECT_TO_COURSEWARE_MICROFRONTEND, ) from lms.djangoapps.courseware.url_helpers import get_microfrontend_url, get_redirect_url from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient @@ -3448,3 +3449,104 @@ class MFERedirectTests(BaseViewsTestCase): # lint-amnesty, pylint: disable=miss with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, active=True): assert self.client.get(lms_url).status_code == 200 + + +class ContentOptimizationTestCase(ModuleStoreTestCase): + """ + Test our ability to make browser optimizations based on XBlock content. + """ + def setUp(self): + super().setUp() + self.math_html_usage_keys = [] + + with self.store.default_store(ModuleStoreEnum.Type.split): + self.course = CourseFactory.create(display_name=u'teꜱᴛ course', run="Testing_course") + with self.store.bulk_operations(self.course.id): + chapter = ItemFactory.create( + category='chapter', + parent_location=self.course.location, + display_name="Chapter 1", + ) + section = ItemFactory.create( + category='sequential', + parent_location=chapter.location, + due=datetime(2013, 9, 18, 11, 30, 00), + display_name='Sequential 1', + format='Homework' + ) + self.math_vertical = ItemFactory.create( + category='vertical', + parent_location=section.location, + display_name='Vertical with Mathjax HTML', + ) + self.no_math_vertical = ItemFactory.create( + category='vertical', + parent_location=section.location, + display_name='Vertical with No Mathjax HTML', + ) + MATHJAX_TAG_PAIRS = [ + (r"\(", r"\)"), + (r"\[", r"\]"), + ("[mathjaxinline]", "[/mathjaxinline]"), + ("[mathjax]", "[/mathjax]"), + ] + for (i, (start_tag, end_tag)) in enumerate(MATHJAX_TAG_PAIRS): + math_html_block = ItemFactory.create( + category='html', + parent_location=self.math_vertical.location, + display_name=f"HTML With Mathjax {i}", + data=f"

Hello Math! {start_tag}x^2 + y^2{end_tag}

", + ) + self.math_html_usage_keys.append(math_html_block.location) + + self.html_without_mathjax = ItemFactory.create( + category='html', + parent_location=self.no_math_vertical.location, + display_name="HTML Without Mathjax", + data="

I talk about mathjax, but I have no actual Math!

", + ) + + self.course_key = self.course.id + self.user = UserFactory(username='staff_user', profile__country='AX', is_staff=True) + self.date = datetime(2013, 1, 22, tzinfo=UTC) + self.enrollment = CourseEnrollment.enroll(self.user, self.course_key) + self.enrollment.created = self.date + self.enrollment.save() + + @override_waffle_flag(COURSEWARE_OPTIMIZED_RENDER_XBLOCK, True) + def test_mathjax_detection(self): + self.client.login(username=self.user.username, password=TEST_PASSWORD) + + # Check the HTML blocks with Math + for usage_key in self.math_html_usage_keys: + url = reverse("render_xblock", kwargs={'usage_key_string': str(usage_key)}) + response = self.client.get(url) + assert response.status_code == 200 + assert b"MathJax.Hub.Config" in response.content + + # Check the one without Math... + url = reverse("render_xblock", kwargs={ + 'usage_key_string': str(self.html_without_mathjax.location) + }) + response = self.client.get(url) + assert response.status_code == 200 + assert b"MathJax.Hub.Config" not in response.content + + # The containing vertical should still return MathJax (for now) + url = reverse("render_xblock", kwargs={ + 'usage_key_string': str(self.no_math_vertical.location) + }) + response = self.client.get(url) + assert response.status_code == 200 + assert b"MathJax.Hub.Config" in response.content + + @override_waffle_flag(COURSEWARE_OPTIMIZED_RENDER_XBLOCK, False) + def test_mathjax_detection_disabled(self): + """Check that we can disable optimizations.""" + self.client.login(username=self.user.username, password=TEST_PASSWORD) + url = reverse("render_xblock", kwargs={ + 'usage_key_string': str(self.html_without_mathjax.location) + }) + response = self.client.get(url) + assert response.status_code == 200 + assert b"MathJax.Hub.Config" in response.content diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index 46ac561abd..46be367250 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -115,6 +115,21 @@ EXAM_RESUME_PROCTORING_IMPROVEMENTS = CourseWaffleFlag( WAFFLE_FLAG_NAMESPACE, 'exam_resume_proctoring_improvements', __name__ ) +# .. toggle_name: courseware.optimized_render_xblock +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag that determines whether we speed up the render_xblock for browsers by +# removing unnecessary JavaScript and CSS. It is possible that this could introduce edge cases with content +# that relies on these assets, so being a CourseWaffleFlag will give us the flexibility to exempt courses +# from these optimizations. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2021-02-09 +# .. toggle_target_removal_date: 2021-05-01 +# .. toggle_warnings: None +COURSEWARE_OPTIMIZED_RENDER_XBLOCK = CourseWaffleFlag( + WAFFLE_FLAG_NAMESPACE, 'optimized_render_xblock', __name__ +) + def course_exit_page_is_active(course_key): return ( diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index b292f3ce9d..acecda81cd 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -33,7 +33,7 @@ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_GET, require_http_methods, require_POST from django.views.generic import View from edx_django_utils import monitoring as monitoring_utils -from edx_django_utils.monitoring import set_custom_attributes_for_course_key +from edx_django_utils.monitoring import set_custom_attribute, set_custom_attributes_for_course_key from ipware.ip import get_ip from markupsafe import escape from opaque_keys import InvalidKeyError @@ -135,6 +135,7 @@ from ..context_processor import user_timezone_locale_prefs from ..entrance_exams import user_can_skip_entrance_exam from ..module_render import get_module, get_module_by_usage_id, get_module_for_descriptor from ..tabs import _get_dynamic_tabs +from ..toggles import COURSEWARE_OPTIMIZED_RENDER_XBLOCK log = logging.getLogger("edx.courseware") @@ -1669,6 +1670,11 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True): usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) course_key = usage_key.course_key + # Gathering metrics to make performance measurements easier. + set_custom_attributes_for_course_key(course_key) + set_custom_attribute('usage_key', usage_key_string) + set_custom_attribute('block_type', usage_key.block_type) + requested_view = request.GET.get('view', 'student_view') if requested_view != 'student_view' and requested_view != 'public_view': # lint-amnesty, pylint: disable=consider-using-in return HttpResponseBadRequest( @@ -1747,8 +1753,11 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True): ) ) + fragment = block.render(requested_view, context=student_view_context) + optimization_flags = get_optimization_flags_for_content(block, fragment) + context = { - 'fragment': block.render(requested_view, context=student_view_context), + 'fragment': fragment, 'course': course, 'disable_accordion': True, 'allow_iframing': True, @@ -1768,10 +1777,82 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True): 'is_learning_mfe': is_learning_mfe, 'is_mobile_app': is_request_from_mobile_app(request), 'reset_deadlines_url': reverse(RESET_COURSE_DEADLINES_NAME), + + **optimization_flags, } return render_to_response('courseware/courseware-chromeless.html', context) +def get_optimization_flags_for_content(block, fragment): + """ + Return a dict with a set of display options appropriate for the block. + + This is going to start in a very limited way. + """ + safe_defaults = { + 'enable_mathjax': True + } + + # Only run our optimizations on the leaf HTML and ProblemBlock nodes. The + # mobile apps access these directly, and we don't have to worry about + # XBlocks that dynamically load content, like inline discussions. + usage_key = block.location + + # For now, confine ourselves to optimizing just the HTMLBlock + if usage_key.block_type != 'html': + return safe_defaults + + if not COURSEWARE_OPTIMIZED_RENDER_XBLOCK.is_enabled(usage_key.course_key): + return safe_defaults + + inspector = XBlockContentInspector(block, fragment) + flags = dict(safe_defaults) + flags['enable_mathjax'] = inspector.has_mathjax_content() + + return flags + + +class XBlockContentInspector: + """ + Class to inspect rendered XBlock content to determine dependencies. + + A lot of content has been written with the assumption that certain + JavaScript and assets are available. This has caused us to continue to + include these assets in the render_xblock view, despite the fact that they + are not used by the vast majority of content. + + In order to try to provide faster load times for most users on most content, + this class has the job of detecting certain patterns in XBlock content that + would imply these dependencies, so we know when to include them or not. + """ + def __init__(self, block, fragment): + self.block = block + self.fragment = fragment + + def has_mathjax_content(self): + """ + Returns whether we detect any MathJax in the fragment. + + Note that this only works for things that are rendered up front. If an + XBlock is capable of modifying the DOM afterwards to inject math content + into the page, this will not catch it. + """ + # The following pairs are used to mark Mathjax syntax in XBlocks. There + # are other options for the wiki, but we don't worry about those here. + MATHJAX_TAG_PAIRS = [ + (r"\(", r"\)"), + (r"\[", r"\]"), + ("[mathjaxinline]", "[/mathjaxinline]"), + ("[mathjax]", "[/mathjax]"), + ] + content = self.fragment.body_html() + for (start_tag, end_tag) in MATHJAX_TAG_PAIRS: + if start_tag in content and end_tag in content: + return True + + return False + + # Translators: "percent_sign" is the symbol "%". "platform_name" is a # string identifying the name of this installation, such as "edX". FINANCIAL_ASSISTANCE_HEADER = _( diff --git a/lms/templates/courseware/courseware-chromeless.html b/lms/templates/courseware/courseware-chromeless.html index 4e61397f80..929b002cb5 100644 --- a/lms/templates/courseware/courseware-chromeless.html +++ b/lms/templates/courseware/courseware-chromeless.html @@ -59,7 +59,9 @@ ${static.get_page_title_breadcrumbs(course_name())} <%static:js group='courseware'/> - <%include file="/mathjax_include.html" args="disable_fast_preview=True"/> + % if enable_mathjax: + <%include file="/mathjax_include.html" args="disable_fast_preview=True"/> + % endif % if staff_access: <%include file="xqa_interface.html"/> % endif