Skip to content

Commit

Permalink
Merge pull request #14307 from ckeditor/ck/14216-remove-htmlXAttribut…
Browse files Browse the repository at this point in the history
…es-from-unassociated-elements

Fix (html-support): Remove unrelated `html*Attributes` from elements. See #14216.
  • Loading branch information
niegowski committed Jun 1, 2023
2 parents 316d426 + 1d6196b commit 80daee5
Show file tree
Hide file tree
Showing 14 changed files with 231 additions and 52 deletions.
4 changes: 2 additions & 2 deletions packages/ckeditor5-engine/src/conversion/upcasthelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -790,8 +790,8 @@ function upcastDataToMarker( config: {
//
// This hack probably would not be needed if attributes are upcasted separately.
//
const basePriority = priorities.get( 'low' );
const maxPriority = priorities.get( 'highest' );
const basePriority = priorities.low;
const maxPriority = priorities.highest;
const priorityFactor = priorities.get( config.converterPriority ) / maxPriority; // Number in range [ -1, 1 ].

dispatcher.on<UpcastElementEvent>(
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-heading/src/headingediting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export default class HeadingEditing extends Plugin {
view: 'h1',
// With a `low` priority, `paragraph` plugin autoparagraphing mechanism is executed. Make sure
// this listener is called before it. If not, `h1` will be transformed into a paragraph.
converterPriority: priorities.get( 'low' ) + 1
converterPriority: priorities.low + 1
} );
}
}
72 changes: 66 additions & 6 deletions packages/ckeditor5-html-support/src/datafilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ export default class DataFilter extends Plugin {

this._registerElementsAfterInit();
this._registerElementHandlers();
this._registerModelPostFixer();
this._registerCoupledAttributesPostFixer();
this._registerAssociatedHtmlAttributesPostFixer();
}

/**
Expand Down Expand Up @@ -322,7 +323,7 @@ export default class DataFilter extends Plugin {
}, {
// With the highest priority listener we are able to register elements right before
// running data conversion.
priority: priorities.get( 'highest' ) + 1
priority: priorities.highest + 1
} );
}
}
Expand All @@ -346,7 +347,7 @@ export default class DataFilter extends Plugin {
// * Make sure no other features hook into this event before GHS because otherwise the
// downcast conversion (for these features) could run before GHS registered its converters
// (https://github.com/ckeditor/ckeditor5/issues/11356).
priority: priorities.get( 'highest' ) + 1
priority: priorities.highest + 1
} );
}

Expand Down Expand Up @@ -410,7 +411,7 @@ export default class DataFilter extends Plugin {
* The `htmlA` attribute would stay in the model and would cause GHS to generate an `<a>` element.
* This is incorrect from UX point of view, as the user wanted to remove the whole link (not only `href`).
*/
private _registerModelPostFixer() {
private _registerCoupledAttributesPostFixer() {
const model = this.editor.model;

model.document.registerPostFixer( writer => {
Expand Down Expand Up @@ -447,6 +448,63 @@ export default class DataFilter extends Plugin {
} );
}

/**
* Removes `html*Attributes` attributes from incompatible elements.
*
* For example, consider the following HTML:
*
* ```html
* <heading2 htmlH2Attributes="...">foobar[]</heading2>
* ```
*
* Pressing `enter` creates a new `paragraph` element that inherits
* the `htmlH2Attributes` attribute from `heading2`.
*
* ```html
* <heading2 htmlH2Attributes="...">foobar</heading2>
* <paragraph htmlH2Attributes="...">[]</paragraph>
* ```
*
* This postfixer ensures that this doesn't happen, and that elements can
* only have `html*Attributes` associated with them,
* e.g.: `htmlPAttributes` for `<p>`, `htmlDivAttributes` for `<div>`, etc.
*
* With it enabled, pressing `enter` at the end of `<heading2>` will create
* a new paragraph without the `htmlH2Attributes` attribute.
*
* ```html
* <heading2 htmlH2Attributes="...">foobar</heading2>
* <paragraph>[]</paragraph>
* ```
*/
private _registerAssociatedHtmlAttributesPostFixer() {
const model = this.editor.model;

model.document.registerPostFixer( writer => {
const changes = model.document.differ.getChanges();
let changed = false;

for ( const change of changes ) {
if ( change.type !== 'insert' || change.name === '$text' ) {
continue;
}

for ( const attr of change.attributes.keys() ) {
if ( !attr.startsWith( 'html' ) || !attr.endsWith( 'Attributes' ) ) {
continue;
}

if ( !model.schema.checkAttribute( change.name, attr ) ) {
writer.removeAttribute( attr, change.position.nodeAfter! );
changed = true;
}
}
}

return changed;
} );
}

/**
* Collects the map of coupled attributes. The returned map is keyed by the feature attribute name
* and coupled GHS attribute names are stored in the value array.
Expand Down Expand Up @@ -515,7 +573,8 @@ export default class DataFilter extends Plugin {
model: viewToModelObjectConverter( definition ),
// With a `low` priority, `paragraph` plugin auto-paragraphing mechanism is executed. Make sure
// this listener is called before it. If not, some elements will be transformed into a paragraph.
converterPriority: priorities.get( 'low' ) + 1
// `+ 2` is used to take priority over `_addDefaultH1Conversion` in the Heading plugin.
converterPriority: priorities.low + 2
} );
conversion.for( 'upcast' ).add( viewToModelBlockAttributeConverter( definition as DataSchemaBlockElementDefinition, this ) );

Expand Down Expand Up @@ -557,7 +616,8 @@ export default class DataFilter extends Plugin {
view: viewName,
// With a `low` priority, `paragraph` plugin auto-paragraphing mechanism is executed. Make sure
// this listener is called before it. If not, some elements will be transformed into a paragraph.
converterPriority: priorities.get( 'low' ) + 1
// `+ 2` is used to take priority over `_addDefaultH1Conversion` in the Heading plugin.
converterPriority: priorities.low + 2
} );

conversion.for( 'downcast' ).elementToElement( {
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-html-support/src/dataschema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module html-support/dataschema
*/

import { Plugin, type Editor } from 'ckeditor5/src/core';
import { Plugin } from 'ckeditor5/src/core';
import { toArray } from 'ckeditor5/src/utils';
import defaultConfig from './schemadefinitions';
import { mergeWith } from 'lodash-es';
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-html-support/src/htmlcomment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module html-support/htmlcomment
*/

import type { Marker, Position, Range, Element, ViewRootEditableElement } from 'ckeditor5/src/engine';
import type { Marker, Position, Range, Element } from 'ckeditor5/src/engine';
import { Plugin } from 'ckeditor5/src/core';
import { uid } from 'ckeditor5/src/utils';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ export default class DualContentModelElementSupport extends Plugin {
},
// With a `low` priority, `paragraph` plugin auto-paragraphing mechanism is executed. Make sure
// this listener is called before it. If not, some elements will be transformed into a paragraph.
converterPriority: priorities.get( 'low' ) + 0.5
converterPriority: priorities.low + 0.5
} );

conversion.for( 'downcast' ).elementToElement( {
Expand Down
32 changes: 1 addition & 31 deletions packages/ckeditor5-html-support/src/integrations/heading.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,10 @@
*/

import { Plugin, type Editor } from 'ckeditor5/src/core';
import type { Item } from 'ckeditor5/src/engine';
import type { HeadingOption } from '@ckeditor/ckeditor5-heading';
import {
Enter,
type EnterCommand,
type EnterCommandAfterExecuteEvent
} from 'ckeditor5/src/enter';
import { Enter } from 'ckeditor5/src/enter';

import DataSchema from '../dataschema';
import { getHtmlAttributeName, modifyGhsAttribute } from '../utils';
import type { HeadingElementOption } from '@ckeditor/ckeditor5-heading/src/headingconfig';

/**
* Provides the General HTML Support integration with {@link module:heading/heading~Heading Heading} feature.
Expand Down Expand Up @@ -51,7 +44,6 @@ export default class HeadingElementSupport extends Plugin {
const options: Array<HeadingOption> = editor.config.get( 'heading.options' )!;

this.registerHeadingElements( editor, options );
this.removeClassesOnEnter( editor, options );
}

/**
Expand Down Expand Up @@ -79,26 +71,4 @@ export default class HeadingElementSupport extends Plugin {
}
} );
}

/**
* Removes css classes from "html*Attributes" of new paragraph created when hitting "enter" in heading.
*/
private removeClassesOnEnter( editor: Editor, options: Array<HeadingOption> ): void {
const enterCommand: EnterCommand = editor.commands.get( 'enter' )!;

this.listenTo<EnterCommandAfterExecuteEvent>( enterCommand, 'afterExecute', ( evt, data ) => {
const positionParent = editor.model.document.selection.getFirstPosition()!.parent;
const heading = options.find( ( option ): option is HeadingElementOption => positionParent.is( 'element', option.model ) );

if ( heading && positionParent.childCount === 0 ) {
modifyGhsAttribute(
data.writer,
positionParent as Item,
getHtmlAttributeName( heading.view as string ),
'classes',
classes => classes.clear()
);
}
} );
}
}
5 changes: 3 additions & 2 deletions packages/ckeditor5-html-support/src/integrations/image.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ import type {
DowncastDispatcher,
Element,
UpcastDispatcher,
ViewElement } from 'ckeditor5/src/engine';
ViewElement
} from 'ckeditor5/src/engine';

import DataFilter, { type DataFilterRegisterEvent } from '../datafilter';
import { type GHSViewAttributes, setViewAttributes, updateViewAttributes, getHtmlAttributeName } from '../utils';
import { type GHSViewAttributes, setViewAttributes, updateViewAttributes } from '../utils';
import { getDescendantElement } from './integrationutils';

/**
Expand Down
1 change: 0 additions & 1 deletion packages/ckeditor5-html-support/src/integrations/script.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
} from '../converters';
import DataFilter, { type DataFilterRegisterEvent } from '../datafilter';
import type { DataSchemaBlockElementDefinition } from '../dataschema';
import { getHtmlAttributeName } from '../utils';

/**
* Provides the General HTML Support for `script` elements.
Expand Down
1 change: 0 additions & 1 deletion packages/ckeditor5-html-support/src/integrations/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
} from '../converters';
import DataFilter, { type DataFilterRegisterEvent } from '../datafilter';
import type { DataSchemaBlockElementDefinition } from '../dataschema';
import { getHtmlAttributeName } from '../utils';

/**
* Provides the General HTML Support for `style` elements.
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-html-support/src/integrations/table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type {
import { Plugin } from 'ckeditor5/src/core';
import type { TableUtils } from '@ckeditor/ckeditor5-table';

import { updateViewAttributes, type GHSViewAttributes, getHtmlAttributeName } from '../utils';
import { updateViewAttributes, type GHSViewAttributes } from '../utils';
import DataFilter, { type DataFilterRegisterEvent } from '../datafilter';
import { getDescendantElement } from './integrationutils';

Expand Down
9 changes: 6 additions & 3 deletions packages/ckeditor5-html-support/src/schemadefinitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,17 +520,20 @@ export default {
{
model: 'htmlLiAttributes',
view: 'li',
appliesToBlock: true
appliesToBlock: true,
coupledAttribute: 'listItemId'
},
{
model: 'htmlListAttributes',
view: 'ol',
appliesToBlock: true
appliesToBlock: true,
coupledAttribute: 'listItemId'
},
{
model: 'htmlListAttributes',
view: 'ul',
appliesToBlock: true
appliesToBlock: true,
coupledAttribute: 'listItemId'
},
{
model: 'htmlFigureAttributes',
Expand Down

0 comments on commit 80daee5

Please sign in to comment.