Bug 7245 - caret does not display at the end of some lines in contenteditable divs
Summary: caret does not display at the end of some lines in contenteditable divs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-14 03:45 PST by Graham Dennis
Modified: 2006-02-24 19:01 PST (History)
1 user (show)

See Also:


Attachments
patch (629 bytes, patch)
2006-02-17 06:36 PST, Graham Dennis
justin.garcia: review-
Details | Formatted Diff | Diff
automated testcase (685 bytes, text/html)
2006-02-17 15:55 PST, Graham Dennis
no flags Details
fixed testcase (932 bytes, text/html)
2006-02-17 16:07 PST, Graham Dennis
no flags Details
fixed (x2) testcase (796 bytes, text/html)
2006-02-17 16:09 PST, Graham Dennis
no flags Details
patch 2 (3.62 KB, patch)
2006-02-18 15:33 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 3 (4.04 KB, patch)
2006-02-22 04:14 PST, Graham Dennis
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Graham Dennis 2006-02-14 03:45:54 PST
When arrowing to the end of a line in a contentEditable div (that isn't the last line), the caret moves from before the penultimate character on a line to before the start of the first character on the next line. Entering text replaces the space character between the words on the separate lines as though the caret was now a selection encompassing the space between the words.

Using the mouse it is possible to place the cursor at the end of a line (it displays correctly), but the behaviour on entering text is the same as when using the arrow keys.
Comment 1 Graham Dennis 2006-02-14 03:50:22 PST
The testcase for #6933 can also be used as a testcase for this bug.
http://bugzilla.opendarwin.org/attachment.cgi?id=6105
Comment 2 Joost de Valk (AlthA) 2006-02-15 16:29:27 PST
In the latest nightly from http://nightly.webkit.org/ this works good for me it seems.
Comment 3 Graham Dennis 2006-02-15 22:04:33 PST
I have just verified that the bug is still present in the latest nightly. Although you can get the caret to the end of the line using CMD+Right (or by clicking), positioning the caret before the last character on a line, and pressing 'right' does still have the same effect. Nothing has changed since Safari in 10.4.5
Comment 4 Graham Dennis 2006-02-17 06:36:39 PST
Created attachment 6564 [details]
patch

This problem is caused by the contentEditable styling not being applied for elements which do not have a renderer when setContentEditable() is called on the root element. This bug is not present in Mail.app as the HTML body is set editable programmatically once the renderers exist. The check is in applyEditingStyleToElement, and is used to check that the element that the editing style is being applied to has a renderer that is a BlockFlow. However, this check doesn't make sense (to me) as although the root contentEditable element may not be a BlockFlow, elements that inherit the style from it may be.
My solution is to simply remove the BlockFlow check (and the renderer check).
Comment 5 Justin Garcia 2006-02-17 14:35:51 PST
Looks good.  Graham: how did you test?
Comment 6 Graham Dennis 2006-02-17 15:55:12 PST
Created attachment 6578 [details]
automated testcase

Automated testcase. In current ToT, the space between 'div' and 'editable' disappears upon inserting an 'x', giving 'divxis'. With the patch, the character 'x' is inserted before 'is'. This gives 'div xis'.
Comment 7 Graham Dennis 2006-02-17 16:07:09 PST
Created attachment 6579 [details]
fixed testcase

Test is now self-documenting
Comment 8 Graham Dennis 2006-02-17 16:09:00 PST
Created attachment 6580 [details]
fixed (x2) testcase

I forgot to remove unnecessary javascript
Comment 9 Justin Garcia 2006-02-17 16:26:47 PST
Comment on attachment 6564 [details]
patch

I'm r-'ing this until Graham and I go through all of the layout test changes that this change causes.
Comment 10 Justin Garcia 2006-02-17 17:19:37 PST
This patch is wrong.  For elements defined in the source to be contenteditable, applyEditingStyleToElement is called at parse time before style has been resolved.  At that time, currentStyle will be empty, regardless of what style the element may eventually have.  So, instead of being appended to style on the element, the editing style will blow away style on the element.  It seems to me that this is something that should happen when a contenteditable element is attached. 
Comment 11 Graham Dennis 2006-02-18 15:33:21 PST
Created attachment 6592 [details]
patch 2

New patch.

In this patch, instead of modifying the inlineStyleDecl of an element, I've modified setContentEditable (called by parseMappedAttribute) to add in the editing CSS properties. I've modified Frame::applyEditingStyleToElement (and removeEditingStyle) to simply call setContentEditable on the element. This way the inline style doesn't get blown away. Also, this way, the editing style can be overridden by the inline style of the element.

This patch breaks many layout tests as it adds a space to the end of each line in a contentEditable div. The vast majority of the problems are simply adding a space and increasing the width of the rendered element (or its parents).
However, for two layout tests, this is not the case:
fast/overflow/overflow-focus-ring
fast/text/justified-text-rect

For the overflow-focus-ring test, the periods change from being on a single line to being on many lines. This is caused by the "word-wrap: break-word" style.

For the justified-text-rect test, the test checks that text is justified correctly, however "-khtml-nbsp-mode: space" interferes with this. To be honest, I don't know what "-khtml-nbsp-mode: space" does and whether the layout test needs to be changed, the patch, or something else.
Comment 12 Justin Garcia 2006-02-21 13:47:03 PST
Graham: why do you no longer remove the editing style when contenteditable is set to false?
Comment 13 Graham Dennis 2006-02-22 04:14:00 PST
Created attachment 6662 [details]
patch 3

There was no reason why I wasn't removing the editing style when the contentEditable attribute was set to false. I've fixed that in this patch.
Previously, the editing style was forcibly removed, overriding any style that may have been applied (e.g. if a style of "word-wrap: break-word" was set on an element in the HTML, it would be removed if a contentEditable attribute was removed). In this patch, only the mapped attribute is removed, which means that if the element has an inline style of "word-wrap: break-word" it won't be overridden back to whatever the default is.
Comment 14 Justin Garcia 2006-02-24 14:11:14 PST
Comment on attachment 6662 [details]
patch 3

r=me, I'll land this.