Bug 254989

Summary: REGRESSION(261836@main): Incorrect caret placement after hitting enter key in long text
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, bugs-noreply, darin, koivisto, mcatanzaro, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Test case
none
Patch
none
Test case
none
Patch
none
[fast-cq]Patch none

Description Michael Catanzaro 2023-04-04 05:31:22 PDT 
Since upgrading to 2.41.1, the Enter key does not always work reliably in text fields. When pressing Enter, a new line should get added to the web content and the cursor should advance to the next line. However, since 2.41.1, sometimes the cursor does not advance, giving the impression that the Enter key is not doing anything. In fact, it is still adding new lines to the text field, but there is a delay before the lines get added, and the cursor does not move. This bug may occur on all websites: yesterday I saw it on Bugzilla, GitLab, GitHub, and Jira. In fact, I've encountered it just now, trying to report this bug.

I managed to find a reproducer. Load https://bugs.webkit.org/enter_bug.cgi?product=WebKit, then paste the following text in the Description field:

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.

Then press Enter, then Backspace, then Enter again. It will break. This does not happen with smaller amounts of text: you need a certain amount of text to trigger the bug.
Comment 1 Michael Catanzaro 2023-04-17 20:21:18 PDT 
Bisected this to https://commits.webkit.org/261836@main "[IFC][Partial layout] Populate InlineDamage::m_trailingDisplayBoxes for subsequent partial layout"
Comment 2 zalan 2023-04-18 07:01:32 PDT 
Created attachment 465965 [details]
Test case

This is a manual test as inserting "new line" programmatically works fine.
Comment 3 zalan 2023-04-18 07:03:15 PDT 
> Then press Enter, then Backspace, then Enter again. It will break. This does
> not happen with smaller amounts of text: you need a certain amount of text
> to trigger the bug.
Thank you for the test case! Looking into it now.
Comment 4 Michael Catanzaro 2023-04-18 08:47:51 PDT 
(In reply to zalan from comment #2)
> Created attachment 465965 [details]
> Test case
> 
> This is a manual test as inserting "new line" programmatically works fine.

I actually cannot reproduce the bug following this test case unless I add step two "press enter, then backspace" before step three "press enter"
Comment 5 zalan 2023-04-18 09:37:15 PDT 
(In reply to Michael Catanzaro from comment #4)
> (In reply to zalan from comment #2)
> > Created attachment 465965 [details]
> > Test case
> > 
> > This is a manual test as inserting "new line" programmatically works fine.
> 
> I actually cannot reproduce the bug following this test case unless I add
> step two "press enter, then backspace" before step three "press enter"
I was using MiniBrowser with WK1 and I was able to repro it, but you are right with WK2, those additional steps are required. Not sure what's up with WK1 -will upload a new test case.
Comment 6 zalan 2023-04-18 11:32:54 PDT 
Created attachment 465967 [details]
Patch
Comment 7 zalan 2023-04-18 11:33:26 PDT 
This fixes this issue, but I still need to come up with a test case.
Comment 8 zalan 2023-04-18 11:38:29 PDT 
Created attachment 465968 [details]
Test case
Comment 9 Radar WebKit Bug Importer 2023-04-18 12:11:01 PDT 
<rdar://problem/108215532>
Comment 10 zalan 2023-04-18 12:40:20 PDT 
Created attachment 465970 [details]
Patch
Comment 11 zalan 2023-04-18 12:41:12 PDT 
Created attachment 465971 [details]
[fast-cq]Patch
Comment 12 Darin Adler 2023-04-18 12:55:47 PDT 
Don’t need the braces on this line:

std::optional<size_t> offset { };
Comment 13 zalan 2023-04-18 20:11:36 PDT 
(In reply to Darin Adler from comment #12)
> Don’t need the braces on this line:
> 
> std::optional<size_t> offset { };
Indeed. The reason I tend to add is it lets me omit an extra { } at the callsites

leadingInlineItemPositionByDamagedBox({ *previousSibling },...
vs.
leadingInlineItemPositionByDamagedBox({ *previousSibling, { } },...

(error: missing field 'offset' initializer [-Werror,-Wmissing-field-initializers]
            damagedLine = leadingInlineItemPositionByDamagedBox({ *previousSibling }, m_inlineItems, displayBoxes());)
Comment 14 EWS 2023-04-18 20:16:09 PDT 
Committed 263113@main (b6df426f37df): <https://commits.webkit.org/263113@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465971 [details].
Comment 15 Darin Adler 2023-04-19 09:04:39 PDT 
(In reply to zalan from comment #13)
> (In reply to Darin Adler from comment #12)
> > Don’t need the braces on this line:
> > 
> > std::optional<size_t> offset { };
> Indeed. The reason I tend to add is it lets me omit an extra { } at the
> callsites

I see. Seems like a good reason to keep it.