Skip to content

Commit

Permalink
Merge pull request #14222 from ckeditor/ck/13730-scroll-to-the-select…
Browse files Browse the repository at this point in the history
…ion-viewport-offset

Fix (engine): `View#scrollToTheSelection()` should respect the `config.ui.viewportOffset` configuration and consider the geometry of a classic editor's toolbar to assure the caret is always visible to the user. Closes #13730.
  • Loading branch information
oleq committed Jun 1, 2023
2 parents 80daee5 + 6241605 commit d65f486
Show file tree
Hide file tree
Showing 9 changed files with 530 additions and 25 deletions.
46 changes: 44 additions & 2 deletions packages/ckeditor5-editor-classic/src/classiceditorui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

import type { Editor, ElementApi } from 'ckeditor5/src/core';
import { EditorUI, normalizeToolbarConfig, type EditorUIReadyEvent } from 'ckeditor5/src/ui';
import { enablePlaceholder } from 'ckeditor5/src/engine';
import { ElementReplacer } from 'ckeditor5/src/utils';
import {
enablePlaceholder,
type ViewScrollToTheSelectionEvent
} from 'ckeditor5/src/engine';
import { ElementReplacer, Rect, type EventInfo } from 'ckeditor5/src/utils';
import type ClassicEditorUIView from './classiceditoruiview';

/**
Expand Down Expand Up @@ -44,6 +47,9 @@ export default class ClassicEditorUI extends EditorUI {
this.view = view;
this._toolbarConfig = normalizeToolbarConfig( editor.config.get( 'toolbar' ) );
this._elementReplacer = new ElementReplacer();

this.listenTo<ViewScrollToTheSelectionEvent>(
editor.editing.view, 'scrollToTheSelection', this._handleScrollToTheSelectionWithStickyPanel.bind( this ) );
}

/**
Expand Down Expand Up @@ -165,5 +171,41 @@ export default class ClassicEditorUI extends EditorUI {
} );
}
}

/**
* Provides an integration between the sticky toolbar and {@link module:utils/dom/scroll~scrollViewportToShowTarget}.
* It allows the UI-agnostic engine method to consider the geometry of the
* {@link module:editor-classic/classiceditoruiview~ClassicEditorUIView#stickyPanel} that pins to the
* edge of the viewport and can obscure the user caret after scrolling the window.
*
* @param evt The `scrollToTheSelection` event info.
* @param data The payload carried by the `scrollToTheSelection` event.
* @param originalArgs The original arguments passed to `scrollViewportToShowTarget()` method (see implementation to learn more).
*/
private _handleScrollToTheSelectionWithStickyPanel(
evt: EventInfo<'scrollToTheSelection'>,
data: ViewScrollToTheSelectionEvent[ 'args' ][ 0 ],
originalArgs: ViewScrollToTheSelectionEvent[ 'args' ][ 1 ]
): void {
const stickyPanel = this.view.stickyPanel;

if ( stickyPanel.isSticky ) {
const stickyPanelHeight = new Rect( stickyPanel.element! ).height;

data.viewportOffset.top += stickyPanelHeight;
} else {
const scrollViewportOnPanelGettingSticky = () => {
this.editor.editing.view.scrollToTheSelection( originalArgs );
};

this.listenTo( stickyPanel, 'change:isSticky', scrollViewportOnPanelGettingSticky );

// This works as a post-scroll-fixer because it's impossible predict whether the panel will be sticky after scrolling or not.
// Listen for a short period of time only and if the toolbar does not become sticky very soon, cancel the listener.
setTimeout( () => {
this.stopListening( stickyPanel, 'change:isSticky', scrollViewportOnPanelGettingSticky );
}, 20 );
}
}
}

159 changes: 158 additions & 1 deletion packages/ckeditor5-editor-classic/tests/classiceditorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* globals document, Event, console */
/* globals window, document, Event, console */

import View from '@ckeditor/ckeditor5-ui/src/view';

Expand Down Expand Up @@ -351,6 +351,157 @@ describe( 'ClassicEditorUI', () => {
expect( ui.getEditableElement( 'absent' ) ).to.be.undefined;
} );
} );

describe( 'View#scrollToTheSelection integration', () => {
it( 'should listen to View#scrollToTheSelection and inject the height of the panel into `viewportOffset` when sticky', async () => {
const editorElement = document.createElement( 'div' );
document.body.appendChild( editorElement );

const editor = await ClassicEditor.create( editorElement, {
ui: {
viewportOffset: {
top: 10,
bottom: 20,
left: 30,
right: 40
}
}
} );

editor.ui.view.stickyPanel.isSticky = true;
sinon.stub( editor.ui.view.stickyPanel.element, 'getBoundingClientRect' ).returns( {
height: 50
} );

editor.editing.view.once( 'scrollToTheSelection', ( evt, data ) => {
const range = editor.editing.view.document.selection.getFirstRange();

expect( data ).to.deep.equal( {
target: editor.editing.view.domConverter.viewRangeToDom( range ),
viewportOffset: {
top: 160,
bottom: 120,
left: 130,
right: 140
},
ancestorOffset: 20,
alignToTop: undefined,
forceScroll: undefined
} );
} );

editor.editing.view.scrollToTheSelection( { viewportOffset: 100 } );

editorElement.remove();
await editor.destroy();
} );

it( 'should listen to View#scrollToTheSelection and re-scroll if the panel was not sticky at the moment of execution' +
'but becomes sticky after a short while', async () => {
const editorElement = document.createElement( 'div' );
document.body.appendChild( editorElement );

const editor = await ClassicEditor.create( editorElement, {
ui: {
viewportOffset: {
top: 10,
bottom: 20,
left: 30,
right: 40
}
}
} );

editor.ui.view.stickyPanel.isSticky = false;
sinon.stub( editor.ui.view.stickyPanel.element, 'getBoundingClientRect' ).returns( {
height: 50
} );

const spy = sinon.spy();

editor.editing.view.on( 'scrollToTheSelection', spy );
editor.editing.view.scrollToTheSelection( { viewportOffset: 100 } );

const range = editor.editing.view.document.selection.getFirstRange();

// The first call will trigger another one shortly once the panel becomes sticky.
sinon.assert.calledWith( spy.firstCall, sinon.match.object, {
target: editor.editing.view.domConverter.viewRangeToDom( range ),
alignToTop: undefined,
forceScroll: undefined,
viewportOffset: { top: 110, bottom: 120, left: 130, right: 140 },
ancestorOffset: 20
} );

await wait( 10 );
editor.ui.view.stickyPanel.isSticky = true;

// This is the second and final scroll that considers the geometry of a now-sticky panel.
sinon.assert.calledWith( spy.secondCall, sinon.match.object, {
target: editor.editing.view.domConverter.viewRangeToDom( range ),
alignToTop: undefined,
forceScroll: undefined,
viewportOffset: { top: 160, bottom: 120, left: 130, right: 140 },
ancestorOffset: 20
} );

editorElement.remove();
await editor.destroy();
} );

it( 'should listen to View#scrollToTheSelection and refuse re-scrolling if the panel was not sticky at the moment of execution' +
'and its state it didn\'t change', async () => {
const editorElement = document.createElement( 'div' );
document.body.appendChild( editorElement );

const editor = await ClassicEditor.create( editorElement, {
ui: {
viewportOffset: {
top: 10,
bottom: 20,
left: 30,
right: 40
}
}
} );

editor.ui.view.stickyPanel.isSticky = false;
sinon.stub( editor.ui.view.stickyPanel.element, 'getBoundingClientRect' ).returns( {
height: 50
} );

const spy = sinon.spy();

editor.editing.view.on( 'scrollToTheSelection', spy );
editor.editing.view.scrollToTheSelection( { viewportOffset: 100 } );

const range = editor.editing.view.document.selection.getFirstRange();

// The first call can trigger another one shortly once the panel becomes sticky.
sinon.assert.calledWith( spy.firstCall, sinon.match.object, {
target: editor.editing.view.domConverter.viewRangeToDom( range ),
alignToTop: undefined,
forceScroll: undefined,
viewportOffset: { top: 110, bottom: 120, left: 130, right: 140 },
ancestorOffset: 20
} );

// This timeout exceeds the time slot for scrollToTheSelection() affecting the stickiness of the panel.
// If the panel hasn't become sticky yet as a result of window getting scrolled chances are this will never happen.
await wait( 30 );

sinon.assert.calledOnce( spy );

editor.ui.view.stickyPanel.isSticky = true;

// There was no second scroll even though the panel became sticky. Too much time has passed and the change of its state
// cannot be attributed to doings of scrollToTheSelection() anymore.
sinon.assert.calledOnce( spy );

editorElement.remove();
await editor.destroy();
} );
} );
} );

describe( 'Focus handling and navigation between editing root and editor toolbar', () => {
Expand Down Expand Up @@ -544,3 +695,9 @@ class VirtualClassicTestEditor extends VirtualTestEditor {
} );
}
}

function wait( time ) {
return new Promise( res => {
window.setTimeout( res, time );
} );
}
2 changes: 1 addition & 1 deletion packages/ckeditor5-engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export type {
export type { ViewDocumentTabEvent } from './view/observer/tabobserver';
export type { ViewDocumentClickEvent } from './view/observer/clickobserver';
export type { ViewDocumentSelectionChangeEvent } from './view/observer/selectionobserver';
export type { ViewRenderEvent } from './view/view';
export type { ViewRenderEvent, ViewScrollToTheSelectionEvent } from './view/view';

// View / Styles.
export { StylesProcessor, type BoxSides } from './view/stylesmap';
Expand Down
75 changes: 66 additions & 9 deletions packages/ckeditor5-engine/src/view/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ import {
import { injectUiElementHandling } from './uielement';
import { injectQuirksHandling } from './filler';

import { cloneDeep } from 'lodash-es';

type IfTrue<T> = T extends true ? true : never;
type DomRange = globalThis.Range;

/**
* Editor's view controller class. Its main responsibility is DOM - View management for editing purposes, to provide
Expand Down Expand Up @@ -393,6 +396,9 @@ export default class View extends ObservableMixin() {
* Scrolls the page viewport and {@link #domRoots} with their ancestors to reveal the
* caret, **if not already visible to the user**.
*
* **Note**: Calling this method fires the {@link module:engine/view/view~ViewScrollToTheSelectionEvent} event that
* allows custom behaviors.
*
* @param options Additional configuration of the scrolling behavior.
* @param options.viewportOffset A distance between the DOM selection and the viewport boundary to be maintained
* while scrolling to the selection (default is 20px). Setting this value to `0` will reveal the selection precisely at
Expand All @@ -411,22 +417,40 @@ export default class View extends ObservableMixin() {
viewportOffset = 20,
ancestorOffset = 20
}: {
readonly viewportOffset?: number;
readonly viewportOffset?: number | { top: number; bottom: number; left: number; right: number };
readonly ancestorOffset?: number;
readonly alignToTop?: T;
readonly forceScroll?: U;
} = {} ): void {
const range = this.document.selection.getFirstRange();

if ( range ) {
scrollViewportToShowTarget( {
target: this.domConverter.viewRangeToDom( range ),
viewportOffset,
ancestorOffset,
alignToTop,
forceScroll
} );
if ( !range ) {
return;
}

// Clone to make sure properties like `viewportOffset` are not mutated in the event listeners.
const originalArgs = cloneDeep( { alignToTop, forceScroll, viewportOffset, ancestorOffset } );

if ( typeof viewportOffset === 'number' ) {
viewportOffset = {
top: viewportOffset,
bottom: viewportOffset,
left: viewportOffset,
right: viewportOffset
};
}

const options = {
target: this.domConverter.viewRangeToDom( range ),
viewportOffset,
ancestorOffset,
alignToTop,
forceScroll
};

this.fire<ViewScrollToTheSelectionEvent>( 'scrollToTheSelection', options, originalArgs );

scrollViewportToShowTarget( options );
}

/**
Expand Down Expand Up @@ -776,3 +800,36 @@ export type ViewRenderEvent = {
name: 'render';
args: [];
};

/**
* An event fired at the moment of {@link module:engine/view/view~View#scrollToTheSelection} being called. It
* carries two objects in its payload (`args`):
*
* * The first argument is the {@link module:engine/view/view~ViewScrollToTheSelectionEventData object containing data} that gets
* passed down to the {@link module:utils/dom/scroll~scrollViewportToShowTarget} helper. If some event listener modifies it, it can
* adjust the behavior of the scrolling (e.g. include additional `viewportOffset`).
* * The second argument corresponds to the original arguments passed to {@link module:utils/dom/scroll~scrollViewportToShowTarget}.
* It allows listeners to re-execute the `scrollViewportToShowTarget()` method with its original arguments if there is such a need,
* for instance, if the integration requires re–scrolling after certain interaction.
*
* @eventName ~View#scrollToTheSelection
*/
export type ViewScrollToTheSelectionEvent = {
name: 'scrollToTheSelection';
args: [
ViewScrollToTheSelectionEventData,
Parameters<View[ 'scrollToTheSelection' ]>[ 0 ]
];
};

/**
* An object passed down to the {@link module:utils/dom/scroll~scrollViewportToShowTarget} helper while calling
* {@link module:engine/view/view~View#scrollToTheSelection}.
*/
export type ViewScrollToTheSelectionEventData = {
target: DomRange;
viewportOffset: { top: number; bottom: number; left: number; right: number };
ancestorOffset: number;
alignToTop?: boolean;
forceScroll?: boolean;
};

0 comments on commit d65f486

Please sign in to comment.