Bug 45174 – Freeze in VisiblePosition::leftVisuallyDistinctCandidate with ::first-letter

• Component: HTML Editing
• Status: NEW

[Thaddee Tyl]

Created attachment 66491 [details]
WebPage with ::first-letter and contenteditable applied.

See attachment.

Steps to reproduce crash:

1. Open attachment in Webkit, Safari or Chrome.
2. Click on the right of the 'O'.
3. Type a single space.
4. Press the left arrow key twice.
5. Now, it is too late. The browser (or the tab, in Chrome)
is no longer responding. It uses all the CPU until killed.

[Alexey Proskuryakov]

Hangs in VisiblePosition::leftVisuallyDistinctCandidate().

[Alexey Proskuryakov]

<rdar://problem/8393558>

[Levi Weintraub]

This no longer hangs on TOT WebKit in Safari or Chrome. Instead, typing space causes all the text to be lost. Also, hitting the left arrow key causes the caret to keep jumping left and right to between the 'K' and 'O' and after the 'O', but never actually getting before the 'K'.

[Levi Weintraub]

Created attachment 73413 [details]
Removing leading space leads to hang

Removing the leading space before the 'K' actually causes the hang to once-again manifest in TOT WebKit.

[Levi Weintraub]

The first-letter implementation seems to break assumptions made throughout WebKit. In our example, we end up with a render tree that looks something like this:

RenderBlock
- RenderInline
-- RenderTextFragment "K"
- RenderTextFragment "O"

where "O" is considered the renderer for the DOM text node "KO". Breaking the RenderText into two RenderTextFragments who's first common ancestor is a renderer for a different node causes the first letter to be "lost" in visible units. It seems like we need to nest the RenderTextFragments and the RenderInline that contains the first-letter style inside of a new RenderObject that is considered the DOM node's renderer.

[Levi Weintraub]

I started working on a fix for this but unfortunately reached a bit of a head... Position and VisiblePosition code needs to be made entirely too aware of the atypical Renderer layout of text nodes with first-letter content for my fix to be clean.

First of all, it seems that Position shouldn't be aware of Renderer code, and should be able to get all it needs from the DOM. Furthermore, IMHO VisiblePosition could be cleaned up to push strange behavior like this into Renderer's own implementation, instead of trying to handle all cases in one class.

[Levi Weintraub]

Created attachment 74042 [details]
Patch

This patch solves a number of issues surrounding first-letter:
- leftVisuallyDistinctCandidate no longer hangs
- the caret can correctly be positioned in first-letter content
- typing at the end of an editable field with first-letter content no longer replaces the final character (Bug 26442)

There are still serious issues with selection and contenteditable, all stemming from the fundamental break from the rendering norm that first-letter presents by having part of a text node's rendered content in a fully separate renderer than the one it directly owns. Solving the remaining bugs involves making more parts of the code aware of this specific render-tree structure, which makes me uncomfortable.

I believe that having the text node's renderer be the parent (or grand-parent) of *all* associated RenderTextFragments would solve many of these problems, but I'll defer to those who better understand WebKit rendering. Feedback is very appreciated. I think this solution is not ideal.

[Antaryami Pandia (apandia)]

I am currently working on html editing issues and found that with current implentation, there is a gap in how the offsets are calculated using RenderTextFragments (bug 58800). So I think there is a need to change the current working of RenderTextFragments.

Levi Weintraub: If you are not working on the issue, then I would like to take the issue forward.

Thanks.

[Levi Weintraub]

(In reply to comment #8)
> I am currently working on html editing issues and found that with current implentation, there is a gap in how the offsets are calculated using RenderTextFragments (bug 58800). So I think there is a need to change the current working of RenderTextFragments.
> 
> Levi Weintraub: If you are not working on the issue, then I would like to take the issue forward.
> 
> Thanks.

Hi Antaryami!

I'd love to have your patch. I haven't been thinking about this problem for quite some time and would be happy to take a look at any patches you have :)

[Antaryami Pandia (apandia)]

(In reply to comment #9)
> 
> Hi Antaryami!
> 
> I'd love to have your patch. I haven't been thinking about this problem for quite some time and would be happy to take a look at any patches you have :)

Thanks Levi.
I don't have a patch ready with me. But while working on editing issues I also found same issue that you have analyzed. There is an issue how the offsets are calculated and used using RenderTextFragment.

I am working on to make editing works with first letter.
I will also appreciate any direction/guide from you.