Bug 7245 – caret does not display at the end of some lines in contenteditable divs

• Component: HTML Editing
• Status: RESOLVED FIXED

[Graham Dennis]

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.

[Graham Dennis]

The testcase for #6933 can also be used as a testcase for this bug.
http://bugzilla.opendarwin.org/attachment.cgi?id=6105

[Joost de Valk (AlthA)]

In the latest nightly from http://nightly.webkit.org/ this works good for me it seems.

[Graham Dennis]

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

[Graham Dennis]

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).

[Justin Garcia]

Looks good.  Graham: how did you test?

[Graham Dennis]

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'.

[Graham Dennis]

Created attachment 6579 [details]
fixed testcase

Test is now self-documenting

[Graham Dennis]

Created attachment 6580 [details]
fixed (x2) testcase

I forgot to remove unnecessary javascript

[Justin Garcia]

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.

[Justin Garcia]

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. 

[Graham Dennis]

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.

[Justin Garcia]

Graham: why do you no longer remove the editing style when contenteditable is set to false?

[Graham Dennis]

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.

[Justin Garcia]

Comment on attachment 6662 [details]
patch 3

r=me, I'll land this.