Bug 89649 - Caret position is wrong when a editable container has word-wrap:normal set
Summary: Caret position is wrong when a editable container has word-wrap:normal set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pravin D
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-21 04:00 PDT by Pravin D
Modified: 2012-08-06 07:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2012-06-22 04:45 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (5.14 KB, patch)
2012-06-22 12:41 PDT, Pravin D
no flags Details | Formatted Diff | Diff
TestCase (1.37 KB, text/html)
2012-07-06 12:54 PDT, Pravin D
no flags Details
Patch (6.57 KB, patch)
2012-07-10 04:28 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2012-07-12 05:27 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (6.08 KB, patch)
2012-07-31 06:04 PDT, Pravin D
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pravin D 2012-06-21 04:00:31 PDT
Issue : The position of the caret when we type in a text that goes beyond the visual limit of a editable container which has style="word-wrap:break-word"


Step to reproduce:

<div contenteditable="true" style="height:50px;overflow:auto;width:500px;word-wrap:break-word;">
SOMEFILLERTEXTSOMEFILLERTEXTSOMEFILLERTEXTSOMEFILLERTEXTSOMEFILLERTEXT
</div>

1) Open the code snippet in a browser.
2) Scroll to the end of the text using the horizontal scroll bar.
3) Press the END key and start typing. Observe the behavior.
4) Also in the above case (3) space characters cannot be inserted at the end.
Comment 1 Pravin D 2012-06-22 04:45:03 PDT
Created attachment 148996 [details]
Patch
Comment 2 Ryosuke Niwa 2012-06-22 09:17:05 PDT
Comment on attachment 148996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148996&action=review

> Source/WebCore/ChangeLog:9
> +        This done taking into account that no word will spill out of the visible area. However if we have a long enough word and 

Nit: this is done?

> Source/WebCore/ChangeLog:11
> +        area internally the edit location will be set properly but the caret will not be rendered properly due to the above constraint.

What do you mean by "internally"?

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:14
> +    if (window.layoutTestController)
> +        layoutTestController.waitUntilDone();

Do we really need this?
Comment 3 Pravin D 2012-06-22 12:41:30 PDT
Created attachment 149086 [details]
Patch
Comment 4 Pravin D 2012-06-22 12:43:26 PDT
(In reply to comment #2)
> (From update of attachment 148996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148996&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This done taking into account that no word will spill out of the visible area. However if we have a long enough word and 
> 
> Nit: this is done?
> 
> > Source/WebCore/ChangeLog:11
> > +        area internally the edit location will be set properly but the caret will not be rendered properly due to the above constraint.
> 
> What do you mean by "internally"?
> 

Changed the description altogether...

> > LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:14
> > +    if (window.layoutTestController)
> > +        layoutTestController.waitUntilDone();
> 
> Do we really need this?
Removed waitUntilDone()...
Comment 5 Ryosuke Niwa 2012-06-25 09:54:47 PDT
As much as I'd like to fix this bug, I don't think I'm qualified to review this patch.
Comment 6 Eric Seidel (no email) 2012-06-26 10:43:28 PDT
xji was the last to edit these lines. http://trac.webkit.org/changeset/84659
Comment 7 Robert Hogan 2012-07-03 10:51:48 PDT
Comment on attachment 149086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149086&action=review

> LayoutTests/ChangeLog:9
> +        * editing/input/editable-container-with-word-wrap-normal-expected.html: Added.
> +        * editing/input/editable-container-with-word-wrap-normal.html: Added.

A reftest isn't appropriate here. You need to run this as a dumpAsText() test and log the position of the caret.
Comment 8 Eric Seidel (no email) 2012-07-03 11:23:02 PDT
Is this the bug where you can't type a space when the text fills exactly to the end of the line? (like happens often in bugzilla)?
Comment 9 Eric Seidel (no email) 2012-07-03 11:23:23 PDT
Please attach a test case so others can see the bug. :)  I tried to repro using your steps and couldn't.
Comment 10 Pravin D 2012-07-06 12:54:44 PDT
Created attachment 151112 [details]
TestCase

The test case contains valid and invalid edit area...
Comment 11 Pravin D 2012-07-06 14:32:49 PDT
(In reply to comment #7)
> (From update of attachment 149086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149086&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * editing/input/editable-container-with-word-wrap-normal-expected.html: Added.
> > +        * editing/input/editable-container-with-word-wrap-normal.html: Added.
> 
> A reftest isn't appropriate here. You need to run this as a dumpAsText() test and log the position of the caret.
> 

Im not sure how take only the position of the caret.
Firstly the test contains text and will become platform dependent. Second, the caret is not drawn at a proper location and also scrolling does not happen. However as the selection offset is working properly, cannot use JS to locate the position of caret.

So is it ok if I make the text case a non-ref test case and dump the render tree as text ??
Comment 12 Ryosuke Niwa 2012-07-06 19:51:52 PDT
Comment on attachment 149086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149086&action=review

>>> LayoutTests/ChangeLog:9
>>> +        * editing/input/editable-container-with-word-wrap-normal.html: Added.
>> 
>> A reftest isn't appropriate here. You need to run this as a dumpAsText() test and log the position of the caret.
> 
> Im not sure how take only the position of the caret.
> Firstly the test contains text and will become platform dependent. Second, the caret is not drawn at a proper location and also scrolling does not happen. However as the selection offset is working properly, cannot use JS to locate the position of caret.
> 
> So is it ok if I make the text case a non-ref test case and dump the render tree as text ??

Use Ahem font and internals.absoluteCaretBounds
See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Comment 13 Pravin D 2012-07-09 10:54:19 PDT
(In reply to comment #12)
> Use Ahem font and internals.absoluteCaretBounds
> See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

> 

Thanks for the link I'll check it...
Comment 14 Pravin D 2012-07-10 04:28:55 PDT
Created attachment 151437 [details]
Patch
Comment 15 Pravin D 2012-07-10 04:32:18 PDT
(In reply to comment #14)
> Created an attachment (id=151437) [details]
> Patch
> 

Made test case to use dumpAsText() as suggested by Eric ,
using Ahem fonts and used caret rect to varify the issue, as suggested by Rniwa...
Comment 16 Ryosuke Niwa 2012-07-10 20:09:46 PDT
Comment on attachment 151437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151437&action=review

> Source/WebCore/ChangeLog:13
> +        The allowed min and max values for the left position of the caret was constrained to the bounding rect of the container which had auto wrap set.
> +        The constraint works as long as there is no scrollable content. If we have a long enough word and word-wrap:normal then there will be some scrollable 
> +        content. In this case if we try to reach the end of the text(using 'END' key), scroll does not happen as scrolling is dependent on the location of the  
> +        caret rect which is constrained within the bounding rect of the container. 
> +        The caret rect has to be correctly maintained within the container's bounding rect for it to visible. This is currently calculated properly at a later point when
> +        there is no pending scrolls.

These are really long lines. Please make each line shorter (120-150).

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:24
> + startCaretRect = null;
> + finalCaretRect = null;
> + editableContainer = null;
> + caretWidth = null;
> + function runTest() {

Why is this script indented by 1 space? I'd prefer not having any indentation here.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:26
> +    // Run test case.

Please remove this comment. It doesn't add any value.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:31
> +    if (window.internals) {
> +        startCaretRect = internals.absoluteCaretBounds(document);
> +    }

No { } around single statements.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:38
> +    if (window.internals) {
> +        finalCaretRect = internals.absoluteCaretBounds(document);
> +    }

Ditto.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:42
> +    // The content must scroll for the caret to reach the end of the editable text.

This should be in debug().

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:49
> +    // The caret must be at the end of the text.
> +    // The final caret rect is always calculated wrt to the bounding rect of its container.
> +    // Using the below constrain to find the final position of the caret
> +    // 1) scrollWidth = text content width + caret width,
> +    // 2) caret rect is always within container bounding box (thus substracting the scroll left)

This should be in debug() but it should be cut down to a much shorter sentence. It's too verbose as is.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:52
> +    // Clean up

Another useless comment.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:59
> + <div  id="test" contenteditable="true" class="editableDiv" >

Nit: another one space indentation & two spaces between div & id.
Comment 17 Pravin D 2012-07-12 05:27:17 PDT
Created attachment 151919 [details]
Patch
Comment 18 Pravin D 2012-07-12 05:30:11 PDT
(In reply to comment #17)
> Created an attachment (id=151919) [details]
> Patch
> 

Uploaded a new patch with the changes suggested by Rniwa in Comment #16 .
Comment 19 Pravin D 2012-07-12 05:32:03 PDT
(In reply to comment #16)
> (From update of attachment 151437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151437&action=review
> 

Have uploaded a patch with the suggested changes. Also wanted know your opinion on the fix itself... thank you
Comment 20 Levi Weintraub 2012-07-30 14:50:38 PDT
Comment on attachment 151919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151919&action=review

The code and test look good to me. I'd be happy to get this in with a more succinct ChangeLog entry and a link to the bug in the test output text :)

> Source/WebCore/ChangeLog:14
> +        The allowed min and max values for the left position of the caret was constrained to the bounding rect 
> +        of the container which had auto wrap set. The constraint works as long as there is no scrollable content.
> +        If we have a long enough word and word-wrap:normal then there will be some scrollable content. In this 
> +        case, if we try to reach the end of the text(using 'END' key), scroll does not happen as scrolling is 
> +        dependent on the location of the caret rect which is constrained within the bounding rect of the container. 
> +        The caret rect has to be correctly maintained within the container's bounding rect for it to visible.
> +        This is currently calculated properly at a later point when there is no pending scrolls. 

Whoa this is really wordy!

"rect for it to visible." is missing a 'be.'

"when there is no pending scrolls" s/is/are.

> Source/WebCore/rendering/RenderText.cpp:683
> +    leftEdge = min(static_cast<float>(0), rootLeft);
> +    rightEdge = max(static_cast<float>(cb->logicalWidth()), rootRight);

Nit: min<float> and max<float> would be shorter.

> LayoutTests/editing/input/editable-container-with-word-wrap-normal-expected.txt:1
> +The test case checks if caret is drawn properly(especially scrolls properly) inside a editable container having word-wrap:normal.

How about adding the url for the bug?
Comment 21 Pravin D 2012-07-31 06:04:09 PDT
Created attachment 155512 [details]
Patch
Comment 22 Levi Weintraub 2012-07-31 10:47:14 PDT
Comment on attachment 155512 [details]
Patch

Ok.
Comment 23 WebKit Review Bot 2012-07-31 11:56:43 PDT
Comment on attachment 155512 [details]
Patch

Clearing flags on attachment: 155512

Committed r124231: <http://trac.webkit.org/changeset/124231>
Comment 24 WebKit Review Bot 2012-07-31 11:56:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Pravin D 2012-08-04 21:27:16 PDT
(In reply to comment #25)
> It appears that the test added by this patch has been failing on Mac:
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r124708%20(1598)/editing/input/editable-container-with-word-wrap-normal-pretty-diff.html
> 

The scrollLeft seems to be wrong on MAC(conclusion from test failure).
Need to check...
Comment 27 Pravin D 2012-08-05 08:49:38 PDT
(In reply to comment #25)
> It appears that the test added by this patch has been failing on Mac:
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r124708%20(1598)/editing/input/editable-container-with-word-wrap-normal-pretty-diff.html

> 

Checked the issue on Mac Lion r124713 release build. The issue seems to be fixed but the test case is failing...  Either 'END' key is not behaving properly on Mac EventSender or we need to put some sort of timeout/wait for the event to occur. 
I'll try n update this by tom.
Comment 28 Pravin D 2012-08-05 08:49:57 PDT
(In reply to comment #25)
> It appears that the test added by this patch has been failing on Mac:
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r124708%20(1598)/editing/input/editable-container-with-word-wrap-normal-pretty-diff.html

> 

Checked the issue on Mac Lion r124713 release build. The issue seems to be fixed but the test case is failing...  Either 'END' key is not behaving properly on Mac EventSender or we need to put some sort of timeout/wait for the event to occur. 
I'll try n update this by tom.
Comment 29 Ryosuke Niwa 2012-08-06 07:45:53 PDT
Comment on attachment 155512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155512&action=review

> LayoutTests/editing/input/editable-container-with-word-wrap-normal.html:35
> +    if (window.eventSender)
> +        eventSender.keyDown('end');

"end" key doesn't do what you expect it to do on Mac :(
What you want is getSelection().modify('move', 'forward', 'lineboundary');