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

[bug 1171413] Fix highlighting for <code>. #2754

Merged
merged 1 commit into from
Dec 22, 2015

Conversation

Osmose
Copy link
Contributor

@Osmose Osmose commented Dec 17, 2015

The parsers for CodeMirror's syntax highlighting of <code>, <pre>, and <nowiki> now consume input until their closing tags are found, and no longer have the odd behavior around </nowiki> tags.

I'm not sure whether I should add tests for this; if I did, I'd be testing if the <div class="CodeMirror-code"> tag within the CodeMirror editor has display: none applied to it. I'm also unsure of where I'd add that test: Would it be with the in-browser tests in smoketests? Somewhere else? I'm happy to add them if I can get guidance on where to add them. :D
r?

The parsers for CodeMirror's syntax highlighting of <code>, <pre>, and
<nowiki>  now consume input until their closing tags are found, and
no longer have the odd behavior around </nowiki> tags.
@mythmon
Copy link
Contributor

mythmon commented Dec 17, 2015

Thinking about this more, I wonder if the display: none behavior is intentional, and there is a source of parser errors we should be displaying?

The tests for this would like live in something like kitsune/sumo/static/sumo/js/tests/. These could also exist in kitsune/questions/, but there aren't any there yet.

@Osmose
Copy link
Contributor Author

Osmose commented Dec 17, 2015

Thinking about this more, I wonder if the display: none behavior is intentional, and there is a source of parser errors we should be displaying?

I don't think so. I'm pretty sure this is where it's being set: https://github.com/codemirror/CodeMirror/blob/9951761dc3e690cb958b4f11f7a84092466c9c56/lib/codemirror.js#L722

Looks like it's set to none for big updates to speed up DOM and style changes. And because we're throwing an exception in the middle of highlighting, it's stuck in that state.

I'll see about adding tests in the place you mentioned, thanks!

@mythmon
Copy link
Contributor

mythmon commented Dec 17, 2015

Looks like it's set to none for big updates to speed up DOM and style changes. And because we're throwing an exception in the middle of highlighting, it's stuck in that state.

Well, damn. I was hoping it wasn't that. Oh well, that makes sense.

@Osmose
Copy link
Contributor Author

Osmose commented Dec 18, 2015

After some digging, I've hit a blocker in getting CodeMirror running in the mocha tests: It relies on document.createRange, which hasn't been implemented in jsdom yet.

At this point I'm of the opinion that a regression test for this isn't going to be worth the effort required to pull it off. Unless maybe I should write a smoketest for it?

@mythmon
Copy link
Contributor

mythmon commented Dec 18, 2015

I consider writing Selenium tests a special kind of hell. If you'd like to do it, go for it, but I'd be fine without it.

@rdalal, I don't know much about the code mirror parser stuff. Can you take a look at this?

@rehandalal
Copy link
Contributor

code looks fine to me

mythmon pushed a commit that referenced this pull request Dec 22, 2015
[bug 1171413] Fix highlighting for <code>.
@mythmon mythmon merged commit 264eda8 into mozilla:master Dec 22, 2015
@Osmose Osmose deleted the syntax-highlight-code-pre-nowiki branch January 5, 2016 00:37
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.

3 participants