Bug 89649 – Caret position is wrong when a editable container has word-wrap:normal set

• Component: HTML Editing
• Status: RESOLVED FIXED

[Pravin D]

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.

[Pravin D]

Created attachment 148996 [details]
Patch

[Ryosuke Niwa]

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?

[Pravin D]

Created attachment 149086 [details]
Patch

[Pravin D]

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

[Ryosuke Niwa]

As much as I'd like to fix this bug, I don't think I'm qualified to review this patch.

[Eric Seidel (no email)]

xji was the last to edit these lines. http://trac.webkit.org/changeset/84659

[Robert Hogan]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Please attach a test case so others can see the bug. :)  I tried to repro using your steps and couldn't.

[Pravin D]

Created attachment 151112 [details]
TestCase

The test case contains valid and invalid edit area...

[Pravin D]

(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 ??

[Ryosuke Niwa]

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

[Pravin D]

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

[Pravin D]

Created attachment 151437 [details]
Patch

[Pravin D]

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

[Ryosuke Niwa]

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.

[Pravin D]

Created attachment 151919 [details]
Patch

[Pravin D]

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

[Pravin D]

(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

[Levi Weintraub]

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?

[Pravin D]

Created attachment 155512 [details]
Patch

[Levi Weintraub]

Comment on attachment 155512 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 155512 [details]
Patch

Clearing flags on attachment: 155512

Committed r124231: <http://trac.webkit.org/changeset/124231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Pravin D]

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

[Pravin D]

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

[Pravin D]

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

[Ryosuke Niwa]

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');