#7753: Upcast conversion should handle wrapping in paragraphs if it would allow conversion #7787
Conversation
| * @param {module:engine/model/writer~Writer} writer The model writer. | ||
| * @returns {Boolean} `true` if any change has been applied, `false` otherwise. | ||
| */ | ||
| _autoparagraphEmptyRoots( writer ) { |
There was a problem hiding this comment.
This is not using this AFAICS so maybe we could move it to a helper function outside model class.
I'm not entirely sure if this belongs to model, but seeing this called in DataController as this.model._autoparagraphEmptyRoots() gives me strange feeling.
AFAICS we have model/utils with some default post-fixers.
ps.: As we cleanup this I think that autoparagraphEmptyRoots should be autoParagraphEmptyRoots.
| // Check if the node wrapped with a paragraph would be accepted by the schema. | ||
| const paragraph = wrapWithParagraphIfPossible( node, modelCursor, writer, schema ); | ||
|
|
||
| return paragraph && { |
There was a problem hiding this comment.
Oh no :D Please don do't do that ;) Conditional return shouldn't be used. For me the only acceptable way for using && is in isFooBar() checks.
Maybe split logic of wrapWith... to ifPossible() (return null) and wrapWithParagraph + return {} would make it clearer?
|
|
||
| conversionApi.writer.insert( text, data.modelCursor ); | ||
| // When node is already converted then do nothing. | ||
| if ( data.modelRange || !consumable.test( data.viewItem ) ) { |
There was a problem hiding this comment.
We should be able to tell that we can convert anything using consumable.test() so checking for data.modelRange is unexpected here. What case this covers? Maybe we need to check for a fix for that elsewhere.
There was a problem hiding this comment.
This was handled this way (only data.modelRange) in the original code for this. I was also surprised so I added consumable.test there. It's also strange that CC is 100%
There was a problem hiding this comment.
Seems that CC can't notice that it's redundant. Updated it.
|
|
||
| dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => { | ||
| const section = conversionApi.writer.createElement( 'section' ); | ||
| const position = ModelPosition._createAt( section, 0 ); |
There was a problem hiding this comment.
could you use conversionApi.writer to create postions? Using writer or model is preferable then importing ModelPosition.
We left that in tests and in engine source code but we might use proper API in the test.
| @@ -969,6 +969,31 @@ describe( 'upcast-converters', () => { | |||
| expect( conversionResult.getChild( 0 ).data ).to.equal( 'foobar' ); | |||
| } ); | |||
|
|
|||
| it( 'should wrap text with paragraph if it would allow conversion', () => { | |||
There was a problem hiding this comment.
Maybe 'should auto-paragraph text if it is not allowed at insert postion but is inserted where paragraph is allowed'?
…raph option worked properly for empty root.
|
@niegowski and @jodator thank you for taking the time to do this and fix #6698, greatly appreciated ! |
Suggested merge commit message (convention)
Fix (engine): Upcast conversion should handle wrapping in paragraphs if it would allow conversion. Closes #7753. Closes #6698.
Internal (paragraph): Auto-paragraphing content conversion moved to the engine package.
Tests (table): Added tests for upcasting inline elements inside a table cell.
Additional information