1840700 - Bug 1815802 broke line break behaviour for CKEditor

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Sören Hentzschel]

The change in bug 1815802 broke the line break behaviour for CKEditor, one of the world's most popular WYISWYG editors. Please note: The issue does not always happen but very often. If it doesn't happen just reload the page and try again. For me it always happened with the second or third try at the latest.

STR scenario 1:

1. Open https://ckeditor.com/ckeditor-5/demo/feature-rich/
2. Remove the content, enter some content
3. Press Shift + Enter to make a line break
4. Enter more content

Actual:

The cursor stays in the first line after step 3, so you think that no line break was applied. But after step 4 the additional content is in a new line, so in fact the line break worked but it was not obvious to the user.

Expected:

The cursor is in a new line after step 3.

STR scenario 2:

1. Open https://ckeditor.com/ckeditor-5/demo/feature-rich/
2. Remove the content, enter some content
3. Press Shift + Enter to make a line break
4. Since you think that the line break didn't work repeat step 3
5. Enter more content

Actual:

After step 3 the cursor is still in the first line (see scenario 1), after step 4 the cursor is in a new line. After step 5 an additional line break is between both lines of text.

Expected:

The cursor should be in the second line after step 3, a further line should be added after step 4, no jump after step 5 because the additional line was already added after step 4.

I also reported it to the developers of CKEditor:
https://github.com/ckeditor/ckeditor5/issues/14479

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The latest selection API calls are at typing Shift + Enter:

I/SelectionAPI 21c527154a0 Selection::CollapseJS(aContainer=p.div[contenteditable="true"].div.div.div.div['b-demo-editor'].div.div.section.main.body.html.#document, aOffset=2)
D/SelectionAPI
#01: mozilla::dom::Selection::CollapseJS (M:\src\dom\base\Selection.cpp:2349)
#02: mozilla::dom::Selection_Binding::collapse (M:\fx64-dbg\dom\bindings\SelectionBinding.cpp:549)
#03: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (M:\src\dom\bindings\BindingUtils.cpp:3329)
#04: CallJSNative (M:\src\js\src\vm\Interpreter.cpp:486)
#05: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:580)
#06: js::Interpret (M:\src\js\src\vm\Interpreter.cpp:3395)
#07: js::RunScript (M:\src\js\src\vm\Interpreter.cpp:458)
#08: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:612)

I/SelectionAPI 21c527154a0 Selection::ExtendJS(aContainer=p.div[contenteditable="true"].div.div.div.div['b-demo-editor'].div.div.section.main.body.html.#document, aOffset=2)
D/SelectionAPI
#01: mozilla::dom::Selection::ExtendJS (M:\src\dom\base\Selection.cpp:2684)
#02: mozilla::dom::Selection_Binding::extend (M:\fx64-dbg\dom\bindings\SelectionBinding.cpp:751)
#03: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (M:\src\dom\bindings\BindingUtils.cpp:3329)
#04: CallJSNative (M:\src\js\src\vm\Interpreter.cpp:486)
#05: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:580)
#06: js::Interpret (M:\src\js\src\vm\Interpreter.cpp:3395)
#07: js::RunScript (M:\src\js\src\vm\Interpreter.cpp:458)
#08: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:612)

I/SelectionAPI 21c527154a0 Selection::AddRangeJS(aRange={ mStart=mEnd={ mParent=0000021C5289CB80 (p.div[contenteditable="true"].div.div.div.div['b-demo-editor'].div.div.section.main.body.html.#document, Length()=3), mRef=br.p.div[contenteditable="true"].div.div.div.div['b-demo-editor'].div.div.section.main.body.html.#document @ 0000021C528978B0 (br.p.div[contenteditable="true"].div.div.div.div['b-demo-editor'].div.div.section.main.body.html.#document), mOffset=2, mIsMutationObserved=true }, mIsGenerated=false, mCalledByJS=true, mIsDynamicRange=true })
D/SelectionAPI
#01: mozilla::dom::Selection::AddRangeJS (M:\src\dom\base\Selection.cpp:2146)
#02: mozilla::dom::Selection_Binding::addRange (M:\fx64-dbg\dom\bindings\SelectionBinding.cpp:355)
#03: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (M:\src\dom\bindings\BindingUtils.cpp:3329)
#04: CallJSNative (M:\src\js\src\vm\Interpreter.cpp:486)
#05: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:580)
#06: js::Interpret (M:\src\js\src\vm\Interpreter.cpp:3395)
#07: js::RunScript (M:\src\js\src\vm\Interpreter.cpp:458)
#08: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:612)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, it seems that we need to do this:
https://searchfox.org/mozilla-central/rev/926b6c9fc7d84b603f501448c31d489473862bca/dom/base/Selection.cpp#2216-2219

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Workaround for web apps is, call removeAllRanges() before calling addRange().

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1840700 - Make `Selection::AddRangeAndSelectFramesAndNotifyListenersInternal()` update interline position even if it does nothing r=jjaschke!

It always set to "start of next line" when new range was added. However,
after bug 1815802, it does nothing when adding range is one of its ranges.
Therefore, if interline position is set to "end of line" by our internal
code, like editor, getSelection().addRange(getSelection().getRangeAt(0))
keeps the last interline position.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4144104e69d6
Make `Selection::AddRangeAndSelectFramesAndNotifyListenersInternal()` update interline position even if it does nothing r=jjaschke

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4144104e69d6

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 8

[Sören Hentzschel]

Thank you very much for the really fast fix!

Since Firefox 115 is already a RC, the bug exists since Firefox 111 and it doesn't seem that there are many similar reports if any at all I guess it's fine to WONTFIX this for the Firefox 115.0 release and let it ride the train to Firefox 116. But as Firefox 115 will be the new ESR and this bug can be very annoying when using CKEditor I suggest to consider this for Firefox ESR 115.1.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Sören Hentzschel from comment #8)

Since Firefox 115 is already a RC, the bug exists since Firefox 111 and it doesn't seem that there are many similar reports if any at all I guess it's fine to WONTFIX this for the Firefox 115.0 release and let it ride the train to Firefox 116. But as Firefox 115 will be the new ESR and this bug can be very annoying when using CKEditor I suggest to consider this for Firefox ESR 115.1.

Thank you. Yeah, I completely agree with that.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9341439 [details]
Bug 1840700 - Make Selection::AddRangeAndSelectFramesAndNotifyListenersInternal() update interline position even if it does nothing r=jjaschke!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is an annoying bug for users (caret is not painted at proper position), a new regression (from point of view of ESR) starting from 111 and reproducible with a major editing web application framework.
• User impact if declined: While this bug is reproduced, typing text is inserted to different position (start of next line).
• Fix Landed on Version: 116
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only resetting "interline position" which was done before landing bug 1815802. That specifies the caret position (if selection is collapsed at <br>) whether end of previous line or start of new line.

Comment 11

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9341439 [details]
Bug 1840700 - Make Selection::AddRangeAndSelectFramesAndNotifyListenersInternal() update interline position even if it does nothing r=jjaschke!

Approved for 115.1esr

Comment 12

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

https://hg.mozilla.org/releases/mozilla-esr115/rev/f15ac4f8bfd3

Comment 13

[Peter Magyari (Desktop QA)]

I have reproduced the issue using Firefox 114.0.2 (20230619081400) and verified the fix using Firefox Nightly 116.0a1 (20230628153602) and Firefox 115.1.0esr (20230706135840) on MacOS 11.6, Ubuntu 20.04 and Windows 10.