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

Deleting and entering newlines (soft enter) at the end of long document causes auto-scroll to top #14028

Closed
tomcdonnell opened this issue May 4, 2023 · 6 comments · Fixed by #15691
Assignees
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:collaboration Issue to be handled by the Collaboration team. squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@tomcdonnell
Copy link

tomcdonnell commented May 4, 2023

📝 Provide detailed reproduction steps (if any)

  1. Visit CKEditor demo page: https://ckeditor.com/ckeditor-5/demo/feature-rich/
  2. Delete all content from CKEditor field
  3. Paste content from attached file (long_text_file_no_html_tags.txt)
  4. Scroll to bottom of document
  5. Press backspace a few times until a newline is deleted.

✔️ Expected result

No auto-scrolling occurs

❌ Actual result

The page auto-scrolls to the top of the CKEditor field.
(See attached video file for video demonstration)

📃 Other details

  • Browser: Firefox 111.0.1 (64-bit)
  • OS: Ubuntu 22.04.2 LTS
  • First affected CKEditor version: Don't know
  • Installed CKEditor plugins: None I think since bug occurs on demo page

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

ckeditor_autoscroll_bug_on_demo_page.webm
long_text_file_no_html_tags.txt

@tomcdonnell tomcdonnell added the type:bug This issue reports a buggy (incorrect) behavior. label May 4, 2023
@mlewand
Copy link
Contributor

mlewand commented Jul 27, 2023

This issue could have the same underlying issue as #14605

@Witoso
Copy link
Member

Witoso commented Jul 31, 2023

@mlewand AFAICS it happens when the selection lands after delete at the end of a soft brake. The same case as in a code block, right?

@Witoso Witoso added domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:core Issue to be handled by the Core team. labels Jul 31, 2023
@niegowski
Copy link
Contributor

This is happening because this code can't resolve proper client rect and falls back to parent container bounding rect:

if ( clientRects.length ) {
for ( const rect of clientRects ) {
rects.push( new Rect( rect ) );
}
}
// If there's no client rects for the Range, use parent container's bounding rect
// instead and adjust rect's width to simulate the actual geometry of such range.
// https://github.com/ckeditor/ckeditor5-utils/issues/153
// https://github.com/ckeditor/ckeditor5-ui/issues/317
else {
let startContainer = range.startContainer;
if ( isText( startContainer ) ) {
startContainer = startContainer.parentNode!;
}
const rect = new Rect( ( startContainer as Element ).getBoundingClientRect() );
rect.right = rect.left;
rect.width = 0;
rects.push( rect );
}

@niegowski
Copy link
Contributor

After a deeper checking, I noticed that the range.getClientRects() has a problem resolving screen coordinates if the range is not attached to any text node. In the case of <br/>[]<br/> it fails to return client rect. I noticed that it happens only in the last line (before the final <br> - a block filler). In the other lines there is an inline filler and client rects are resolved properly:
2023-08-22 16 53 48
(Note the missing &NoBreak;&NoBreak;... - an inline filler, before the block filler)

The line responsible for not adding an inline filler is:

// We have block filler, we do not need inline one.
if ( selectionOffset === selectionParent.getFillerOffset!() ) {
return false;
}

And while checking the getFillerOffset() of the ContainerElement I noticed that it was updated to support block filler after the last <br>:

// Block filler is required after a `<br>` if it's the last element in its container. See #1422.
if ( lastChild && lastChild.is( 'element', 'br' ) ) {
return this.childCount;
}

but this was not applied to the inline filler logic that should allow for inline filler next to a block filler if this is after another <br>.

@Rashid101
Copy link

Where and what is the solution @Witoso ??

@Witoso
Copy link
Member

Witoso commented Sep 13, 2023

@Rashid101 there's none, this is an open bug on our side. Please +1 (:+1: ), the top comment.

@Witoso Witoso changed the title Deleting newlines from end of long document causes auto-scroll to top of ckeditor field Deleting and entering newlines (soft enter) at the end of long document causes auto-scroll to top Sep 13, 2023
@Mati365 Mati365 self-assigned this Feb 8, 2024
@DawidKossowski DawidKossowski added squad:collaboration Issue to be handled by the Collaboration team. and removed squad:core Issue to be handled by the Core team. labels Feb 8, 2024
@DawidKossowski DawidKossowski added the squad:core Issue to be handled by the Core team. label Feb 21, 2024
niegowski added a commit that referenced this issue Apr 10, 2024
Fix (engine): An inline filler should be rendered after a BR just before a block filler so that scrolling to selection could properly find the client rect. Closes #14028.
@CKEditorBot CKEditorBot added this to the iteration 74 milestone Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:collaboration Issue to be handled by the Collaboration team. squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
8 participants