Bug 25319 – 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

• Component: HTML Editing
• Status: RESOLVED FIXED

[Xiaomei Ji]

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.

[Xiaomei Ji]

Created attachment 29671 [details]
test case

[Xiaomei Ji]

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;"

[Xiaomei Ji]

related Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=10801

[Xiaomei Ji]

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

[Xiaomei Ji]

Created attachment 30583 [details]
an incomplete patch

I attach an incomplete patch just to show the changes in the test files.

[Jeremy Moskovich]

ojan: can you provide any advice here?

I don’t think it is correct to change layout just in order to make room for an insertion point.

[Xiaomei Ji]

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

[Xiaomei Ji]

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.

[Xiaomei Ji]

(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

[Ojan Vafai]

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.

[Xiaomei Ji]

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

[Xiaomei Ji]

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

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

[Dave Hyatt]

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?

[Eric Seidel (no email)]

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.

[Xiaomei Ji]

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.

[Eric Seidel (no email)]

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

[Xiaomei Ji]

Created attachment 53499 [details]
path /w layout test

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.

[Xiaomei Ji]

(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;
    }

[Xiaomei Ji]

Created attachment 53685 [details]
path /w layout test

[Darin Adler]

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 on attachment 53685 [details]
path /w layout test

r=me assuming you address Darin’s comments

[Xiaomei Ji]

Committed r58191: <http://trac.webkit.org/changeset/58191>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58191 might have broken Qt Linux Release

[Xiaomei Ji]

Created attachment 54199 [details]
fix test breaks QT Linux

obsolete the patch already committed.

[Eric Seidel (no email)]

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.

[Xiaomei Ji]

Created attachment 54202 [details]
patch skip caret-position.html in QT

[Xiaomei Ji]

Committed r58198: <http://trac.webkit.org/changeset/58198>

[Tony Chang]

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

[Tony Chang]

The tests fail on webkit mac as well if you run with --tolerance=0.  I've opened bug 38104 to track this.