Bug 58800 - Delete removes two characters at a time in a container that has :first-letter applied to it
Summary: Delete removes two characters at a time in a container that has :first-letter...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 58911
  Show dependency treegraph
 
Reported: 2011-04-18 11:13 PDT by Justin Garcia
Modified: 2012-06-07 21:12 PDT (History)
7 users (show)

See Also:


Attachments
testcase (173 bytes, text/html)
2011-04-18 11:13 PDT, Justin Garcia
no flags Details
Reference Patch. (1.15 KB, patch)
2012-05-28 00:01 PDT, Antaryami Pandia (apandia)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2011-04-18 11:13:26 PDT
Open the attached test case
Click after the 'o' in "Francisco" to start editing
Start deleting 

Delete removes two characters at a time.

<rdar://problem/9299050> Delete removes two characters at a time in a container that has :first-letter applied to it

<head>
<style>
div {
    font-size: 48px;
}
div:first-letter {
    color: blue;
}
</style>
</head>
<body contentEditable="true">
<div>Graffiti in San Francisco</div>
</body>
Comment 1 Justin Garcia 2011-04-18 11:13:58 PDT
Created attachment 90058 [details]
testcase
Comment 2 Justin Garcia 2011-04-18 11:26:18 PDT
This is more severe than I thought. Characters don't go where you insert them and WebKit hangs when you try to move the caret before the first letter of the container.
Comment 3 Hajime Morrita 2011-04-18 12:52:58 PDT
There might be similar old-4digit bug but I dont' remember.
Kent-san, do you remember that?
Comment 4 Alexey Proskuryakov 2011-04-18 14:41:56 PDT
See also: bug 26442, bug 26443, bug 43830
Comment 5 Kent Tamura 2011-04-19 11:33:32 PDT
(In reply to comment #3)
> There might be similar old-4digit bug but I dont' remember.
> Kent-san, do you remember that?

I'm not sure this bug is same as bugs Alexey listed.

I'll make a meta bug for :first-letter.
Comment 6 Alexey Proskuryakov 2011-04-19 11:43:04 PDT
Re-titling back to track only the specific issue reported originally, now that we have a separate meta bug.
Comment 7 Antaryami Pandia (apandia) 2012-05-28 00:01:47 PDT
Created attachment 144283 [details]
Reference Patch.

The string contains 25 characters. With current webkit design when there is a first letter preset it uses RenderTextFragment. For this string it creates 2 RenderTextFragment as follows:-
1. RenderTextFragments with first letter, start offset:0 and endoffset/length:1
2. RenderTextFragments with remaining letter, start offset: 1 and endoffset/length:24

Now when we place the cursor at the end , the offsets are as follows:-
-  offset 25 in original string and offset 24 in RenderTextFragment for remaining text. 

Now when we delete the text, it uses the RenderTextFragmnets offsets to calclulate the new offset. Since the current offset for it is 24 it calculates the new offsets as 23.

Now while deleting the characters it uses the original string (with 25 characters), and the new offsets calculated using RenderTextFragment (which is 23). 

The code that is used to delete is "DeleteSelectionCommand::handleGeneralDelete".
In this method it uses the offset to determine the length as follows:-
deleteTextFromNode(text, startOffset, m_downstreamEnd.deprecatedEditingOffset() - startOffset);

So since the new offset (calculated using the RenderTextFragment) is 23 and the value of m_downstreamEnd.deprecatedEditingOffset (which is same as the length of original string) is 25, the number of elements to be removed is calculated as 2. Hence it deletes 2 characters.

So there is a mismatch in calculation of offsets when first letter is involved.So if we add the start offset value of RenderTextFragment to the offset that is calculated in deletes one character. A reference patch is attached.

Please provide your feedback.
Comment 8 Antaryami Pandia (apandia) 2012-06-07 02:02:01 PDT
As I have mentioned in my previous analysis, the issue is caused by using renderTextFragments offsets while calculating the offsets position. I have been working on to find some solution and I have below ideas:

1. We should make RenderTextFragment child of text renderer (also suggested by Levi in bug 45174).
2. We should implement first-letter in the same way first-line is implemented.
   - I checked the selector spec and I think it didn't mandate in specific internal implementation (http://www.w3.org/TR/css3-selectors/). Please correct me if my understanding is wrong.

I am trying to figure out other options. Request you to provide feedback on the above mentioned options.
Comment 9 Ryosuke Niwa 2012-06-07 18:10:58 PDT
We can't implement first-letter like we implement first-line so the only option is 1. Also see the bug I'cced you about how we should have regular children under RenderTextFragment.
Comment 10 Ryosuke Niwa 2012-06-07 18:13:57 PDT
See https://bugs.webkit.org/show_bug.cgi?id=63368
Comment 11 Antaryami Pandia (apandia) 2012-06-07 19:52:30 PDT
(In reply to comment #9)
> We can't implement first-letter like we implement first-line so the only option is 1. 
Thanks for the feedback. I will work on this.

Also see the bug I'cced you about how we should have regular children under RenderTextFragment.

are you talking of bug 63368? yes, I am looking at.
Comment 12 Antaryami Pandia (apandia) 2012-06-07 20:10:53 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > We can't implement first-letter like we implement first-line so the only option is 1. 
> Thanks for the feedback. I will work on this.

also should I post my patches here or on bug 63368?
bug 63368 seems more relevent for the RenderText/RenderTextFragment change.
Comment 13 Ryosuke Niwa 2012-06-07 21:12:05 PDT
(In reply to comment #11)
> are you talking of bug 63368? yes, I am looking at.

Yes.

(In reply to comment #12)
> also should I post my patches here or on bug 63368?
> bug 63368 seems more relevent for the RenderText/RenderTextFragment change.

Right.