Run parser plugins for top-level unknown elements #627
Conversation
closes bustle#494 - run parser plugins for the top-level section elements - do not continue parsing children elements if a parser marks the top-level element as finished - update tests to double-check the output for top-level elements is unchanged
…raphs refs TryGhost/Ghost#9623 - switch to custom `mobiledoc-kit` build - fixes top-level elements not being run through parser plugins (bustle/mobiledoc-kit#627) - fixes <kbd>Alt</kbd> getting stuck and causing <kbd>Backspace</kbd> to delete whole words (bustle/mobiledoc-kit#626) - fixes error that can occur when a paste results in blank insert (bustle/mobiledoc-kit#620) - add new `figureToImageCard` parser - replaces hacky workaround to detect an image+figcaption inside the `imgToCard` parser plugin - remove wrapping of html in a `<div>...</div>` when pasting - no longer necessary now that top-level elements are parsed - fixes rich-text pastes where multiple paragraphs would be collapsed into a single paragraph
There was a problem hiding this comment.
@kevinansfield thanks for diving in here, I left a few thoughts.
src/js/parsers/section.js
Outdated
| @@ -69,14 +69,18 @@ class SectionParser { | |||
|
|
|||
| this._updateStateFromElement(element); | |||
|
|
|||
| let childNodes = isTextNode(element) ? [element] : element.childNodes; | |||
| let finished = this.runPlugins(element); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch 😄 Pushed a fix
| @@ -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']); | |||
There was a problem hiding this comment.
Is this characterized as a bugfix or as a breaking change?
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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 👍
| @@ -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']); | |||
There was a problem hiding this comment.
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 👍
|
Looking forward to trying We found that when our users pasted in I've worked around this by using computed properties (eg toggle |
closes #494
@bantic I'd be interested in a quick review and any pointers for any edge cases/tests you think may need adding 😄