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 Editing | Assignee: | 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
Xiaomei Ji
2009-04-21 17:15:04 PDT
Created attachment 29671 [details]
test case
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;" related Chromium bug: http://code.google.com/p/chromium/issues/detail?id=10801 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 Created attachment 30583 [details]
an incomplete patch
I attach an incomplete patch just to show the changes in the test files.
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. (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? 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.
(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 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. 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.
(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. 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 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.
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. 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 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. (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; } Created attachment 53685 [details]
path /w layout test
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
Committed r58191: <http://trac.webkit.org/changeset/58191> http://trac.webkit.org/changeset/58191 might have broken Qt Linux Release Created attachment 54199 [details]
fix test breaks QT Linux
obsolete the patch already committed.
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.
Created attachment 54202 [details]
patch skip caret-position.html in QT
Committed r58198: <http://trac.webkit.org/changeset/58198> 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 The tests fail on webkit mac as well if you run with --tolerance=0. I've opened bug 38104 to track this. |