Bug 4003 - contentEditable div cannot be edited if it starts out with empty or <p/>
Summary: contentEditable div cannot be edited if it starts out with empty or <p/>
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: Justin Garcia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-14 12:21 PDT by Dan Wood
Modified: 2020-08-06 09:55 PDT (History)
4 users (show)

See Also:


Attachments
Simple test case showing inability to start editing div when it's empty (660 bytes, text/html)
2005-07-14 12:22 PDT, Dan Wood
no flags Details
editable div test: empty vs. white space (405 bytes, text/html)
2005-11-19 07:04 PST, Dan Wood
no flags Details
Patch for a contentEditable div containing only whitespace (893 bytes, patch)
2005-11-19 14:06 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 2 (2.16 KB, patch)
2005-12-01 02:53 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 3a (3.55 KB, patch)
2005-12-01 19:51 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 3b (2.54 KB, patch)
2005-12-01 19:51 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 Dan Wood 2005-07-14 12:21:22 PDT
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.
Comment 1 Dan Wood 2005-07-14 12:22:28 PDT
Created attachment 2965 [details]
Simple test case showing inability to start editing div when it's empty
Comment 2 David Storey 2005-07-19 14:52:32 PDT
confirmed on 10.3.9
Comment 3 Mark Rowe (bdash) 2005-08-07 05:36:31 PDT
Confirmed with ToT WebKit.
Comment 4 Justin Garcia 2005-11-17 22:57:36 PST
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(). 
Comment 5 Graham Dennis 2005-11-19 03:14:48 PST
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.
Comment 6 Dan Wood 2005-11-19 07:04:23 PST
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
Comment 7 Dan Wood 2005-11-19 07:09:08 PST
(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>

Comment 8 Graham Dennis 2005-11-19 14:06:31 PST
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.
Comment 9 Graham Dennis 2005-12-01 02:53:54 PST
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
Comment 10 Darin Adler 2005-12-01 10:10:32 PST
Comment on attachment 4892 [details]
patch 2

I think Justin should review this.
Comment 11 Justin Garcia 2005-12-01 13:21:56 PST
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.
Comment 12 Graham Dennis 2005-12-01 19:51:05 PST
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.
Comment 13 Graham Dennis 2005-12-01 19:51:39 PST
Created attachment 4901 [details]
patch 3b

Second proposed patch.
Comment 14 Justin Garcia 2005-12-02 01:02:20 PST
Comment on attachment 4901 [details]
patch 3b

Made some minor tweaks and landed this.