From 9930acbbfbcdf7b78a08907c6e07e023433fcf64 Mon Sep 17 00:00:00 2001 From: Ed Schiebel Date: Tue, 27 Jul 2021 11:13:08 -0400 Subject: [PATCH] Harvard RCE tweaks - not directly related to the RCE, but now preload the 3 lato-exended font variants likely to be used on the page. this keeps font loading from blocking - the big change is to make it possible to limit the number of RCEs that will fully render on page load. This should address the loading issues for quizzes with many essay questions (the sample quiz from Harvard had 30 + other questions) For now, the necessary property is only being set on the take-quiz page closes: MAT-355 flag=rce_enhancements,flag=rce_limit_init_render_on_page test plan: account flag. - create a legacy quiz with > 5 essay questions. Feel free to have other questions in there too if you like - preview or take the quiz > expect all 6 RCEs to be created on the page - you could use React dev tools or document.querySelectorAll('.rce-wrapper').length === 6 - turn the "RCE Limit number of RCEs initially rendered on th page" account feature flag on - take the quiz again > expect only the first 5 RCEs to be fully rendered - scroll down to the bring the 6th into view > expect it to be fully realized - while scrolled to the bottom, refresh the page > not that the 6th RCE is in view, expect it to be fully realized also Change-Id: Idd76a56c4ea69e45a4f1cc28e3cd8561b40c2403 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/270075 Tested-by: Service Cloud Jenkins QA-Review: Weston Dransfield Reviewed-by: Jon Scheiding Product-Review: Ed Schiebel --- app/controllers/application_controller.rb | 2 +- app/helpers/application_helper.rb | 5 + app/views/layouts/_head.html.erb | 3 + config/brandable_css.yml | 1 + .../feature_flags/learning_activities_rce.yml | 8 ++ lib/brandable_css.rb | 32 +++++ package.json | 1 + packages/canvas-rce/src/rce/RCE.js | 7 ++ packages/canvas-rce/src/rce/RCEWrapper.js | 107 +++++++++++++---- packages/canvas-rce/src/rce/wrapInitCb.js | 32 ++--- .../canvas-rce/test/module/RCEWrapper.test.js | 110 +++++++++++++++--- .../jsx/shared/rce/serviceRCELoaderSpec.js | 39 ++++--- spec/lib/brandable_css_spec.rb | 16 +++ ui/features/take_quiz/jquery/index.js | 3 +- ui/shared/rce/react/CanvasRce.js | 10 ++ ui/shared/rce/serviceRCELoader.js | 7 +- 16 files changed, 309 insertions(+), 74 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1b54b78cd47..db107dba6d7 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -237,7 +237,7 @@ class ApplicationController < ActionController::Base ].freeze JS_ENV_ROOT_ACCOUNT_FEATURES = [ :responsive_awareness, :responsive_misc, :product_tours, :module_dnd, :files_dnd, :unpublished_courses, - :usage_rights_discussion_topics, :inline_math_everywhere, :granular_permissions_manage_users + :usage_rights_discussion_topics, :inline_math_everywhere, :granular_permissions_manage_users, :rce_limit_init_render_on_page ].freeze JS_ENV_BRAND_ACCOUNT_FEATURES = [ :embedded_release_notes diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 685b7e18700..3b9a20eeb5a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -304,6 +304,11 @@ module ApplicationHelper File.join('/dist', 'brandable_css', base_dir, "#{bundle_path}-#{cache[:combinedChecksum]}.css") end + def font_url_for(nominal_font_href) + cache = BrandableCSS.font_path_cache() + cache[nominal_font_href] || nominal_font_href + end + def brand_variable(variable_name) BrandableCSS.brand_variable_value(variable_name, active_brand_config) end diff --git a/app/views/layouts/_head.html.erb b/app/views/layouts/_head.html.erb index 23f3e7ea99c..7789a3d2df2 100644 --- a/app/views/layouts/_head.html.erb +++ b/app/views/layouts/_head.html.erb @@ -21,6 +21,9 @@ <% if Setting.get('disable_lato_extended', false) == 'false' %> + " as="font" type="font/woff2" crossorigin> + " as="font" type="font/woff2" crossorigin> + " as="font" type="font/woff2" crossorigin> <%= stylesheet_link_tag(css_url_for('lato_extended')) %> <% else %> <%= stylesheet_link_tag(css_url_for('lato')) %> diff --git a/config/brandable_css.yml b/config/brandable_css.yml index 08f83f083be..d0669c83ff4 100644 --- a/config/brandable_css.yml +++ b/config/brandable_css.yml @@ -7,6 +7,7 @@ paths: file_checksums: public/dist/brandable_css/brandable_css_file_checksums.json output_dir: public/dist/brandable_css browsers_yml: config/browsers.yml + rev_manifest: public/dist/rev-manifest.json indices: handlebars: diff --git a/config/feature_flags/learning_activities_rce.yml b/config/feature_flags/learning_activities_rce.yml index 7fae40d70f9..dae1f536d71 100644 --- a/config/feature_flags/learning_activities_rce.yml +++ b/config/feature_flags/learning_activities_rce.yml @@ -26,3 +26,11 @@ rce_better_file_previewing: display_name: RCE Better File Previewing description: Preview files linked in the RCE in an overlay rather than a new browser tab. applies_to: SiteAdmin +rce_limit_init_render_on_page: + state: hidden + display_name: RCE Limit number of RCEs initially rendered on the page + description: |- + Cap the number of RCEs rendered on initial page load. After that + initialize each as it is about to enter the viewport. + applies_to: RootAccount + root_opt_in: true diff --git a/lib/brandable_css.rb b/lib/brandable_css.rb index 61227649274..bccd21290ab 100644 --- a/lib/brandable_css.rb +++ b/lib/brandable_css.rb @@ -329,6 +329,38 @@ module BrandableCSS fingerprint end + # build a cache of nominal paths to font files to the decorated version we need to request + # e.g. "/fonts/lato/extended/Lato-Bold.woff2": "/dist/fonts/lato/extended/Lato-Bold-cccb897485.woff2" + # only track .woff2 font files since those will be the only ones ever asked for + # (truth be told, could limit it to just lato extended) + # this function is more or less modeled after combined_checksums + def font_path_cache + return @decorated_font_paths if defined?(@decorated_font_paths) && defined?(ActionController) && ActionController::Base.perform_caching + + file = APP_ROOT.join(CONFIG.dig('paths', 'rev_manifest')) + + # in reality, if the rev-manifest.json file is missing you won't get this far, but let's be careful anyway + return( + @decorated_font_paths = + JSON.parse(file.read).each_with_object({}) do |(k, v), memo| + memo["/#{k}"] = "/dist/#{v}" if k =~ /^fonts.*woff2/ + end.freeze + ) if file.exist? + + # the file does not exist in production, we have a problem + if defined?(Rails) && Rails.env.production? + raise "#{file.expand_path} does not exist. You need to run brandable_css before you can serve css." + end + + # for dev/test there might be cases where you don't want it to raise an exception + # if you haven't run `brandable_css` and the manifest file doesn't exist yet. + # eg: you want to test a controller action and you don't care that it links + # to a css file that hasn't been created yet. + @decorated_font_paths = { + anyfont: 'Error: unknown css checksum. you need to run brandable_css' + }.freeze + end + def all_fingerprints_for(bundle_path) variants.each_with_object({}) do |variant, object| object[variant] = cache_for(bundle_path, variant) diff --git a/package.json b/package.json index f91c7ecc75f..6e3ec90e6c0 100644 --- a/package.json +++ b/package.json @@ -336,6 +336,7 @@ "build": "yarn run build:css && yarn run build:js", "build:watch": "concurrently --raw \"yarn build:css:watch\" \"yarn build:js:watch\"", "build:css": "brandable_css", + "build:css:compressed": "SASS_STYLE=compressed brandable_css", "build:css:watch": "brandable_css --watch", "build:js": "yarn run webpack-development", "build:js:watch": "yarn run webpack", diff --git a/packages/canvas-rce/src/rce/RCE.js b/packages/canvas-rce/src/rce/RCE.js index 7a47605effe..aa7c67ea3f1 100644 --- a/packages/canvas-rce/src/rce/RCE.js +++ b/packages/canvas-rce/src/rce/RCE.js @@ -168,6 +168,12 @@ RCE.propTypes = { // array of lti tools available to the user // {id, favorite} are all that's required, ther fields are ignored ltiTools: ltiToolsPropType, + // if the rce_limit_init_render_on_page flag is on, this + // is the maximum number of RCEs that will render on page load. + // Any more than this will be deferred until it is nearly + // scrolled into view. + // if isNaN or <=0, render them all + maxInitRenderedRCEs: number, // name:value pairs of attributes to add to the textarea // tinymce creates as the backing store of the RCE mirroredAttrs: objectOf(string), @@ -201,6 +207,7 @@ RCE.defaultProps = { instRecordDisabled: false, language: 'en', liveRegion: () => document.getElementById('flash_screenreader_holder'), + maxInitRenderedRCEs: -1, mirroredAttrs: {}, readOnly: false, use_rce_pretty_html_editor: true, diff --git a/packages/canvas-rce/src/rce/RCEWrapper.js b/packages/canvas-rce/src/rce/RCEWrapper.js index a4d84330bf2..94880d8e85c 100644 --- a/packages/canvas-rce/src/rce/RCEWrapper.js +++ b/packages/canvas-rce/src/rce/RCEWrapper.js @@ -270,6 +270,8 @@ class RCEWrapper extends React.Component { plugins: PropTypes.arrayOf(PropTypes.string), instRecordDisabled: PropTypes.bool, highContrastCSS: PropTypes.arrayOf(PropTypes.string), + maxInitRenderedRCEs: PropTypes.number, + // feature flag related props use_rce_pretty_html_editor: PropTypes.bool, use_rce_buttons_and_icons: PropTypes.bool, use_rce_a11y_checker_notifications: PropTypes.bool @@ -280,7 +282,8 @@ class RCEWrapper extends React.Component { languages: [{id: 'en', label: 'English'}], autosave: {enabled: false}, highContrastCSS: [], - ltiTools: [] + ltiTools: [], + maxInitRenderedRCEs: -1 } static skinCssInjected = false @@ -300,6 +303,7 @@ class RCEWrapper extends React.Component { this.indicator = false this._elementRef = React.createRef() + this._editorPlaceholderRef = React.createRef() this._prettyHtmlEditorRef = React.createRef() this._showOnFocusButton = null @@ -310,6 +314,11 @@ class RCEWrapper extends React.Component { ht = `${ht}px` } + const currentRCECount = document.querySelectorAll('.rce-wrapper').length + const maxInitRenderedRCEs = Number.isNaN(props.maxInitRenderedRCEs) + ? RCEWrapper.defaultProps.maxInitRenderedRCEs + : props.maxInitRenderedRCEs + this.state = { path: [], wordCount: 0, @@ -325,8 +334,13 @@ class RCEWrapper extends React.Component { headerDisp: 'static', isTinyFullscreen: false }, - a11yErrorsCount: 0 + a11yErrorsCount: 0, + shouldShowEditor: + typeof IntersectionObserver === 'undefined' || + maxInitRenderedRCEs <= 0 || + currentRCECount < maxInitRenderedRCEs } + this.pendingEventHandlers = [] // Get top 2 favorited LTI Tools this.ltiToolFavorites = @@ -605,6 +619,15 @@ class RCEWrapper extends React.Component { return contentInsertion.existingContentToLinkIsImg(editor) } + // since we may defer rendering tinymce, queue up any tinymce event handlers + tinymceOn(tinymceEventName, handler) { + if (this.state.shouldShowEditor) { + this.mceInstance().on(tinymceEventName, handler) + } else { + this.pendingEventHandlers.push({name: tinymceEventName, handler}) + } + } + mceInstance() { if (this.editor) { return this.editor @@ -1334,12 +1357,15 @@ class RCEWrapper extends React.Component { } componentWillUnmount() { - window.clearTimeout(this.blurTimer) - if (!this._destroyCalled) { - this.destroy() + if (this.state.shouldShowEditor) { + window.clearTimeout(this.blurTimer) + if (!this._destroyCalled) { + this.destroy() + } + this._elementRef.current.removeEventListener('keydown', this.handleKey, true) + this.mutationObserver?.disconnect() + this.intersectionObserver?.disconnect() } - this._elementRef.current.removeEventListener('keydown', this.handleKey, true) - this.observer?.disconnect() } wrapOptions(options = {}) { @@ -1555,6 +1581,46 @@ class RCEWrapper extends React.Component { } componentDidMount() { + if (this.state.shouldShowEditor) { + this.editorReallyDidMount() + } else { + this.intersectionObserver = new IntersectionObserver( + entries => { + const entry = entries[0] + if (entry.isIntersecting || entry.intersectionRatio > 0) { + this.setState({shouldShowEditor: true}) + } + }, + // initialize the RCE when it gets close to entering the viewport + {root: null, rootMargin: '200px 0px', threshold: 0.0} + ) + this.intersectionObserver.observe(this._editorPlaceholderRef.current) + } + } + + componentDidUpdate(prevProps, prevState) { + if (this.state.shouldShowEditor) { + if (!prevState.shouldShowEditor) { + this.editorReallyDidMount() + this.intersectionObserver?.disconnect() + } else { + this.registerTextareaChange() + if (prevState.editorView !== this.state.editorView) { + this.setEditorView(this.state.editorView) + this.focusCurrentView() + } + if (prevProps.readOnly !== this.props.readOnly) { + this.mceInstance().mode.set(this.props.readOnly ? 'readonly' : 'design') + } + } + } + } + + editorReallyDidMount() { + const myTiny = this.mceInstance() + this.pendingEventHandlers.forEach(e => { + myTiny.on(e.name, e.handler) + }) this.registerTextareaChange() this._elementRef.current.addEventListener('keydown', this.handleKey, true) // give the textarea its initial size @@ -1574,29 +1640,18 @@ class RCEWrapper extends React.Component { // my portal will be the last one in the doc because tinyumce appends them const tinymce_floating_toolbar_portal = portals[portals.length - 1] if (tinymce_floating_toolbar_portal) { - this.observer = new MutationObserver((mutationList, _observer) => { + this.mutationObserver = new MutationObserver((mutationList, _observer) => { mutationList.forEach(mutation => { if (mutation.type === 'childList' && mutation.addedNodes.length > 0) { this.handleFocusEditor(new Event('focus', {target: mutation.target})) } }) }) - this.observer.observe(tinymce_floating_toolbar_portal, {childList: true}) + this.mutationObserver.observe(tinymce_floating_toolbar_portal, {childList: true}) } bridge.renderEditor(this) } - componentDidUpdate(prevProps, prevState) { - this.registerTextareaChange() - if (prevState.editorView !== this.state.editorView) { - this.setEditorView(this.state.editorView) - this.focusCurrentView() - } - if (prevProps.readOnly !== this.props.readOnly) { - this.mceInstance().mode.set(this.props.readOnly ? 'readonly' : 'design') - } - } - setEditorView(view) { switch (view) { case RAW_HTML_EDITOR_VIEW: @@ -1682,6 +1737,18 @@ class RCEWrapper extends React.Component { render() { const {trayProps, ...mceProps} = this.props + if (!this.state.shouldShowEditor) { + return ( +
+ ) + } return (
{ - const target = e.target - let mceSrc - if (target.nodeType === 1 && target.nodeName === 'IMG' && (mceSrc = $(target).data('url'))) { - $(target).attr('src', tinymce.activeEditor.documentBaseURI.toAbsolute(mceSrc)) - } - }) + if (!window.ENV?.use_rce_enhancements) { + // this is a hack so that when you drag an image from the sidebar to the editor that it doesn't + // try to embed the thumbnail but rather the full size version of the image. + // so basically, to document why and how this works: in wiki_sidebar.js we add the + // _mce_src="http://path/to/the/fullsize/image" to the images whose src="path/to/thumbnail/of/image/" + // what this does is check to see if some DOM node that got inserted into the editor has the attribute _mce_src + // and if it does, use that instead. + $(ed.contentDocument).bind('DOMNodeInserted', e => { + const target = e.target + let mceSrc + if ( + target.nodeType === 1 && + target.nodeName === 'IMG' && + (mceSrc = $(target).data('url')) + ) { + $(target).attr('src', tinymce.activeEditor.documentBaseURI.toAbsolute(mceSrc)) + } + }) + } // tiny sets a focusout event handler, which only IE supports // (Chrome/Safari/Opera support DOMFocusOut, FF supports neither) diff --git a/packages/canvas-rce/test/module/RCEWrapper.test.js b/packages/canvas-rce/test/module/RCEWrapper.test.js index 84259587faa..7fa23b9d8ae 100644 --- a/packages/canvas-rce/test/module/RCEWrapper.test.js +++ b/packages/canvas-rce/test/module/RCEWrapper.test.js @@ -17,7 +17,6 @@ */ import assert from 'assert' -import jsdomify from 'jsdomify' import sinon from 'sinon' import Bridge from '../../src/bridge' import * as indicateModule from '../../src/common/indicate' @@ -32,6 +31,7 @@ import RCEWrapper, { const textareaId = 'myUniqId' let React, fakeTinyMCE, editorCommandSpy, sd, editor +let failedCount = 0 // ==================== // HELPERS @@ -88,11 +88,13 @@ function trayProps() { // to provide the default props function defaultProps() { return { + textareaId, highContrastCSS: [], languages: [{id: 'en', label: 'English'}], autosave: {enabled: false}, ltiTools: [], - editorOptions: {} + editorOptions: {}, + liveRegion: () => document.getElementById('flash_screenreader_holder') } } @@ -102,16 +104,14 @@ describe('RCEWrapper', () => { // ==================== beforeEach(() => { - jsdomify.create(` - -
+ document.body.innerHTML = ` +