Bug 254989 – REGRESSION(261836@main): Incorrect caret placement after hitting enter key in long text

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Michael Catanzaro]

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.

[Michael Catanzaro]

Bisected this to https://commits.webkit.org/261836@main "[IFC][Partial layout] Populate InlineDamage::m_trailingDisplayBoxes for subsequent partial layout"

[zalan]

Created attachment 465965 [details]
Test case

This is a manual test as inserting "new line" programmatically works fine.

[zalan]

> 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.

[Michael Catanzaro]

(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"

[zalan]

(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.

[zalan]

Created attachment 465967 [details]
Patch

[zalan]

This fixes this issue, but I still need to come up with a test case.

[zalan]

Created attachment 465968 [details]
Test case

[Radar WebKit Bug Importer]

<rdar://problem/108215532>

[zalan]

Created attachment 465970 [details]
Patch

[zalan]

Created attachment 465971 [details]
[fast-cq]Patch

[Darin Adler]

Don’t need the braces on this line:

std::optional<size_t> offset { };

[zalan]

(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());)

[EWS]

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].

[Darin Adler]

(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.