Bug 7319 - Can't see caret at the end of a contenteditable div with overflow:hidden
Summary: Can't see caret at the end of a contenteditable div with overflow:hidden
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Adele Peterson
URL:
Keywords: InRadar
Depends on:
Blocks: 6986
  Show dependency treegraph
 
Reported: 2006-02-16 20:29 PST by Adele Peterson
Modified: 2006-03-07 11:51 PST (History)
3 users (show)

See Also:


Attachments
test case (121 bytes, text/html)
2006-02-16 20:31 PST, Adele Peterson
no flags Details
patch (1.66 KB, patch)
2006-02-17 01:03 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
test case w/ editable div (123 bytes, text/html)
2006-03-06 11:27 PST, Adele Peterson
no flags Details
patch (1.38 KB, patch)
2006-03-07 11:02 PST, Adele Peterson
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2006-02-16 20:29:47 PST
See test case.  In the new text fields, the caret stops blinking when you arrow over to the right edge.
Comment 1 Adele Peterson 2006-02-16 20:31:58 PST
Created attachment 6555 [details]
test case
Comment 2 Adele Peterson 2006-02-17 01:03:26 PST
Created attachment 6559 [details]
patch

I'm not sure this is the right approach for the long term, but it solves this specific problem.  Maybe we want a render class for any renderer that has an anonymous DOM tree.
Comment 3 David Harrison 2006-02-17 10:51:07 PST
Agreed that this patch does not seem right.  Reason is that the loop you're changing is trying to skip what I'd call "intermediate" nodes, but I would not a call a text field intermediate.

Please elaborate on what is going wrong to cause this bug (apparently something to do with anon DOM?), then I'll try to help with longer term solution.  Thanks.
Comment 4 Adele Peterson 2006-02-17 11:19:23 PST
When the caret is at the end, and RenderBlock::paintCaret is called for the RenderTextField, the containing block for the text node is its parent div (which is anonymous), and not the RenderTextField, which is preventing paintCaret from actually painting.

At other caret positions, paintCaret isn't called for the RenderTextField-- only for the div, so the containingBlock matches the caller.  So maybe we should never be getting to the point where the RenderTextField is trying to draw the caret?
Comment 5 Adele Peterson 2006-03-01 17:02:34 PST
<rdar://problem/4463504> caret stops blinking when it reaches the end of the new text field (7319)
Comment 6 Adele Peterson 2006-03-06 11:26:50 PST
justin and i discussed the possibility that the div just needed more padding, but after experimenting, that doesn't seem to have any effect.

I was also able to reproduce the problem with a normal div.  I'll attach the test case.
Comment 7 Adele Peterson 2006-03-06 11:27:30 PST
Created attachment 6900 [details]
test case w/ editable div
Comment 8 Adele Peterson 2006-03-06 14:38:33 PST
One explanation is that the div's overflow layer is not large enough for the caret at the end.

RenderLayer::computeScrollDimensions uses the rightmostPosition to compute the scrollWidth.  rightmostPosition will compute a position based on the text nodes in the block.  If we add code to account for the possibility of a cursor, then the rightmostPosition would increase by 1 in this case.

Something like this in RenderBlock::rightmostPosition...

// If this node is a root editable element, then the rightmostPosition should account for a caret at the end.
if (node()->isContentEditable() && node() == node()->rootEditableElement())
    right += 1;

This does the trick, but I'm not sure its the right thing to do.
Comment 9 Adele Peterson 2006-03-07 11:02:42 PST
Created attachment 6921 [details]
patch

I don't think this addresses the rtl case- but my testing showed that there are other serious problems with the caret displaying correctly for rtl editable areas, which makes it difficult to test this specific case.
Comment 10 Dave Hyatt 2006-03-07 11:47:20 PST
Comment on attachment 6921 [details]
patch

You should add style()->direction() == LTR as well.

r=me