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

Paste from word should work for legal list only #15982

Merged
merged 12 commits into from
Mar 12, 2024
Merged

Paste from word should work for legal list only #15982

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f536a35
Fix implementation.
illia-stv Feb 29, 2024
3d0a8d4
Fix test.
illia-stv Mar 4, 2024
3617e64
Fix.
illia-stv Mar 5, 2024
d1b1585
Merge remote-tracking branch 'origin/master' into ck/15955-PFW-should…
illia-stv Mar 5, 2024
7b4f1bd
Fix.
illia-stv Mar 5, 2024
0f720f3
Merge branch 'cc/epic/multi-level-list' into ck/15955-PFW-should-work…
illia-stv Mar 5, 2024
8b85daf
Code review fixes.
illia-stv Mar 5, 2024
66d2d1e
Code review fixes.
illia-stv Mar 6, 2024
464d684
Remove window.
illia-stv Mar 6, 2024
5dd39aa
Code review fixes.
illia-stv Mar 8, 2024
ffc6597
Code review fixes.
illia-stv Mar 12, 2024
d315c4b
Apply suggestions from code review.
niegowski Mar 12, 2024
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
31 changes: 18 additions & 13 deletions packages/ckeditor5-paste-from-office/src/filters/list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ import {
*/
export function transformListItemLikeElementsIntoLists(
documentFragment: ViewDocumentFragment,
stylesString: string
stylesString: string,
hasMultiLevelListPlugin: boolean
): void {
if ( !documentFragment.childCount ) {
return;
Expand Down Expand Up @@ -61,12 +62,12 @@ export function transformListItemLikeElementsIntoLists(
const listStyle = detectListStyle( itemLikeElement, stylesString );

if ( !currentList ) {
currentList = insertNewEmptyList( listStyle, itemLikeElement.element, writer );
currentList = insertNewEmptyList( listStyle, itemLikeElement.element, writer, hasMultiLevelListPlugin );
} else if ( itemLikeElement.indent > currentIndentation ) {
const lastListItem = currentList.getChild( currentList.childCount - 1 ) as ViewElement;
const lastListItemChild = lastListItem!.getChild( lastListItem.childCount - 1 ) as ViewElement;

currentList = insertNewEmptyList( listStyle, lastListItemChild, writer );
currentList = insertNewEmptyList( listStyle, lastListItemChild, writer, hasMultiLevelListPlugin );
currentIndentation += 1;
} else if ( itemLikeElement.indent < currentIndentation ) {
const differentIndentation = currentIndentation - itemLikeElement.indent;
Expand Down Expand Up @@ -178,9 +179,12 @@ function detectListStyle( listLikeItem: ListLikeElement, stylesString: string )
const listStyleRegexp = new RegExp( `@list l${ listLikeItem.id }:level${ listLikeItem.indent }\\s*({[^}]*)`, 'gi' );
const listStyleTypeRegex = /mso-level-number-format:([^;]{0,100});/gi;
const listStartIndexRegex = /mso-level-start-at:\s{0,100}([0-9]{0,10})\s{0,100};/gi;
const legalListTypeRegex = new RegExp( `@list l${ listLikeItem.id }:level\\d\\s*{[^{]*mso-level-text:"%\\d\\\\.`, 'gi' );
const legalStyleListRegex = new RegExp( `@list l${ listLikeItem.id }:level\\d\\s*{[^{]*mso-level-text:"%\\d\\\\.`, 'gi' );
const multiLevelNumberFormatTypeRegex = new RegExp( `@list l${ listLikeItem.id }:level\\d\\s*{[^{]*mso-level-number-format:`, 'gi' );

const legalListMatch = legalListTypeRegex.exec( stylesString );
const legalStyleListMatch = legalStyleListRegex.exec( stylesString );
const multiLevelNumberFormatMatch = multiLevelNumberFormatTypeRegex.exec( stylesString );
const islegalStyleList = legalStyleListMatch && !multiLevelNumberFormatMatch;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could also avoid checking this if the Multi-level list plugin is not loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should not cause performance issue, and also if multi level list plugin is not loaded it doesn't change the fact legal list was detected. It just give us information legal list was found and thats it. Finally our logic will check does multi level list plugin is loaded or not, so checking this 2 times to my mind is not so necessary.


const listStyleMatch = listStyleRegexp.exec( stylesString );

Expand Down Expand Up @@ -214,16 +218,16 @@ function detectListStyle( listLikeItem: ListLikeElement, stylesString: string )
}
}

if ( legalListMatch ) {
// type = 'ol'; // TODO: while working on the task: PFW should work for legal only
if ( islegalStyleList ) {
type = 'ol';
}
}

return {
type,
startIndex,
style: mapListStyleDefinition( listStyleType )
// isLegal: !!legalListMatch // TODO: while working on the task: PFW should work for legal only
style: mapListStyleDefinition( listStyleType ),
isLegalStyleList: islegalStyleList
};
}

Expand Down Expand Up @@ -324,7 +328,8 @@ function mapListStyleDefinition( value: string ) {
function insertNewEmptyList(
listStyle: ReturnType<typeof detectListStyle>,
element: ViewElement,
writer: UpcastWriter
writer: UpcastWriter,
hasMultiLevelListPlugin: boolean
) {
const parent = element.parent!;
const list = writer.createElement( listStyle.type );
Expand All @@ -342,9 +347,9 @@ function insertNewEmptyList(
writer.setAttribute( 'start', listStyle.startIndex, list );
}

// if ( listStyle.isLegal ) { // TODO: while working on the task: PFW should work for legal only
// writer.addClass( 'legal-list', list );
// }
if ( listStyle.isLegalStyleList && hasMultiLevelListPlugin ) {
writer.addClass( 'legal-list', list );
}

return list;
}
Expand Down
7 changes: 5 additions & 2 deletions packages/ckeditor5-paste-from-office/src/normalizers/mswordnormalizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ const msWordMatch2 = /xmlns:o="urn:schemas-microsoft-com/i;
export default class MSWordNormalizer implements Normalizer {
public readonly document: ViewDocument;

public readonly hasMultiLevelListPlugin: boolean;

/**
* Creates a new `MSWordNormalizer` instance.
*
* @param document View document.
*/
constructor( document: ViewDocument ) {
constructor( document: ViewDocument, hasMultiLevelListPlugintor: boolean ) {
this.document = document;
this.hasMultiLevelListPlugin = hasMultiLevelListPlugintor;
}

/**
Expand All @@ -44,7 +47,7 @@ export default class MSWordNormalizer implements Normalizer {
public execute( data: NormalizerData ): void {
const { body: documentFragment, stylesString } = data._parsedData;

transformListItemLikeElementsIntoLists( documentFragment, stylesString );
transformListItemLikeElementsIntoLists( documentFragment, stylesString, this.hasMultiLevelListPlugin );
replaceImagesSourceWithBase64( documentFragment, data.dataTransfer.getData( 'text/rtf' ) );
removeMSAttributes( documentFragment );

Expand Down
3 changes: 2 additions & 1 deletion packages/ckeditor5-paste-from-office/src/pastefromoffice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ export default class PasteFromOffice extends Plugin {
const clipboardPipeline: ClipboardPipeline = editor.plugins.get( 'ClipboardPipeline' );
const viewDocument = editor.editing.view.document;
const normalizers: Array<Normalizer> = [];
const hasMultiLevelListPlugin = this.editor.plugins.has( 'MultiLevelList' );

normalizers.push( new MSWordNormalizer( viewDocument ) );
normalizers.push( new MSWordNormalizer( viewDocument, hasMultiLevelListPlugin ) );
normalizers.push( new GoogleDocsNormalizer( viewDocument ) );
normalizers.push( new GoogleSheetsNormalizer( viewDocument ) );

Expand Down
36 changes: 36 additions & 0 deletions packages/ckeditor5-paste-from-office/tests/filters/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/html
import { stringify } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js';
import Document from '@ckeditor/ckeditor5-engine/src/view/document.js';
import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter.js';
import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils.js';

import {
transformListItemLikeElementsIntoLists,
Expand All @@ -15,6 +16,8 @@ import {
import { StylesProcessor } from '@ckeditor/ckeditor5-engine/src/view/stylesmap.js';

describe( 'PasteFromOffice - filters', () => {
testUtils.createSinonSandbox();

describe( 'list - paste from MS Word', () => {
const htmlDataProcessor = new HtmlDataProcessor( new Document( new StylesProcessor() ) );

Expand Down Expand Up @@ -104,6 +107,38 @@ describe( 'PasteFromOffice - filters', () => {
);
} );

it( 'handles "legal-list" when multi-level-list is loaded', () => {
const level1 = 'style="mso-list:l0 level1 lfo0"';
const styles = '@list l0:level1\n' +
'{mso-level-text:"%1\\.%2\\.";}';

const html = `<p ${ level1 }>Foo</p>`;
const view = htmlDataProcessor.toView( html );
const hasMultiLevelListPluginLoaded = true;

transformListItemLikeElementsIntoLists( view, styles, hasMultiLevelListPluginLoaded );

expect( stringify( view ) ).to.equal(
`<ol class="legal-list"><li ${ level1 }>Foo</li></ol>`
);
} );

it( 'handles legal-list when multi-level-list is not loaded', () => {
const level1 = 'style="mso-list:l0 level1 lfo0"';
const styles = '@list l0:level1\n' +
'{mso-level-text:"%1\\.%2\\.";}';

const html = `<p ${ level1 }>Foo</p>`;
const view = htmlDataProcessor.toView( html );
const hasMultiLevelListPluginLoaded = false;

transformListItemLikeElementsIntoLists( view, styles, hasMultiLevelListPluginLoaded );

expect( stringify( view ) ).to.equal(
`<ol><li ${ level1 }>Foo</li></ol>`
);
} );

describe( 'nesting', () => {
const level1 = 'style="mso-list:l0 level1 lfo0"';
const level2 = 'style="mso-list:l0 level2 lfo0"';
Expand Down Expand Up @@ -279,6 +314,7 @@ describe( 'PasteFromOffice - filters', () => {
`<ol style="list-style-type:lower-alpha"><li ${ level1 }>Foo</li></ol>`
);
} );

it( 'converts "roman-upper" style to proper CSS attribute', () => {
const styles = '@list l0:level1\n' +
'{mso-level-number-format:roman-upper;}';
Expand Down