Bug 25319

Summary: caret does not paint after type in characters in right aligned div or after delete all characters in RTL div or 0px right padding RTL textarea
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, justin.garcia, mitz, ojan, playmobil, progame+wk, simon.fraser, tony, webkit.review.bot, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
test case
none
an incomplete patch
none
patch w/ Layout test
none
patch w/ layout test
eric: review-
path /w layout test
none
path /w layout test
mitz: review+
fix test breaks QT Linux
eric: review-
patch skip caret-position.html in QT eric: review+

Description Xiaomei Ji 2009-04-21 17:15:04 PDT
Safari works fine for textarea probably because it's
textarea’s inner block always has horizontal padding.

Steps to reproduce:
1. open the attached test case
2. focus on 2nd input box, which is a RTL div, caret is painted. Then, type character 'a', caret painted at the left side of 'a', then, use backspace to delete 'a', caret is not painted anymore.
3. focus on 3rd input box, which is a right-aligned LTR div, caret is painted. Then, type character 'a', caret is not painted anymore.
Comment 1 Xiaomei Ji 2009-04-21 17:15:53 PDT
Created attachment 29671 [details]
test case
Comment 2 Xiaomei Ji 2009-04-21 17:20:28 PDT
The root cause probably is in:
void RenderBlock::computeHorizontalPositionsForLine(RootInlineBox* lineBox, bool firstLine, BidiRun* firstRun, BidiRun* trailingSpaceRun, bool reachedEnd)


    int x = leftOffset(height(), firstLine);
    switch(textAlign) {
        .....
        case RIGHT:
        case WEBKIT_RIGHT:
            // Wide lines spill out of the block based off direction.
            // So even if text-align is right, if direction is LTR, wide lines should overflow out of the right
            // side of the block.
            if (style()->direction() == LTR) {
                if (trailingSpaceRun) {
                    totWidth -= trailingSpaceRun->m_box->width();
                    trailingSpaceRun->m_box->setWidth(0);
                }
                if (totWidth < availableWidth)
                    x += availableWidth - totWidth;
            } else {
                if (totWidth > availableWidth && trailingSpaceRun) {
                    trailingSpaceRun->m_box->setWidth(max(0, trailingSpaceRun->m_box->width() - totWidth + availableWidth));
                    totWidth -= trailingSpaceRun->m_box->width();
                } else
                    x += availableWidth - totWidth;
            }
            break;


Both statement of "x += availableWidth - totWidth;" should be "x += availableWidth - totWidth - caretWidth;"
Comment 3 Xiaomei Ji 2009-04-21 17:22:51 PDT
related Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=10801
Comment 4 Xiaomei Ji 2009-05-22 11:12:57 PDT
I feel the patch is correct. But since RenderBlock::computeHorizontalPositionsForLine() is such a basic function, a lot of test results change.

I got ~200 differences on expected text files due to the 1-pixel shift (in DumpRenderTree, I guess). And ~200 pixel result diffs.

I am wondering is there any way to "work around" changing so many test files, which is kind of error prone? Should I include the png file change (at least the file names) in the patch? 

Look forward to your suggestions.

Thanks,
Xiaomei

Comment 5 Xiaomei Ji 2009-05-22 11:19:57 PDT
Created attachment 30583 [details]
an incomplete patch

I attach an incomplete patch just to show the changes in the test files.
Comment 6 Jeremy Moskovich 2009-05-22 11:22:08 PDT
ojan: can you provide any advice here?
Comment 7 mitz 2009-05-22 11:23:48 PDT
I don’t think it is correct to change layout just in order to make room for an insertion point.
Comment 8 Xiaomei Ji 2009-05-22 11:52:08 PDT
(In reply to comment #7)
> I don�t think it is correct to change layout just in order to make room for an
> insertion point.
> 

Maybe I did not understand what you meant.

The cause of the non-rendering-caret is similar to the cause of issue 24527 (https://bugs.webkit.org/show_bug.cgi?id=24527). It is because x-axis is 1-pixel shifted when alignment is right, so the boundary of the layout is not correct.

Or maybe it could be fixed in other layer?
Comment 9 Xiaomei Ji 2009-05-27 16:54:40 PDT
Created attachment 30720 [details]
patch w/ Layout test

I've excluded the content of the 2 PNG files from the patch except the file names.
Comment 10 Xiaomei Ji 2009-05-27 16:58:08 PDT
(In reply to comment #7)
> I don’t think it is correct to change layout just in order to make room for an
> insertion point.
> 

Hi Mitz,

Sorry that I did not understand what you meant until Ojan elaborated it for me.

I've uploaded a patch which fixes RenderText::localCaretRect(). Hope it is the right place to fix.

Thanks,
Xiaomei
Comment 11 Ojan Vafai 2009-06-01 02:20:41 PDT
Comment on attachment 30720 [details]
patch w/ Layout test

The fix seems reasonable to me (although, that if-statement is hard to read), but I'm not very familiar with this code and I'm not a webkit reviewer. :) I just had a couple testing comments.

> Index: LayoutTests/editing/deleting/caret-rtl-after-delete.html
> ===================================================================
> --- LayoutTests/editing/deleting/caret-rtl-after-delete.html	(revision 0)
> +++ LayoutTests/editing/deleting/caret-rtl-after-delete.html	(revision 0)
> +function runTest()
> +{
> +    var e = document.getElementById("test");
> +    e.focus();
> +    selectAllCommand();
> +    execDeleteCommand();

I think you should make this a dumpAsText test and just assert here that the caret is at a specific rect using textInputController.firstRectForCharacterRange(i, 0). We don't actually want or need pixel and rendertree results here, right? Also, that makes the expected behavior more clear if the test just asserts that the caret is at a specific location.

> +<div CONTENTEDITABLE id="test" style="direction: rtl; width:200px; font-size:2000%"; outline: solid thin; overflow: hidden>a</div> 

Is the font-size:2000% really necessary for this test?

> +</body>
> +<script> runTest(); </script>
> +</html>
> +
> Index: LayoutTests/editing/inserting/caret-right-align-after-insert.html
> ===================================================================
> --- LayoutTests/editing/inserting/caret-right-align-after-insert.html	(revision 0)
> +++ LayoutTests/editing/inserting/caret-right-align-after-insert.html	(revision 0)

Same comment as for the above test. Make it a dumpAsText.
Comment 12 Xiaomei Ji 2009-06-02 18:00:29 PDT
Created attachment 30887 [details]
patch w/ layout test

updated patch per Ojan's suggestion:
1. change the "if" to "switch" in RenderText.cpp for better readability
2. using dumpAsText() and textInputController.firstRectForCharacterRange(i, 0) for layout test
3. remove pixel tests which are not necessary, and remove "font-size:2000%" which is for pixel test only.
Comment 13 mitz 2009-06-04 10:41:33 PDT
Comment on attachment 30887 [details]
patch w/ layout test

Does the behavior implemented in this patch match any platform's native text system? I think it deviates from Mac OS X’s native behavior.
Comment 14 Xiaomei Ji 2009-06-04 15:40:26 PDT
(In reply to comment #13)
> (From update of attachment 30887 [details] [review])
> Does the behavior implemented in this patch match any platform's native text
> system? I think it deviates from Mac OS X’s native behavior.
> 

Do you mean how the caret width is distributed to left/right side?
I am not familiar with Mac OS X. How text system in Mac OS X handles it?
Comment 15 mitz 2009-06-04 16:07:50 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 30887 [details] [review] [review])
> > Does the behavior implemented in this patch match any platform's native text
> > system? I think it deviates from Mac OS X’s native behavior.
> > 
> 
> Do you mean how the caret width is distributed to left/right side?
> I am not familiar with Mac OS X. How text system in Mac OS X handles it?

As far as I can tell, in the Cocoa text system on Mac OS X, the caret (which is 1 pixel wide) is positioned to the right of the glyph, regardless of alignment and directionality.
Comment 16 Dave Hyatt 2009-06-16 10:53:38 PDT
Comment on attachment 30887 [details]
patch w/ layout test

Isn't the real issue here just that m_overflowWidth needs to include the caret so that it will be able to paint?  Can't we just make the caret part of the visual overflow?
Comment 17 Eric Seidel (no email) 2009-06-18 17:40:57 PDT
Comment on attachment 30887 [details]
patch w/ layout test

r- based on hyatt's above comments.  Please answer and either re-request review or post a revised patch.
Comment 18 Xiaomei Ji 2009-06-18 19:23:22 PDT
It is the computation of the local caret rect wrong.
The wrong computation is in RenderText::localCaretRect().

Hyatt commented through IRC that the "if" and the "else" block are both wrong. He sees 4 mistakes there, and he will need to figure out who wrote the code originally from trac (I guess to figure out the clamp part)

Following is from Hyatt through IRC:
it looks like localCaretRect is wanting to clamp inside the block in one case and the line in anoterh. but its math is wrong.
I do not particularly understand why you would clamp.
the entire notion of clamping seems misguided to me.
like.. if the offset happens to be outside of the block... so what.
maybe it is because of overflow::hidden to keep it from getting clipped.
not sure.
but the math isn't right anyway.
Comment 19 Eric Seidel (no email) 2009-06-18 19:27:09 PDT
You can figure out who wrote the code yourself if that's helpful:
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderText.cpp?annotate=blame&rev=43324
Comment 20 Xiaomei Ji 2010-04-15 18:17:55 PDT
Created attachment 53499 [details]
path /w layout test
Comment 21 mitz 2010-04-16 11:22:38 PDT
I think using cb->x() here is wrong. See bug 33503. This condition looks suspicious because of its asymmetry:
(cbStyle->direction() == RTL || cbStyle->textAlign() == RIGHT || cbStyle->textAlign() == WEBKIT_RIGHT)
it is not immediately clear why left-aligned RTL-text should fall into this case.
Comment 22 Xiaomei Ji 2010-04-16 17:52:45 PDT
(In reply to comment #21)
> I think using cb->x() here is wrong. See bug 33503. 

Hi Mitz,
Thanks for the quick review.

In the previous logic, when the style is autoWrap, the left position of caret is computed using the InlineTextBox's direction. For the test case in caret-rtl-2.html, the textBox is RTL, and the computation falls in:
left = max(left, cb-x());

I changed the logic to use containing block's alignment (same as no-wrap case). The test case's containing block is LTR, and the computation falls in:
left = min(left, cb->x() + cb->width() - caretWidthRightOffset);
left = max(left, rootleft);

I tried caret-rtl-2.html, the patch will place the cursor at the very left of the rtl text, which is the "Bad" scenario you refer.

>This condition looks
> suspicious because of its asymmetry:
> (cbStyle->direction() == RTL || cbStyle->textAlign() == RIGHT ||
> cbStyle->textAlign() == WEBKIT_RIGHT)
> it is not immediately clear why left-aligned RTL-text should fall into this
> case.

Yes, you are right.
I think it should be changed to the same logic of computing alignment in RenderBlock::localCaretRect():

    switch (currentStyle->textAlign()) {
        case TAAUTO:
        case JUSTIFY:
            if (currentStyle->direction() == RTL)
                alignment = alignRight;
            break;
        case LEFT:
        case WEBKIT_LEFT:
            break;
        case CENTER:
        case WEBKIT_CENTER:
            alignment = alignCenter;
            break;
        case RIGHT:
        case WEBKIT_RIGHT:
            alignment = alignRight;
            break;
    }
Comment 23 Xiaomei Ji 2010-04-19 09:47:31 PDT
Created attachment 53685 [details]
path /w layout test
Comment 24 Darin Adler 2010-04-20 15:16:15 PDT
Comment on attachment 53685 [details]
path /w layout test

> +        rightEdge = cb->x() + cb->width();

I think that instead of cb->x() + cb->width() you could write cb->frameRect().right(). It seems a little clearer to me.

I didn't review all the code, but I have two comments on the part I did read.

> +    switch (cbStyle->textAlign()) {
> +    case TAAUTO:
> +    case JUSTIFY:
> +        if (cbStyle->direction() == RTL)
> +            rightAligned = true;

I would write this as:

    rightAligned = cbStyle->direction() == RTL;

> +        break;
> +    case RIGHT:
> +    case WEBKIT_RIGHT:
> +        rightAligned = true;
> +        break;
> +    default:
> +        break;
> +    }

It's better style to list all the ETextAlign values here and leave out the default case. That way, if a new ETextAlign value is added later we will get a warning when compiling with GCC.

I didn't mark this review+ yet because I'm not entirely sure it's right. Mitz should look at this.
Comment 25 mitz 2010-04-21 18:37:19 PDT
Comment on attachment 53685 [details]
path /w layout test

r=me assuming you address Darin’s comments
Comment 26 Xiaomei Ji 2010-04-23 15:08:22 PDT
Committed r58191: <http://trac.webkit.org/changeset/58191>
Comment 27 WebKit Review Bot 2010-04-23 15:31:30 PDT
http://trac.webkit.org/changeset/58191 might have broken Qt Linux Release
Comment 28 Xiaomei Ji 2010-04-23 16:13:28 PDT
Created attachment 54199 [details]
fix test breaks QT Linux

obsolete the patch already committed.
Comment 29 Eric Seidel (no email) 2010-04-23 16:17:47 PDT
Comment on attachment 54199 [details]
fix test breaks QT Linux

This change looks wrong.  It's correct to check for textInputController since you're using it. :)

Does Qt support textInputController?  If not, we should just skip this test on Qt.
Comment 30 Xiaomei Ji 2010-04-23 16:41:20 PDT
Created attachment 54202 [details]
patch skip caret-position.html in QT
Comment 31 Xiaomei Ji 2010-04-23 16:51:05 PDT
Committed r58198: <http://trac.webkit.org/changeset/58198>
Comment 32 Tony Chang 2010-04-25 23:25:46 PDT
This change caused 12 tests to start failing pixel tests in Chromium.  Is this expected?

It looks like in most cases, it's just the cursor moving by a pixel.

  editing/deleting/4845371.html = IMAGE
  editing/deleting/5126166.html = IMAGE
  editing/deleting/5483370.html = IMAGE
  editing/deleting/table-cells.html = IMAGE
  editing/inserting/4875189-2.html = IMAGE
  editing/pasteboard/5387578.html = IMAGE
  editing/pasteboard/paste-4039777-fix.html = IMAGE
  editing/pasteboard/paste-table-001.html = IMAGE
  editing/pasteboard/paste-table-cells.html = IMAGE
  editing/selection/move-past-trailing-space.html = IMAGE
  fast/inline-block/14498-positionForCoordinates.html = IMAGE
  fast/repaint/caret-outside-block.html = IMAGE
Comment 33 Tony Chang 2010-04-25 23:37:00 PDT
The tests fail on webkit mac as well if you run with --tolerance=0.  I've opened bug 38104 to track this.