WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
https://bugs.webkit.org/show_bug.cgi?id=25319
Summary
caret does not paint after type in characters in right aligned div or after d...
Xiaomei Ji
Reported
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.
Attachments
test case
(364 bytes, text/html)
2009-04-21 17:15 PDT
,
Xiaomei Ji
no flags
Details
an incomplete patch
(12.66 KB, patch)
2009-05-22 11:19 PDT
,
Xiaomei Ji
no flags
Details
Formatted Diff
Diff
patch w/ Layout test
(15.40 KB, patch)
2009-05-27 16:54 PDT
,
Xiaomei Ji
no flags
Details
Formatted Diff
Diff
patch w/ layout test
(10.46 KB, patch)
2009-06-02 18:00 PDT
,
Xiaomei Ji
eric
: review-
Details
Formatted Diff
Diff
path /w layout test
(8.41 KB, patch)
2010-04-15 18:17 PDT
,
Xiaomei Ji
no flags
Details
Formatted Diff
Diff
path /w layout test
(8.64 KB, patch)
2010-04-19 09:47 PDT
,
Xiaomei Ji
mitz: review+
Details
Formatted Diff
Diff
fix test breaks QT Linux
(1.42 KB, patch)
2010-04-23 16:13 PDT
,
Xiaomei Ji
eric
: review-
Details
Formatted Diff
Diff
patch skip caret-position.html in QT
(1.18 KB, patch)
2010-04-23 16:41 PDT
,
Xiaomei Ji
eric
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Xiaomei Ji
Comment 1
2009-04-21 17:15:53 PDT
Created
attachment 29671
[details]
test case
Xiaomei Ji
Comment 2
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;"
Xiaomei Ji
Comment 3
2009-04-21 17:22:51 PDT
related Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=10801
Xiaomei Ji
Comment 4
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
Xiaomei Ji
Comment 5
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.
Jeremy Moskovich
Comment 6
2009-05-22 11:22:08 PDT
ojan: can you provide any advice here?
mitz
Comment 7
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.
Xiaomei Ji
Comment 8
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?
Xiaomei Ji
Comment 9
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.
Xiaomei Ji
Comment 10
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
Ojan Vafai
Comment 11
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.
Xiaomei Ji
Comment 12
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.
mitz
Comment 13
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.
Xiaomei Ji
Comment 14
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?
mitz
Comment 15
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.
Dave Hyatt
Comment 16
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?
Eric Seidel (no email)
Comment 17
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.
Xiaomei Ji
Comment 18
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.
Eric Seidel (no email)
Comment 19
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
Xiaomei Ji
Comment 20
2010-04-15 18:17:55 PDT
Created
attachment 53499
[details]
path /w layout test
mitz
Comment 21
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.
Xiaomei Ji
Comment 22
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; }
Xiaomei Ji
Comment 23
2010-04-19 09:47:31 PDT
Created
attachment 53685
[details]
path /w layout test
Darin Adler
Comment 24
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.
mitz
Comment 25
2010-04-21 18:37:19 PDT
Comment on
attachment 53685
[details]
path /w layout test r=me assuming you address Darin’s comments
Xiaomei Ji
Comment 26
2010-04-23 15:08:22 PDT
Committed
r58191
: <
http://trac.webkit.org/changeset/58191
>
WebKit Review Bot
Comment 27
2010-04-23 15:31:30 PDT
http://trac.webkit.org/changeset/58191
might have broken Qt Linux Release
Xiaomei Ji
Comment 28
2010-04-23 16:13:28 PDT
Created
attachment 54199
[details]
fix test breaks QT Linux obsolete the patch already committed.
Eric Seidel (no email)
Comment 29
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.
Xiaomei Ji
Comment 30
2010-04-23 16:41:20 PDT
Created
attachment 54202
[details]
patch skip caret-position.html in QT
Xiaomei Ji
Comment 31
2010-04-23 16:51:05 PDT
Committed
r58198
: <
http://trac.webkit.org/changeset/58198
>
Tony Chang
Comment 32
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
Tony Chang
Comment 33
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug