Bug 4003 – contentEditable div cannot be edited if it starts out with empty or <p/>

• Component: HTML Editing
• Status: RESOLVED FIXED

[Dan Wood]

If you have a div that is marked contentEditable, but it does not contain some visible characters in it -- 
e.g. it's empty of just <p /> in it -- then you can't actually get an insertion point to edit text in the field

Steps to reproduce:

See attached test case.

[Dan Wood]

Created attachment 2965 [details]
Simple test case showing inability to start editing div when it's empty

[David Storey]

confirmed on 10.3.9

[Mark Rowe (bdash)]

Confirmed with ToT WebKit.

[Justin Garcia]

The VisiblePosition created and returned in positionForCoordinates is outside the editable div that was 
clicked.  I thought that this bug would be fixed by my patch for 5354 but it isn't. 

In the test case, the div has a renderer and height of 22, and has rendered child, the p.  The p doesn't 
have a height.  Neither [p, 0] or [div, 0] return true from isCandidate(), so visibleposition::init leaves the div 
and uses the next visibleposition, which is the start of the next paragraph.  I think the fix should be in 
isCandidate(). 

[Graham Dennis]

What exactly is correct behaviour for the second case where there is a <p /> in the div? Should the 
insertion point be placed before the <p />, in the <p></p>, or after the <p />? 

I've had a bit of a play with this, but I'm not sure which is right. So far, I've got the first two cases to work, 
but with graphics bugs.

[Dan Wood]

Created attachment 4732 [details]
editable div test: empty vs. white space

Additional test case contrasting an editable div that is truly empty with an
editable div that has some whitespace in it

[Dan Wood]

(In reply to comment #5)

A good question that I don't have an answer to.  My main concern in this bug report is how to edit a div 
when just an empty paragraph or nbsp is in it (which, after some more investigation, is also a problem 
if you have *any* "blank" content such as a space or newline in your source).

There was some discussion on this on #webkit yesterday, mostly about whether you can get a carat if 
the element has no dimensions.  With an empty <p />, it has no dimensions -- though the enclosing 
<div> does.  I noticed that if you just specify a div to be contentEditable,  it seems to pick up some 
automatic min-height behavior (see my second attachment for an example, vs. my first attachment, 
where I specified explicit min-heights).

Clearly the tricky case is what to do if you have a no-dimension entity like the middle paragraph here: 
<p>first paragraph</p> <p /> <p>third paragraph</p>

[Graham Dennis]

Created attachment 4735 [details]
Patch for a contentEditable div containing only whitespace

This patch builds on the one for 5354, and for a div with no renderable
children (contains only whitespace, no <p />, etc), puts the insertion pointer
at the start of the div (before the #text element).
I don't know, but it might be preferable to set the insertion pointer in the
#text element.

[Graham Dennis]

Created attachment 4892 [details]
patch 2

This sets the cursor at the div node for both the whitespace and <p />
situations. The latter involves a rendering bug that appears to be a regression
from the WebKit in 10.4.3

[Darin Adler]

Comment on attachment 4892 [details]
patch 2

I think Justin should review this.

[Justin Garcia]

Comment on attachment 4892 [details]
patch 2

This patch is great!  Here are some comments:

Since the next/prevIsInOriginalBlock booleans now include a check for identity
with the originalBlock, it no longer makes sense for them to be named "IsIn":
in what sense is A "in" A?  Instead, you could do:

bool nextIsOutsideOriginalBlock = !next.node()->isAncestor(originalBlock) &&
next.node() != originalBlock
bool prevIsOutsideOriginalBlock = !prev.node()->isAncestor(originalBlock) &&
prev.node() != originalBlock

if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock &&
... etc.

Also, I would put hasRenderedChildrenWithHeight into a function, it would make
isCandidate a little easier to read.

[Graham Dennis]

Created attachment 4900 [details]
patch 3a

Addressing comments. An additional function was added to RenderObject,
hasChildrenWithHeight.
I'll add another patch that doesn't modify RenderObject if adding a member
function is a Bad Thing.

[Graham Dennis]

Created attachment 4901 [details]
patch 3b

Second proposed patch.

[Justin Garcia]

Comment on attachment 4901 [details]
patch 3b

Made some minor tweaks and landed this.