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

Bug 1169350 – Add the Accessibility Checker plugin to CKEditor #4989

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
2 participants
@Elchi3
Copy link
Contributor

Elchi3 commented Sep 23, 2018

@escattone
Copy link
Member

escattone left a comment

I'm sorry @Elchi3 for taking so long to review this!

  • This PR uses version 1.0.0 of the a11ychecker plugin and version 4.10.0 of the balloonpanel plugin (a dependency of a11ychecker). The latest versions are 1.1.1 and 4.11.1 (although 4.10.0 was the latest version when this PR was created). I did perform a build using the latest versions, and it built and worked successfully.
  • I re-based this on the latest master, rebuilt CKEditor using the Docker approach, and then compared the results. I saw the expected differences (see https://kuma.readthedocs.io/en/latest/ckeditor.html#committing-changes) as well as the following differences:
    • <script> vs <script type="text/javascript"> within both "kuma/static/js/libs/ckeditor/build/ckeditor/plugins/wsc/dialogs/ciframe.html" and "kuma/static/js/libs/ckeditor/build/ckeditor/plugins/wsc/dialogs/tmpFrameset.html"-- but, since <script> is the recommended approach, that's actually an improvement (perhaps a manual change?).
    • MDN Web Docs vs Mozilla Developer Network within "kuma/static/js/libs/ckeditor/build/ckeditor/samples/old/descriptionlist/descriptionlist.html", but again that's what we want (another manual fix?).
  • I ran this locally, and tested the accessibility checker on various pages, and it worked fine (including making quick fixes).
  • I wondered if we could/should add a11ychecker and balloonpanel to the list of CKEditor plugins that we pull dynamically (like we do for scayt, wsc, wordcount, and descriptionlist), but they don't appear to be the kind of plugins that can be pulled in that fashion. It appears one must manually download them from https://ckeditor.com/cke4/addon/a11ychecker and https://ckeditor.com/cke4/addon/balloonpanel.

This looks good to me! Thanks so much @Elchi3! Feel free to merge as is (and we can perhaps update the plugins in a separate PR later if we think it's worth the effort), or if you prefer, download the latest versions of the a11ychecker and balloonpanel plugins, rebuild, and I'll fast-track an approval.

@Elchi3

This comment has been minimized.

Copy link
Contributor Author

Elchi3 commented Dec 13, 2018

I'm sorry @Elchi3 for taking so long to review this!

No worries, this was just a hackathon poject.

or if you prefer, download the latest versions of the a11ychecker and balloonpanel plugins, rebuild, and I'll fast-track an approval.

Unfortunately, I think, this rebuilding and everything is exactly the painful part here. So I'd say merge this and maybe update dependencies later (which likely means never).

@escattone escattone merged commit 457e1bf into mozilla:master Dec 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No manifest changes detected

@Elchi3 Elchi3 deleted the Elchi3:a11y-ckeditor branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment