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

mathjax a11y update #19509

Merged
merged 1 commit into from
Jan 17, 2019
Merged

mathjax a11y update #19509

merged 1 commit into from
Jan 17, 2019

Conversation

DawoudSheraz
Copy link
Contributor

@DawoudSheraz DawoudSheraz commented Dec 26, 2018

EDUCATOR-3764

Description

Mathjax provides with the accessibility features to allow access to the rendered content in every possible ways. In current scenario, the files required for the a11y are loaded manually. There is need to enable those files automatically whenever the Mathjax is loaded. This PR addresses the configuration to load the a11y files for the Mathjax by upgrading it to 2.7.5 and adding the required configuration.

Change Impact

  1. Adding the MathJax a11y files led to the discovery of a problem in the Discussion component. Surprisingly, the problem has been there in first place for quite a long time, but only came under the observation after the inclusion of MathJax a11y. The problem was that whenever a response (to a discussion forum) was added/edited, Math Processing Error occurred, which halted the further MathJax Processing on the page, unless page was reloaded.
  2. The initial merge had to be taken down because of the fact that some tests, mainly Lettuce, started to show flaky behavior after the merge. That increased the tests failure ratio.

Problem Identification

  1. Identifying the problem was straightforward, but locating the exact point of the origin was cumbersome. When a response is added, the contents from the input area are taken, and appended to the already present response list. During that process, whenever the render of the response model was called, a Typeset call was queued inside the MathJax queue (Discussion util convertMath function was called inside render). Queue & TypeSet being async performed on their own, while the page DOM modified to append the new response. When a response is successfully appended, the backbone model was updated via set call, which in turn generated relevant events. One of the triggered events called the render method again, which led to the additional typeset calls. The problem here is that previous typesetting didn't complete, and DOM changed. The amalgam of async and sync operations is problematic for MathJax operations, as mentioned in one of the MathJax reported issues (For further study, take a look at the question as well).
  2. Find the test failure rationale was a bit complex as the tests were passing locally (with exception of few bokchoy), but failed on the Jenkins consistently. Not only that, but the behaviors in those tests were working fine on the local as well. One particular thing, that was problem for the DiscussionEditorPreviewTest, was the MathJax a11y files loading time. With the introduced MathJax a11y, MathJax related files took more time to load than without the a11y. This was a problem for the Discussion test, but not for the Lettuce tests.

Problem Resolution

  1. Given the coupling of the various discussion component models, it was rather difficult to convert all the operations either sync or async. One of the possible fixes was to TypeSet the response when the underlying operation finished properly. This meant moving the TypeSet call out from convertMath function, and calling the TypeSet explicitly whenever required. This somehow violated the DRY principle as the same thing was mentioned in the multiple places, but in this case, explicit calls were better than the implicit problematic calls. Hence, for the responses, whenever any operation (adding, editing, etc.) finished, only then Typeset was called. This solved the underlying problem.
  2. For the Discussion test, adding the wait did the trick. Normally, the MathJax a11y files took about 5-10 seconds to load. Since MathJax loaded its files internally, the bokchoy wait method didn't work perfectly. The fix was to halt browser execution for sufficient time to make sure MathJax(and a11y files) loaded perfectly. For the lettuce tests, their equivalent bokchoy tests were written, which mimicked the lettuce tests' functionality.

Other Notes

  1. For the a11y update, the CDN was changed from the cloudflare to jsDelivr. The sole reason for the update is that cloudflare didn't serve the media file. With MathJax , the expression navigator can be turned on to parse the individual component in the expression. With a wrong keypress, an ogg file is requested to indicate a beep action. Considering the a11y update, that file was necessary for the users with special needs. This was the reason that CDN was changed.
  2. With the MathJax a11y, some more files are loaded by MathJax(as mentioned previously that was the cause of test failure). Even though tests' flaky behavior has been removed, if in future, such a thing happens again, simply add some wait in the tests to allow the related files to load.
  3. The failing lettuce tests have been disable in favor of their equivalent bokchoy tests.

Testing

Following Mathjax expressions' speech output has been verified:

  • Mathematical symbols (<, >, =)
  • Greek Symbols (alpha, beta, gamma etc.)
  • Summation
  • Fraction
  • Matrices

Sandbox

Reviewers

Post Review

  • Squash & Rebase commits

@@ -727,10 +727,11 @@ def get_new_post_preview_value(self, selector=".wmd-preview > *"):
self.wait_for_element_visibility(selector, "WMD preview pane has contents", timeout=10)
return self.q(css=".wmd-preview").html[0]

def get_new_post_preview_text(self):
def get_new_post_preview_text(self, selector=".wmd-preview > div"):
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for moving this to an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, the 'div' is rendered when we use block mathjax ($$ $$). For inline, the 'p' tag is rendered inside the wmd-preview. By allowing a default value, we can easily add a test case for inline mathjax, by providing a selector from the test case, if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, are you providing this selector from any test at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at the moment, but I was considering adding a new test case for the Editor, just to confirm flaky behavior is no longer there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

problem_page.verify_mathjax_rendered_in_preview()
problem_page.click_submit()
self.assertFalse(problem_page.simpleprob_is_correct())
problem_page.click_reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if self.assertTrue(problem_page.is_reset_button_present()) before clicking the reset button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

problem_page.fill_answer_numerical('R_1/R_3')
problem_page.click_submit()
self.assertFalse(problem_page.simpleprob_is_correct())
problem_page.click_reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

need an empty line at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is empty line. otherwise PEP8 test would have failed. Checked it locally as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got you!

@@ -933,3 +933,71 @@ def test_grader_type_displayed(self):
problem_page = ProblemPage(self.browser)
self.assertEqual(problem_page.problem_name, 'TEST PROBLEM')
self.assertEqual(problem_page.problem_progress_graded_value, "1 point possible (ungraded)")


class FormulaProblemTest(ProblemsTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests look clean. Good job.

@DawoudSheraz
Copy link
Contributor Author

jenkins run all

@DawoudSheraz
Copy link
Contributor Author

jenkins run all

@DawoudSheraz
Copy link
Contributor Author

jenkins run bokchoy

@DawoudSheraz DawoudSheraz force-pushed the dsheraz/mathjax_a11y_update branch 2 times, most recently from 2c3c5eb to 5b6776e Compare January 2, 2019 08:25
@DawoudSheraz
Copy link
Contributor Author

jenkins run all

1 similar comment
@DawoudSheraz
Copy link
Contributor Author

jenkins run all

@DawoudSheraz
Copy link
Contributor Author

jenkins run bokchoy

@DawoudSheraz
Copy link
Contributor Author

jenkins run all

@DawoudSheraz DawoudSheraz force-pushed the dsheraz/mathjax_a11y_update branch 2 times, most recently from 22c3f42 to db76858 Compare January 7, 2019 07:30
@DawoudSheraz
Copy link
Contributor Author

jenkins run bokchoy

Copy link
Contributor

@awaisdar001 awaisdar001 left a comment

Choose a reason for hiding this comment

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

We sat together and discussed that The PR looks good to me and there are few nits which could be addressed.

};

DiscussionUtil.typesetMathJax = function(element) {

Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this extra line.

@DawoudSheraz DawoudSheraz force-pushed the dsheraz/mathjax_a11y_update branch 2 times, most recently from 8653cd3 to 1feafd6 Compare January 16, 2019 07:01
@DawoudSheraz
Copy link
Contributor Author

jenkins run bokchoy

@Rabia23
Copy link
Contributor

Rabia23 commented Jan 16, 2019

awesome work 👍

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@DawoudSheraz DawoudSheraz merged commit 15c6cc7 into master Jan 17, 2019
@fysheets
Copy link
Contributor

🎊 thank you for your continued efforts on this one @DawoudSheraz!

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, January 18, 2019.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-secure edx-secure deleted the dsheraz/mathjax_a11y_update branch January 27, 2019 15:14
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.

None yet

6 participants