Summary: | REGRESSION(r244494): editing/pasteboard/5761530-1.html is flaky due to a simple line layout bug | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | zalan <zalan> | ||||||
Status: | REOPENED --- | ||||||||
Severity: | Normal | CC: | commit-queue, koivisto, tsavell, webkit-bug-importer, wenson_hsieh, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2019-04-30 13:42:23 PDT
It was specifically this change: --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -1125,7 +1125,7 @@ void WebPage::updateEditorStateAfterLayoutIfEditabilityChanged() Frame& frame = m_page->focusController().focusedOrMainFrame(); EditorStateIsContentEditable editorStateIsContentEditable = frame.selection().selection().isContentEditable() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No; if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) - sendPartialEditorStateAndSchedulePostLayoutUpdate(); + scheduleFullEditorStateUpdate(); } String WebPage::renderTreeExternalRepresentation() const ...that causes this test to be flaky. Reverting to always send the partial editor state update immediately when changing editability fixes the flakiness for me. I'll need to see what exactly in the UI process is depending on this state being up to date. ...this is even weirder than I thought! Simply asking whether the current selection is at the beginning of a sentence via FrameSelection::selectionAtSentenceStart (and not even using the result) in this function: Frame& frame = m_page->focusController().focusedOrMainFrame(); EditorStateIsContentEditable editorStateIsContentEditable = frame.selection().selection().isContentEditable() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No; - if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) + if (m_lastEditorStateWasContentEditable != editorStateIsContentEditable) { + frame.selection().selectionAtSentenceStart(); scheduleFullEditorStateUpdate(); + } ...seems to make this test pass reliably 🤔. Oh, I wonder this is an issue with simple line layout. Try disabling it via settings? (In reply to Ryosuke Niwa from comment #4) > Oh, I wonder this is an issue with simple line layout. Try disabling it via > settings? Oh interesting! Adding `internals.settings.setSimpleLineLayoutEnabled(false);` to the beginning of the test does seem to fix the flakiness... Okay. So I think this might be a bug in TextIterator / StylizedMarkupAccumulator with simple line layout code in which a tab character or 4 spaces is collapsed in its serialization even though white-space: pre is set. It's probably a bug in TextIterator::handleTextNode. When copying the content of a plain text, we end up using TextIterator to grab the content in StyledMarkupAccumulator::appendText (In reply to Ryosuke Niwa from comment #7) > It's probably a bug in TextIterator::handleTextNode. When copying the > content of a plain text, we end up using TextIterator to grab the content in > StyledMarkupAccumulator::appendText Yep, I'm poking around in and around this code at the moment :) I'll post some more findings in a bit. There's some behaviors regarding the lifetime of a renderer's SLL that are currently confusing me. Created attachment 368708 [details]
Reduction
Here's a reduction of the issue Zalan and I worked.
Alan, Ryosuke and I chatted about this over IRC. For now, let's disable SLL in this test to make it pass; Alan will continue investigating a fix in SLL logic to get innerText working for tab-spans in SLL. Created attachment 368714 [details]
Patch
Comment on attachment 368714 [details] Patch Clearing flags on attachment: 368714 Committed r244854: <https://trac.webkit.org/changeset/244854> All reviewed patches have been landed. Closing bug. Reopening to track the SLL issue described above. |