ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make typeset math and math image match 

at least get them much closer.
1. typeset math using svg by default, which is how the image is generated
2. size the image so it matches the surrounding font-size

Also, still typeset math from images even if the image fails to load.

closes MAT-501
flag=new_math_equation_handling

SIZING MATH REQUIRES A COMPANION CHANGE TO MATHMAN (the other stuff doesn't)
  - clone git clone ssh://<yourid>@gerrit.instructure.com:29418/mathman
    and follow the instructions for building.
  - checkout https://gerrit.instructure.com/c/mathman/+/277389
  - build it (once built, you don't have to run in in docker)
  - in a canvas rails console:
    Setting.set(
      'equation_image_url,
      'http://localhost:8000/svg?tex='
    )
    http://localhost:8000 is where mathman is via `node app.js`
    it's at http://mathman.docker if in docker (at least that's
    what the README says, I don't really know)
  - test mathman by requesting `http://<mathman-url>/svg?tex=17`
    and expect an image of "17". now try with `request /svg?tex=17&scale=2`
    and your "17" shoul be twice as large.
  - restart your canvas server

test plan:
  Updated typesetting:
  - have a question_bank quetsion with math and answers with math
  - include the question in a quiz
  > verify that the question looks the same in the question bank
    as it does when previewing/taking the quiz
    - if your math includes a "g", it should look the same
    - the size of the equation should be pretty close
  Sizing to match:
  - turn on scale_math_equations site admin feature
  - put an equation w/in a paragraph with a large font, or
    add an equation to the rce, select it, and change the font size
  - click on the equation, edit equation, click insert equation
    w/o changing anything
  > expect the equation to match the larger font.

  - extra credit: DO THIS WITH AND WITHOUT MATHMAN PLUGIN ENABLED
    (yes, there are 2 paths canvas takes to mathman)
    make sure to update dyanmic-settings.yml's math-man entry
    math-man:
      base_url: 'http://localhost:8000'
      use_for_svg: 'true'
      use_for_mml: 'false'
  - at /plugins, enable mathman plugin and ensure use_for_svg istrue

  Error handling:
  - change the URL to mathman to a dead end
  - load a page with math equation images
  > expect the math to be typeset by mathjax

Change-Id: I354e98a0a0256740ce5b4937f5b4e3adc690fe51
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/277245
Reviewed-by: Weston Dransfield <wdransfield@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Weston Dransfield <wdransfield@instructure.com>
Product-Review: David Lyons <lyons@instructure.com>
This commit is contained in:
Ed Schiebel 2021-11-02 08:52:12 -04:00
parent 0a88158e34
commit 2a277632c2
17 changed files with 126 additions and 154 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
app/controllers/application_controller.rb
View File

@ -267,7 +267,8 @@ class ApplicationController < ActionController::Base
JS_ENV_SITE_ADMIN_FEATURES = [
:cc_in_rce_video_tray, :featured_help_links,
:strip_origin_from_quiz_answer_file_references, :rce_buttons_and_icons, :important_dates, :feature_flag_filters, :k5_parent_support,
:conferencing_in_planner, :remember_settings_tab, :word_count_in_speed_grader, :observer_picker, :lti_platform_storage
:conferencing_in_planner, :remember_settings_tab, :word_count_in_speed_grader, :observer_picker, :lti_platform_storage,
:scale_equation_images
].freeze
JS_ENV_ROOT_ACCOUNT_FEATURES = [
:responsive_awareness, :responsive_misc, :product_tours, :files_dnd, :usage_rights_discussion_topics,

8
app/controllers/equation_images_controller.rb
View File

@ -21,6 +21,7 @@ class EquationImagesController < ApplicationController
# Facade to codecogs API for gif generation or microservice MathMan for svg
def show
@latex = params[:id]
@scale = params[:scale] if Account.site_admin.feature_enabled?(:scale_equation_images)
# Usually, the latex string is stored in the db double escaped. By the
# time the value gets here as `params[:id]` it has been unescaped once.
@ -39,9 +40,12 @@ class EquationImagesController < ApplicationController
def url
if MathMan.use_for_svg?
MathMan.url_for(latex: @latex, target: :svg)
MathMan.url_for(latex: @latex, target: :svg, scale: @scale)
else
Setting.get('equation_image_url', 'http://latex.codecogs.com/gif.latex?') + @latex
scale_param = "&scale=#{@scale}" if @scale.present?
scale_param ||= ''
Setting.get('equation_image_url', 'http://latex.codecogs.com/gif.latex?') + @latex +
scale_param
end
end
end

6
app/stylesheets/our_custom_tiny_mce_stuff/_tinymce.editor_box.scss
View File

@ -16,7 +16,8 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
a.youtube_link_to_box, link.youbute_link_to_box {
a.youtube_link_to_box,
link.youbute_link_to_box {
width: 425px !important;
border: 1px solid #aaa;
display: block;
@ -32,7 +33,8 @@ a.youtube_link_to_box, link.youbute_link_to_box {
}
a.instructure_inline_media_comment,
a.instructure_inline_media_comment.mce-item-anchor { // when old rce content is loaded in the new rce
a.instructure_inline_media_comment.mce-item-anchor {
// when old rce content is loaded in the new rce
display: block;
width: 270px !important;
border: 1px solid #aaa;

1
app/stylesheets/pages/quizzes/_quizzes.scss
View File

@ -178,6 +178,7 @@ li.quiz {
background: $backgroundColor;
border: 1px solid #fff;
padding: 16px;
font-size: 1rem;
}
.quiz-submission {

5
config/feature_flags/learning_activities_rce.yml
View File

@ -11,3 +11,8 @@ cc_in_rce_video_tray:
display_name: RCE Edit Closed Captions
description: Edit closed captions/subtitles to videos from the video options tray in the RCE
applies_to: SiteAdmin
scale_equation_images:
state: hidden
display_name: 'Math: Scale equation images'
description: Scale mathman equation images to match the size of the surrounding font.
applies_to: SiteAdmin

5
lib/math_man.rb
View File

@ -22,9 +22,10 @@ require "addressable/uri"
module MathMan
class InvalidConfigurationError < StandardError; end
def self.url_for(latex:, target:)
def self.url_for(latex:, target:, scale: '')
scale_param = "&scale=#{scale}" if scale.present?
uri = base_url.join(target.to_s)
uri.query = "tex=#{latex}"
uri.query = "tex=#{latex}#{scale_param}"
uri.to_s
end

29
packages/canvas-rce/src/rce/RCEWrapper.js
View File

@ -323,7 +323,8 @@ class RCEWrapper extends React.Component {
path: [],
wordCount: 0,
editorView: props.editorView || WYSIWYG_VIEW,
shouldShowOnFocusButton: (props.renderKBShortcutModal === undefined ? true : props.renderKBShortcutModal),
shouldShowOnFocusButton:
props.renderKBShortcutModal === undefined ? true : props.renderKBShortcutModal,
KBShortcutModalOpen: false,
messages: [],
announcement: null,
@ -593,6 +594,32 @@ class RCEWrapper extends React.Component {
this.contentInserted(element)
}
insertMathEquation(tex) {
const ed = this.mceInstance()
const docSz =
parseFloat(
ed.dom.doc.defaultView.getComputedStyle(ed.dom.doc.body).getPropertyValue('font-size')
) || 1
const imgParent = ed.selection.getNode()?.parentElement
const imgSz = imgParent
? parseFloat(
ed.dom.doc.defaultView.getComputedStyle(imgParent).getPropertyValue('font-size')
) || 1
: docSz
let scale = imgSz / docSz
const url = `/equation_images/${encodeURIComponent(encodeURIComponent(tex))}?scale=${scale}`
// if I simply create the html string, xsslint fails jenkins
const img = document.createElement('img')
img.setAttribute('alt', `LaTeX: ${tex}`)
img.setAttribute('title', tex)
img.setAttribute('class', 'equation_image')
img.setAttribute('data-equation-content', tex)
img.setAttribute('src', url)
this.insertCode(img.outerHTML)
}
removePlaceholders(name) {
const placeholder = this.mceInstance().dom.doc.querySelector(
`[data-placeholder-for="${encodeURIComponent(name)}"]`

2
packages/canvas-rce/src/rce/plugins/instructure_equation/clickCallback.js
View File

@ -16,7 +16,7 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
export default function(ed, document) {
export default function (ed, document) {
const ev = document.createEvent('CustomEvent')
ev.initCustomEvent('tinyRCE/initEquation', true, true, {ed})
document.dispatchEvent(ev)

29
packages/canvas-rce/test/module/RCEWrapper.test.js
View File

@ -135,14 +135,16 @@ describe('RCEWrapper', () => {
remove: elem => {
elem.remove()
},
doc: document.createElement('div')
doc: document
},
selection: {
getEnd: () => {
return 0
},
getNode: () => {
return null
const div = document.createElement('div')
document.body.appendChild(div)
return div
},
getContent: () => {
return ''
@ -201,6 +203,7 @@ describe('RCEWrapper', () => {
++failedCount
}
document.body.innerHTML = ''
sinon.reset()
})
after(() => {
@ -339,9 +342,18 @@ describe('RCEWrapper', () => {
contentInsertion.insertLink.restore()
})
it('inserts math equations', () => {
const tex = 'y = x^2'
const dbl_encoded_tex = window.encodeURIComponent(window.encodeURIComponent(tex))
const img_html = `<img alt="LaTeX: ${tex}" title="${tex}" class="equation_image" data-equation-content="${tex}" src="/equation_images/${dbl_encoded_tex}?scale=1">`
sinon.stub(contentInsertion, 'insertContent')
instance.insertMathEquation(tex)
assert.ok(contentInsertion.insertContent.calledWith(editor, img_html))
})
describe('checkReadyToGetCode', () => {
afterEach(() => {
editor.dom.doc = document.createElement('div') // reset
editor.dom.doc.body.innerHTML = ''
})
it('returns true if there are no elements with data-placeholder-for attributes', () => {
assert.ok(instance.checkReadyToGetCode(() => {}))
@ -350,7 +362,7 @@ describe('RCEWrapper', () => {
it('calls promptFunc if there is an element with data-placeholder-for attribute', () => {
const placeholder = document.createElement('img')
placeholder.setAttribute('data-placeholder-for', 'image1')
editor.dom.doc.appendChild(placeholder)
editor.dom.doc.body.appendChild(placeholder)
const spy = sinon.spy()
instance.checkReadyToGetCode(spy)
sinon.assert.calledWith(
@ -362,7 +374,7 @@ describe('RCEWrapper', () => {
it('returns true if promptFunc returns true', () => {
const placeholder = document.createElement('img')
placeholder.setAttribute('data-placeholder-for', 'image1')
editor.dom.doc.appendChild(placeholder)
editor.dom.doc.body.appendChild(placeholder)
const stub = sinon.stub().returns(true)
assert.ok(instance.checkReadyToGetCode(stub))
})
@ -370,7 +382,7 @@ describe('RCEWrapper', () => {
it('returns false if promptFunc returns false', () => {
const placeholder = document.createElement('img')
placeholder.setAttribute('data-placeholder-for', 'image1')
editor.dom.doc.appendChild(placeholder)
editor.dom.doc.body.appendChild(placeholder)
const stub = sinon.stub().returns(false)
assert.ok(!instance.checkReadyToGetCode(stub))
})
@ -576,7 +588,7 @@ describe('RCEWrapper', () => {
it('removes placeholders that match the given name', () => {
const placeholder = document.createElement('img')
placeholder.setAttribute('data-placeholder-for', 'image1')
editor.dom.doc.appendChild(placeholder)
editor.dom.doc.body.appendChild(placeholder)
instance.removePlaceholders('image1')
assert.ok(!editor.dom.doc.querySelector(`[data-placeholder-for="image1"]`))
})
@ -586,7 +598,7 @@ describe('RCEWrapper', () => {
placeholder.setAttribute('data-placeholder-for', 'image1')
const placeholder2 = document.createElement('img')
placeholder2.setAttribute('data-placeholder-for', 'image2')
editor.dom.doc.appendChild(placeholder2)
editor.dom.doc.body.appendChild(placeholder2)
instance.removePlaceholders('image1')
assert.ok(!editor.dom.doc.querySelector(`[data-placeholder-for="image1"]`))
assert.ok(editor.dom.doc.querySelector(`[data-placeholder-for="image2"]`))
@ -636,7 +648,6 @@ describe('RCEWrapper', () => {
})
it('inserts embed code', () => {
sinon.stub(contentInsertion, 'insertContent')
instance.insertEmbedCode('embed me!')
assert(insertedSpy.called)
})

48
packages/mathml/index.js
View File

@ -16,6 +16,7 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
// eslint-disable-next-line import/no-extraneous-dependencies
import $ from 'jquery'
// configure MathJax to use 'color' extension fo LaTeX coding
@ -39,7 +40,7 @@ const localConfig = {
}
const mathml = {
loadMathJax(configFile = 'TeX-MML-AM_HTMLorMML', cb = null) {
loadMathJax(configFile = 'TeX-MML-AM_SVG', cb = null) {
if (this.preventMathJax()) {
return
}
@ -235,9 +236,14 @@ const mathImageHelper = {
let equation_text
const src = img.getAttribute('src')
if (src) {
const srceq = src.split('/equation_images/')[1]
if (srceq) {
equation_text = decodeURIComponent(decodeURIComponent(srceq))
try {
const url = new URL(src, 'http://localhost') // we don't care about the host
const srceq = url.pathname.split('/equation_images/')[1]
if (srceq) {
equation_text = decodeURIComponent(decodeURIComponent(srceq))
}
} catch (_ex) {
// If we failed to parse the src URL, something is wrong.
}
}
return equation_text
@ -248,24 +254,19 @@ const mathImageHelper = {
const eqimgs = refnode.querySelectorAll('img.equation_image')
if (eqimgs.length > 0) {
eqimgs.forEach(img => {
if (img.complete && img.naturalWidth) {
// only process loaded images
const equation_text = this.getImageEquationText(img)
if (equation_text) {
img.setAttribute('mathjaxified', '')
const equation_text = this.getImageEquationText(img)
if (equation_text) {
const mathtex = document.createElement('span')
mathtex.setAttribute('class', 'math_equation_latex')
mathtex.setAttribute('style', img.getAttribute('style'))
mathtex.textContent = `\\(${equation_text}\\)`
mathtex.style.maxWidth = ''
if (img.nextSibling) {
img.parentElement.insertBefore(mathtex, img.nextSibling)
} else {
img.parentElement.appendChild(mathtex)
}
const mathtex = document.createElement('span')
mathtex.setAttribute('class', 'math_equation_latex')
mathtex.setAttribute('style', img.getAttribute('style'))
mathtex.textContent = `\\(${equation_text}\\)`
mathtex.style.maxWidth = ''
if (img.nextSibling) {
img.parentElement.insertBefore(mathtex, img.nextSibling)
} else {
img.parentElement.appendChild(mathtex)
}
} else {
img.addEventListener('load', this.dispatchProcessNewMathOnLoad)
}
})
return true
@ -281,13 +282,6 @@ const mathImageHelper = {
})
},
dispatchProcessNewMathOnLoad(event) {
event.target.removeEventListener('load', this.dispatchProcessNewMathOnLoad)
window.dispatchEvent(
new CustomEvent('process-new-math', {detail: {target: event.target.parentElement}})
)
},
nearlyInfiniteStyleFix(elem) {
elem.querySelectorAll('[style*=clip], [style*=vertical-align]').forEach(e => {
let changed = false

7
spec/coffeescripts/views/tinymce/EquationEditorViewSpec.js
View File

@ -19,13 +19,6 @@ import $ from 'jquery'
import EquationEditorView from '@canvas/rce/backbone/views/EquationEditorView'
QUnit.module('EquationEditorView', () => {
QUnit.module('doubleEncodeEquationForUrl', () => {
test('encodes pound sign using utf-8', assert => {
const equation = '\xA3'
assert.equal(EquationEditorView.doubleEncodeEquationForUrl(equation), '%25C2%25A3')
})
})
QUnit.module('getEquationText', () => {
test("just uses the text if it isn't really an element", assert => {
const equation = '65 * 32'

21
spec/controllers/equation_images_controller_spec.rb
View File

@ -39,18 +39,25 @@ describe EquationImagesController do
end
it 'redirects image requests to codecogs' do
get 'show', params: { :id => 'foo' }
get 'show', params: { id: 'foo' }
expect(response).to redirect_to('http://latex.codecogs.com/gif.latex?foo')
end
it 'includes scale param if present' do
Account.site_admin.enable_feature!(:scale_equation_images)
get 'show', params: { id: 'foo', scale: 2 }
expect(response).to redirect_to('http://latex.codecogs.com/gif.latex?foo&scale=2')
end
it 'omits scale param if feature is off present' do
Account.site_admin.disable_feature!(:scale_equation_images)
get 'show', params: { id: 'foo', scale: 2 }
expect(response).to redirect_to('http://latex.codecogs.com/gif.latex?foo')
end
context 'when using MathMan' do
let(:service_url) { 'http://get.mml.com' }
before do
allow(MathMan).to receive_messages(
url_for: service_url,
use_for_svg?: true
)
end
before { allow(MathMan).to receive_messages(url_for: service_url, use_for_svg?: true) }
it 'redirects to service_url' do
get :show, params: { id: '5' }

41
spec/javascripts/jsx/mathmlSpec.js
View File

@ -231,52 +231,21 @@ test('getImageEquationText returns undefined if it is not an equation image', ()
equal(mathImageHelper.getImageEquationText(img), undefined)
})
test('catchEquationImages only processes loaded images', assert => {
const done = assert.async()
test('catchEquationImages processes equation images', () => {
const root = document.getElementById('fixtures')
root.innerHTML = `
<img id="i1"
class="equation_image"
src="https://www.instructure.com/themes/instructure_blog/images/logo.svg?_time=${Date.now()}"
>
<img id="i2"
class="equation_image"
src="data:image/gif;base64,R0lGODdhDAAMAIABAMzMzP///ywAAAAADAAMAAACFoQfqYeabNyDMkBQb81Uat85nxguUAEAOw=="
>
`
window.setTimeout(() => {
mathImageHelper.catchEquationImages(root)
equal(document.querySelectorAll('img[mathjaxified]').length, 1)
done()
}, 0)
})
test('catchEquationImages defers processing images until loaded', assert => {
const done = assert.async()
const root = document.getElementById('fixtures')
const spy = sinon.spy(mathImageHelper, 'dispatchProcessNewMathOnLoad')
root.innerHTML = `
<img id="i1"
<img id="i2"
class="equation_image"
data-equation-content="x"
src="https://www.instructure.com/themes/instructure_blog/images/logo.svg?_time=${Date.now()}"
src="http://localhost:3000/equation_images/17?scale=1.5"
>
`
mathImageHelper.catchEquationImages(root)
equal(document.querySelectorAll('img[mathjaxified]').length, 0)
equal(spy.callCount, 0)
document
.getElementById('i1')
.setAttribute(
'src',
'data:image/gif;base64,R0lGODdhDAAMAIABAMzMzP///ywAAAAADAAMAAACFoQfqYeabNyDMkBQb81Uat85nxguUAEAOw=='
)
window.setTimeout(() => {
equal(spy.callCount, 1)
done()
}, 0)
equal(document.querySelectorAll('img[mathjaxified]').length, 1)
equal(document.querySelector('.math_equation_latex').textContent, '\\(17\\)')
})
test('removeStrayEquationImages only removes tagged images', () => {

8
spec/lib/math_man_spec.rb
View File

@ -66,6 +66,14 @@ describe MathMan do
Canvas::DynamicSettings.fallback_data = nil
expect { MathMan.url_for(latex: latex, target: :mml) }.to raise_error MathMan::InvalidConfigurationError
end
it 'includes scale param if present' do
expect(MathMan.url_for(latex: latex, target: :svg, scale: '2')).to match(/&scale=2/)
end
it 'excludes scale param if not present' do
expect(MathMan.url_for(latex: latex, target: :svg)).not_to match(/&scale=2/)
end
end
describe '.use_for_mml?' do

5
ui/features/navigation_header/jquery/instructure.js vendored
View File

@ -125,7 +125,10 @@ export function enhanceUserContent() {
)
}
}
if (typeof img.style.width !== 'string' || !img.style.width.endsWith('%')) {
if (
(typeof img.style.width !== 'string' || !img.style.width.endsWith('%')) &&
!img.classList.contains('equation_image')
) {
if (img.naturalWidth === 0) {
img.addEventListener('load', handleWidth)
} else {

3
ui/index.js
View File

@ -163,7 +163,9 @@ ready(() => {
// This is in a setTimeout to have it run on the next time through the event loop
// so that the code that actually renders the user_content runs first,
// because it has to be rendered before we can check if isMathOnPage
let processedBodyMath = false
setTimeout(() => {
processedBodyMath = true
window.dispatchEvent(
new CustomEvent(mathml.processNewMathEventName, {
detail: {target: document.body}
@ -172,6 +174,7 @@ ready(() => {
}, 0)
const observer = new MutationObserver((mutationList, _observer) => {
if (!processedBodyMath) return
for (let m = 0; m < mutationList.length; ++m) {
if (mutationList[m]?.addedNodes?.length) {
const addedNodes = mutationList[m].addedNodes

59
ui/shared/rce/backbone/views/EquationEditorView.js
View File

@ -245,16 +245,6 @@ export default class EquationEditorView extends Backbone.View {
this.$el.dialog('close')
}
// the following is here to make it easier to unit test
static doubleEncodeEquationForUrl(text) {
return encodeURIComponent(encodeURIComponent(text))
}
// the following will be called by onSubmit below
doubleEncodeEquationForUrl(text) {
return this.constructor.doubleEncodeEquationForUrl(text)
}
onSubmit(event) {
event.preventDefault()
@ -265,56 +255,9 @@ export default class EquationEditorView extends Backbone.View {
return
}
// get the equation image to check that it succeeds
// if it does, we'll send its html to the RCE, where
// the image will get pulled from the cache, so the 2nd
// request won't cost much
// NOTE: commented out because the service used in prod
// will not accept a CORS request
const url = `/equation_images/${this.doubleEncodeEquationForUrl(text)}`
// fetch(url, {
// method: 'GET',
// mode: 'cors',
// redirect: 'follow'
// })
// .then(response => {
this.restoreCaret()
// if (response.ok) {
const code = this.loadImage(text, url)
RceCommandShim.send(this.$editor, 'insert_code', code)
// } else {
// const code = this.loadAltMath(text)
// this.editor.selection.setContent(code)
// }
RceCommandShim.send(this.$editor, 'insertMathEquation', text)
this.close()
// })
// .catch(() => {
// const code = this.loadAltMath(text)
// this.editor.selection.setContent(code)
// this.close()
// })
}
// the image generator was successful
loadImage(text, url) {
// if I simple create the html string, xsslint fails jenkins
const img = document.createElement('img')
img.setAttribute('alt', `LaTeX: ${text}`)
img.setAttribute('title', text)
img.setAttribute('class', 'equation_image')
img.setAttribute('data-equation-content', text)
img.setAttribute('src', url)
return img.outerHTML
}
// there are LaTex equations the the image generator can't deal with
// that MathJax can. If the image failed, let's inject the LaTex
// as inline math for MathJax to process later.
loadAltMath(text) {
const span = document.createElement('span')
span.setAttribute('class', 'math_equation_latex')
span.textContent = `\\(${text}\\)`
return span.outerHTML
}
}
EquationEditorView.initClass()