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.

Sign up for GitHub

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

Run parser plugins for top-level unknown elements #627

Merged
merged 3 commits into from
Jun 21, 2018
+90 −10
Merged

Run parser plugins for top-level unknown elements #627

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be446f6
Failing tests for #494
bantic Sep 14, 2016
daf2fe0
Run parser plugins for top-level unknown elements
kevinansfield Jun 18, 2018
9de4405
Avoid running top-level text nodes through parserPlugins twice
kevinansfield Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/js/parsers/section.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,18 @@ class SectionParser {

this._updateStateFromElement(element);

let childNodes = isTextNode(element) ? [element] : element.childNodes;
let finished = this.runPlugins(element);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given an element here that is a text node, wouldn't the add call here mean that text node will be passed to this.runPlugins twice? Once here and once in pardeNode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch 😄 Pushed a fix


if (this.state.section.isListSection) {
this.parseListItems(childNodes);
} else {
forEach(childNodes, el => {
this.parseNode(el);
});
if (!finished) {
let childNodes = isTextNode(element) ? [element] : element.childNodes;

if (this.state.section.isListSection) {
this.parseListItems(childNodes);
} else {
forEach(childNodes, el => {
this.parseNode(el);
});
}
}

this._closeCurrentSection();
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/editor/editor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ test('editor parses HTML post using parser plugins', (assert) => {
editor = new Editor({html, parserPlugins: [parserPlugin]});
assert.ok(!!editor.post, 'editor loads post');

assert.deepEqual(seenTagNames, ['TEXTAREA', 'IMG']);
assert.deepEqual(seenTagNames, ['P', 'TEXTAREA', 'IMG']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this characterized as a bugfix or as a breaking change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would characterise it as a bugfix because the old behaviour meant elements that were expected to go through parser plugins were never seen. I suppose it may be a breaking change if a consumer has built parser plugins to work around the buggy behaviour, I haven't been able to think up an example of that yet 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this and considered some examples. It seems like it wouldn't break any code we looked at in our quick review, so I'm going to call this a bugfix 👍

});

test('#activeMarkups returns the markups at cursor when range is collapsed', (assert) => {
Expand Down
51 changes: 50 additions & 1 deletion tests/unit/parsers/html-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,18 @@ function parseHTML(html, options={}) {
return new HTMLParser(builder, options).parse(html);
}

module('Unit: Parser: HTMLParser');
let didParseVideo;
function videoParserPlugin(node) {
if (node.tagName === 'VIDEO') {
didParseVideo = true;
}
}

module('Unit: Parser: HTMLParser', {
beforeEach() {
didParseVideo = false;
}
});

test('style tags are ignored', (assert) => {
// This is the html you get when copying a message from Slack's desktop app
Expand All @@ -32,3 +43,41 @@ test('newlines ("\\n") are replaced with space characters', (assert) => {

assert.postIsSimilar(post, expected);
});

// see https://github.com/bustlelabs/mobiledoc-kit/issues/494
test('top-level unknown void elements are parsed', (assert) => {
let html = `<video />`;
let post = parseHTML(html, {plugins: [videoParserPlugin]});
let {post: expected} = Helpers.postAbstract.buildFromText([]);

assert.ok(didParseVideo);
assert.postIsSimilar(post, expected);
});

// see https://github.com/bustlelabs/mobiledoc-kit/issues/494
test('top-level unknown elements are parsed', (assert) => {
let html = `<video>...inner...</video>`;
let post = parseHTML(html, {plugins: [videoParserPlugin]});
let {post: expected} = Helpers.postAbstract.buildFromText(['...inner...']);

assert.ok(didParseVideo);
assert.postIsSimilar(post, expected);
});

test('nested void unknown elements are parsed', (assert) => {
let html = `<p>...<video />...</p>`;
let post = parseHTML(html, {plugins: [videoParserPlugin]});
let {post: expected} = Helpers.postAbstract.buildFromText(['......']);

assert.ok(didParseVideo);
assert.postIsSimilar(post, expected);
});

test('nested unknown elements are parsed', (assert) => {
let html = `<p>...<video>inner</video>...</p>`;
let post = parseHTML(html, {plugins: [videoParserPlugin]});
let {post: expected} = Helpers.postAbstract.buildFromText(['...inner...']);

assert.ok(didParseVideo);
assert.postIsSimilar(post, expected);
});