Bug 45174 - Freeze in VisiblePosition::leftVisuallyDistinctCandidate with ::first-letter
Summary: Freeze in VisiblePosition::leftVisuallyDistinctCandidate with ::first-letter
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 58911
  Show dependency treegraph
 
Reported: 2010-09-03 07:29 PDT by Thaddee Tyl
Modified: 2012-05-29 23:01 PDT (History)
7 users (show)

See Also:


Attachments
WebPage with ::first-letter and contenteditable applied. (99 bytes, text/html)
2010-09-03 07:29 PDT, Thaddee Tyl
no flags Details
Removing leading space leads to hang (97 bytes, text/html)
2010-11-09 14:11 PST, Levi Weintraub
no flags Details
Patch (17.22 KB, patch)
2010-11-16 14:34 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thaddee Tyl 2010-09-03 07:29:51 PDT
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.
Comment 1 Alexey Proskuryakov 2010-09-03 13:19:56 PDT
Hangs in VisiblePosition::leftVisuallyDistinctCandidate().
Comment 2 Alexey Proskuryakov 2010-09-03 13:20:19 PDT
<rdar://problem/8393558>
Comment 3 Levi Weintraub 2010-11-09 11:47:05 PST
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'.
Comment 4 Levi Weintraub 2010-11-09 14:11:34 PST
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.
Comment 5 Levi Weintraub 2010-11-10 11:46:54 PST
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.
Comment 6 Levi Weintraub 2010-11-11 15:50:20 PST
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.
Comment 7 Levi Weintraub 2010-11-16 14:34:09 PST
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.
Comment 8 Antaryami Pandia (apandia) 2012-05-28 17:58:38 PDT
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.
Comment 9 Levi Weintraub 2012-05-29 16:25:03 PDT
(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 :)
Comment 10 Antaryami Pandia (apandia) 2012-05-29 23:01:48 PDT
(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.