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.

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

Table properties should be enabled if table is selected from the outside #15150

Merged
merged 1 commit into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import { Command } from 'ckeditor5/src/core';
import type { Writer } from 'ckeditor5/src/engine';
import type TableCaptionEditing from './tablecaptionediting';

import { getCaptionFromTableModelElement, getSelectionAffectedTable } from './utils';
import { getCaptionFromTableModelElement } from './utils';
import { getSelectionAffectedTable } from '../utils/common';

/**
* The toggle table caption command.
Expand Down
16 changes: 2 additions & 14 deletions packages/ckeditor5-table/src/tablecaption/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import type {
ViewElement
} from 'ckeditor5/src/engine';

import { getSelectionAffectedTable } from '../utils/common';

/**
* Checks if the provided model element is a `table`.
*
Expand Down Expand Up @@ -75,17 +77,3 @@ export function matchTableCaptionViewElement( element: ViewElement ): { name: tr

return null;
}

/**
* Depending on the position of the selection we either return the table under cursor or look for the table higher in the hierarchy.
*/
export function getSelectionAffectedTable( selection: DocumentSelection ): Element {
const selectedElement = selection.getSelectedElement();

// Is the command triggered from the `tableToolbar`?
if ( selectedElement && selectedElement.is( 'element', 'table' ) ) {
return selectedElement;
}

return selection.getFirstPosition()!.findAncestor( 'table' )!;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import type { Batch, Element } from 'ckeditor5/src/engine';
import { Command, type Editor } from 'ckeditor5/src/core';
import { getSelectionAffectedTable } from '../../utils/common';

export interface TablePropertyCommandExecuteOptions {
batch?: Batch;
Expand Down Expand Up @@ -55,7 +56,7 @@ export default class TablePropertyCommand extends Command {
const editor = this.editor;
const selection = editor.model.document.selection;

const table = selection.getFirstPosition()!.findAncestor( 'table' )!;
const table = getSelectionAffectedTable( selection );

this.isEnabled = !!table;
this.value = this._getValue( table );
Expand All @@ -76,7 +77,7 @@ export default class TablePropertyCommand extends Command {

const { value, batch } = options;

const table = selection.getFirstPosition()!.findAncestor( 'table' )!;
const table = getSelectionAffectedTable( selection );
const valueToSet = this._getValueToSet( value );

model.enqueueChange( batch, writer => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
lineWidthFieldValidator,
defaultColors
} from '../utils/ui/table-properties';
import { getTableWidgetAncestor } from '../utils/ui/widget';
import { getSelectionAffectedTableWidget } from '../utils/ui/widget';
import { getBalloonTablePositionData, repositionContextualBalloon } from '../utils/ui/contextualballoon';
import { getNormalizedDefaultProperties, type NormalizedDefaultProperties } from '../utils/table-properties';
import type { Batch } from 'ckeditor5/src/engine';
Expand Down Expand Up @@ -357,7 +357,7 @@ export default class TablePropertiesUI extends Plugin {
const editor = this.editor;
const viewDocument = editor.editing.view.document;

if ( !getTableWidgetAncestor( viewDocument.selection ) ) {
if ( !getSelectionAffectedTableWidget( viewDocument.selection ) ) {
this._hideView();
} else if ( this._isViewVisible ) {
repositionContextualBalloon( editor, 'table' );
Expand Down
17 changes: 16 additions & 1 deletion packages/ckeditor5-table/src/utils/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import type {
Item,
Position,
Schema,
Writer
Writer,
DocumentSelection
} from 'ckeditor5/src/engine';

import { downcastAttributeToStyle, upcastStyleToAttribute } from './../converters/tableproperties';
Expand Down Expand Up @@ -87,3 +88,17 @@ export function enableProperty(
upcastStyleToAttribute( conversion, { viewElement: /^(td|th)$/, ...options } );
downcastAttributeToStyle( conversion, { modelElement: 'tableCell', ...options } );
}

/**
* Depending on the position of the selection we either return the table under cursor or look for the table higher in the hierarchy.
*/
export function getSelectionAffectedTable( selection: DocumentSelection ): Element {
const selectedElement = selection.getSelectedElement();

// Is the command triggered from the `tableToolbar`?
if ( selectedElement && selectedElement.is( 'element', 'table' ) ) {
return selectedElement;
}

return selection.getFirstPosition()!.findAncestor( 'table' )!;
}
24 changes: 14 additions & 10 deletions packages/ckeditor5-table/src/utils/ui/contextualballoon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@

import { Rect, type PositionOptions } from 'ckeditor5/src/utils';
import { BalloonPanelView, type ContextualBalloon } from 'ckeditor5/src/ui';

import { getTableWidgetAncestor } from './widget';
import type { Editor } from 'ckeditor5/src/core';
import type { Element, Position, Range } from 'ckeditor5/src/engine';

import { getSelectionAffectedTableWidget, getTableWidgetAncestor } from './widget';
import { getSelectionAffectedTable } from '../common';

const DEFAULT_BALLOON_POSITIONS = BalloonPanelView.defaultPositions;

const BALLOON_POSITIONS = [
Expand All @@ -36,16 +37,19 @@ const BALLOON_POSITIONS = [
*/
export function repositionContextualBalloon( editor: Editor, target: string ): void {
const balloon: ContextualBalloon = editor.plugins.get( 'ContextualBalloon' );
const selection = editor.editing.view.document.selection;
let position;

if ( getTableWidgetAncestor( editor.editing.view.document.selection ) ) {
let position;

if ( target === 'cell' ) {
if ( target === 'cell' ) {
if ( getTableWidgetAncestor( selection ) ) {
position = getBalloonCellPositionData( editor );
} else {
position = getBalloonTablePositionData( editor );
}
}
else if ( getSelectionAffectedTableWidget( selection ) ) {
position = getBalloonTablePositionData( editor );
}

if ( position ) {
balloon.updatePosition( position );
}
}
Expand All @@ -58,8 +62,8 @@ export function repositionContextualBalloon( editor: Editor, target: string ): v
* @param editor The editor instance.
*/
export function getBalloonTablePositionData( editor: Editor ): Partial<PositionOptions> {
const firstPosition = editor.model.document.selection.getFirstPosition()!;
const modelTable = firstPosition.findAncestor( 'table' )!;
const selection = editor.model.document.selection;
const modelTable = getSelectionAffectedTable( selection );
const viewTable = editor.editing.mapper.toViewElement( modelTable )!;

return {
Expand Down
13 changes: 13 additions & 0 deletions packages/ckeditor5-table/src/utils/ui/widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,19 @@ import type { ViewDocumentFragment, ViewDocumentSelection, ViewElement, ViewNode

import { isWidget } from 'ckeditor5/src/widget';

/**
* Depending on the position of the selection either return the selected table or the table higher in the hierarchy.
*/
export function getSelectionAffectedTableWidget( selection: ViewDocumentSelection ): ViewElement | null {
const selectedTable = getSelectedTableWidget( selection );

if ( selectedTable ) {
return selectedTable;
}

return getTableWidgetAncestor( selection );
}

/**
* Returns a table widget editing view element if one is selected.
*/
Expand Down
21 changes: 0 additions & 21 deletions packages/ckeditor5-table/tests/tablecaption/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Selection from '@ckeditor/ckeditor5-engine/src/model/selection';
import View from '@ckeditor/ckeditor5-engine/src/view/view';
import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
Expand All @@ -15,7 +14,6 @@ import TableEditing from '../../src/tableediting';
import {
getCaptionFromModelSelection,
getCaptionFromTableModelElement,
getSelectionAffectedTable,
isTable,
matchTableCaptionViewElement
} from '../../src/tablecaption/utils';
Expand Down Expand Up @@ -212,23 +210,4 @@ describe( 'table caption utils', () => {
} );
} );
} );

describe( 'getSelectionAffectedTable', () => {
it( 'should return null if table is not present', () => {
setModelData( model, '<paragraph>Foo[]</paragraph>' );
const selection = new Selection( model.createPositionFromPath( modelRoot, [ 0 ] ) );

const tableElement = getSelectionAffectedTable( selection );

expect( tableElement ).to.be.null;
} );

it( 'should return table if present higher in the model tree', () => {
const selection = new Selection( model.createPositionFromPath( modelRoot, [ 0, 0, 0 ] ) );

const tableElement = getSelectionAffectedTable( selection );

expect( tableElement ).to.equal( modelRoot.getNodeByPath( [ 0 ] ) );
} );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,15 @@ describe( 'table properties', () => {
expect( command.isEnabled ).to.be.false;
} );

it( 'should be true is selection has table', () => {
it( 'should be true if selection is in a table', () => {
setData( model, modelTable( [ [ 'f[o]o' ] ] ) );
expect( command.isEnabled ).to.be.true;
} );

it( 'should be true if table is selected', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ] ) + ']' );
expect( command.isEnabled ).to.be.true;
} );
} );
} );

Expand All @@ -78,23 +83,35 @@ describe( 'table properties', () => {
} );

describe( 'non-collapsed selection', () => {
it( 'should be false if selection does not have table', () => {
it( 'should be undefined if selection does not have table', () => {
setData( model, '<paragraph>f[oo]</paragraph>' );

expect( command.value ).to.be.undefined;
} );

it( 'should be undefined if selected table has set the default value', () => {
it( 'should be undefined if selected table has set the default value (selection inside table)', () => {
setData( model, modelTable( [ [ 'f[o]o' ] ], { tableAlignment: 'center' } ) );

expect( command.value ).to.be.undefined;
} );

it( 'should be true is selection has table', () => {
it( 'should be set if selection has table (selection inside table)', () => {
setData( model, modelTable( [ [ 'f[o]o' ] ], { tableAlignment: 'left' } ) );

expect( command.value ).to.equal( 'left' );
} );

it( 'should be undefined if selected table has set the default value (selected table)', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ], { tableAlignment: 'center' } ) + ']' );

expect( command.value ).to.be.undefined;
} );

it( 'should be set if selection has table (selected table)', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ], { tableAlignment: 'left' } ) + ']' );

expect( command.value ).to.equal( 'left' );
} );
} );
} );

Expand Down Expand Up @@ -142,7 +159,7 @@ describe( 'table properties', () => {
} );
} );

describe( 'non-collapsed selection', () => {
describe( 'non-collapsed selection (inside table)', () => {
it( 'should set selected table alignment to a passed value', () => {
setData( model, modelTable( [ [ '[foo]' ] ] ) );

Expand Down Expand Up @@ -175,6 +192,40 @@ describe( 'table properties', () => {
assertTableStyle( editor, '' );
} );
} );

describe( 'non-collapsed selection (outside table)', () => {
it( 'should set selected table alignment to a passed value', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ] ) + ']' );

command.execute( { value: 'right' } );

assertTableStyle( editor, null, 'float:right;' );
} );

it( 'should change selected table alignment to a passed value', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ] ) + ']' );

command.execute( { value: 'right' } );

assertTableStyle( editor, null, 'float:right;' );
} );

it( 'should remove alignment from a selected table if no value is passed', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ] ) + ']' );

command.execute();

assertTableStyle( editor, '' );
} );

it( 'should not set alignment in a selected table if passed the default value', () => {
setData( model, '[' + modelTable( [ [ 'foo' ] ] ) + ']' );

command.execute( { value: 'center' } );

assertTableStyle( editor, '' );
} );
} );
} );
} );
} );
Expand Down
Loading