Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/130: Editor crashes in some cases. #132

Merged
merged 4 commits into from
Oct 2, 2018
Merged

T/130: Editor crashes in some cases. #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0337c7e
Use view changes in table cell post-fixer.
jodator Sep 27, 2018
82395e7
Other: Add missing docs. Restore needed method for updating view sele…
jodator Oct 2, 2018
c433d2b
Merge branch 'master' into t/130
jodator Oct 2, 2018
2fbcc91
Merge branch 'master' into t/130
jodator Oct 2, 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
143 changes: 75 additions & 68 deletions src/converters/tablecell-post-fixer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,102 +71,109 @@
* @param {module:engine/controller/editingcontroller~EditingController} editing
*/
export default function injectTableCellPostFixer( model, editing ) {
editing.view.document.registerPostFixer( writer => tableCellPostFixer( writer, model, editing.mapper ) );
editing.view.document.registerPostFixer( writer => tableCellPostFixer( writer, model, editing.mapper, editing.view ) );
}

// The table cell post-fixer.
//
// @param {module:engine/view/writer~Writer} writer
// @param {module:engine/model/model~Model} model
// @param {module:engine/conversion/mapper~Mapper} mapper
function tableCellPostFixer( writer, model, mapper ) {
const changes = model.document.differ.getChanges();
function tableCellPostFixer( writer, model, mapper, view ) {
let wasFixed = false;

// While this is view post fixer only nodes that changed are worth investigating.
for ( const entry of changes ) {
// Attribute change - check if it is single paragraph inside table cell that has attributes changed.
if ( entry.type == 'attribute' && entry.range.start.parent.name == 'tableCell' ) {
const tableCell = entry.range.start.parent;

if ( tableCell.childCount === 1 ) {
const singleChild = tableCell.getChild( 0 );
const renameTo = Array.from( singleChild.getAttributes() ).length ? 'p' : 'span';

wasFixed = renameParagraphIfDifferent( singleChild, renameTo, writer, model, mapper ) || wasFixed;
}
} else {
// Check all nodes inside table cell on insert/remove operations (also other blocks).
const parent = entry.position && entry.position.parent;

if ( parent && parent.is( 'tableCell' ) ) {
const renameTo = parent.childCount > 1 ? 'p' : 'span';

for ( const child of parent.getChildren() ) {
wasFixed = renameParagraphIfDifferent( child, renameTo, writer, model, mapper ) || wasFixed;
}
}
}
const elementsToCheck = getElementsToCheck( view );

for ( const element of elementsToCheck ) {
wasFixed = ensureProperElementName( element, mapper, writer ) || wasFixed;
}

// Selection in the view might not be updated to renamed elements. Happens mostly when other feature inserts paragraph to the table cell
// (ie. when deleting table cell contents) and sets selection to it while table-post fixer changes view <p> to <span> element.
// The view.selection would have outdated nodes.
if ( wasFixed ) {
updateRangesInViewSelection( model.document.selection, mapper, writer );
}

return wasFixed;
}

// Renames associated view element to a desired one. It will only rename if:
// - model element is a paragraph
// - view element is converted (mapped)
// - view element has different name then requested.
// Returns view elements changed in current view.change() block.
//
// @param {module:engine/model/element~Element} modelElement
// @param {String} desiredElementName
// @param {module:engine/view/writer~Writer} writer
// @param {module:engine/model/model~Model} model
// @param {module:engine/conversion/mapper~Mapper} mapper
function renameParagraphIfDifferent( modelElement, desiredElementName, writer, model, mapper ) {
// Only rename paragraph elements.
if ( !modelElement.is( 'paragraph' ) ) {
return false;
}
// **Note**: Currently it uses private property of the view: _renderer to get changed view elements to check.
//
// @param {module:engine/view/view~View} view
function getElementsToCheck( view ) {
const elementsWithChangedAttributes = Array.from( view._renderer.markedAttributes )
.filter( el => !!el.parent )
.filter( isSpanOrP )
.filter( el => isTdOrTh( el.parent ) );

const changedChildren = Array.from( view._renderer.markedChildren )
.filter( el => !!el.parent )
.filter( isTdOrTh )
.reduce( ( prev, element ) => {
const childrenToCheck = Array.from( element.getChildren() ).filter( isSpanOrP );

return [ ...prev, ...childrenToCheck ];
}, [] );

return [ ...elementsWithChangedAttributes, ...changedChildren ];
}

const viewElement = mapper.toViewElement( modelElement );
// This method checks if view element for model's <paragraph> was properly converter.
// Paragraph should be either
// - span: for single paragraph with no attributes.
// - p : in other cases.
function ensureProperElementName( currentViewElement, mapper, writer ) {
const modelParagraph = mapper.toModelElement( currentViewElement );
const expectedViewElementName = getExpectedElementName( modelParagraph.parent, modelParagraph );

// Only rename converted elements which aren't desired ones.
if ( !viewElement || viewElement.name === desiredElementName ) {
return false;
}
if ( currentViewElement.name !== expectedViewElementName ) {
// Unbind current view element as it should be cleared from mapper.
mapper.unbindViewElement( currentViewElement );

// After renaming element in the view by a post-fixer the selection would have references to the previous element.
const selection = model.document.selection;
const shouldFixSelection = checkSelectionForRenamedElement( selection, modelElement );
const renamedViewElement = writer.rename( expectedViewElementName, currentViewElement );

// Unbind current view element as it should be cleared from mapper.
mapper.unbindViewElement( viewElement );
const renamedViewElement = writer.rename( desiredElementName, viewElement );
// Bind paragraph inside table cell to the renamed view element.
mapper.bindElements( modelElement, renamedViewElement );
// Bind paragraph inside table cell to the renamed view element.
mapper.bindElements( modelParagraph, renamedViewElement );

if ( shouldFixSelection ) {
// Re-create view selection based on model selection.
updateRangesInViewSelection( selection, mapper, writer );
return true;
}

return true;
return false;
}

// Checks if model selection contains renamed element.
// Expected view element name depends on model elements:
// - <paragraph> with any attribute set should be rendered as <p>
// - all <paragraphs> in <tableCell> that has more then one children should be rendered as <p>
// - an only <paragraph> child with no attributes should be rendered as <span>
//
// @param {module:engine/model/selection~Selection} selection
// @param {module:engine/model/element~Element} modelElement
// @returns {boolean}
function checkSelectionForRenamedElement( selection, modelElement ) {
return !![ ...selection.getSelectedBlocks() ].find( block => block === modelElement );
// @param {module:engine/model/element~Element} tableCell
// @param {module:engine/model/element~Element} paragraph
// @returns {String}
function getExpectedElementName( tableCell, paragraph ) {
const isOnlyChild = tableCell.childCount > 1;
const hasAttributes = !![ ...paragraph.getAttributes() ].length;

return ( isOnlyChild || hasAttributes ) ? 'p' : 'span';
}

// Re-create view selection from model selection.
// Method to filter out <span> and <p> elements.
//
// @param {module:engine/model/selection~Selection} selection
// @param {module:engine/view/writer~Writer} writer
// @param {module:engine/conversion/mapper~Mapper} mapper
// @param {module:engine/view/element~Element} element
function isSpanOrP( element ) {
return element.is( 'p' ) || element.is( 'span' );
}

// Method to filter out <td> and <th> elements.
//
// @param {module:engine/view/element~Element} element
function isTdOrTh( element ) {
return element.is( 'td' ) || element.is( 'th' );
}

// Resets view selections based on model selection.
function updateRangesInViewSelection( selection, mapper, writer ) {
const fixedRanges = Array.from( selection.getRanges() )
.map( range => mapper.toViewRange( range ) );
Expand Down
4 changes: 3 additions & 1 deletion tests/_utils/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
} from '../../src/converters/downcast';
import upcastTable, { upcastTableCell } from '../../src/converters/upcasttable';

const WIDGET_TABLE_CELL_CLASS = 'ck-editor__editable ck-editor__nested-editable';

/**
* Returns a model representation of a table shorthand notation:
*
Expand Down Expand Up @@ -264,7 +266,7 @@ function makeRows( tableData, options ) {
const attributes = isObject ? tableCellData : {};

if ( asWidget ) {
attributes.class = 'ck-editor__editable ck-editor__nested-editable';
attributes.class = WIDGET_TABLE_CELL_CLASS + ( attributes.class ? ` ${ attributes.class }` : '' );
attributes.contenteditable = 'true';
}

Expand Down
Loading