Test case attached. For an RTL div with 0px right padding, the first time you focus on it, caret does not paint. The reason of caret not painted is because in SelectionController::paintCaret(), after computing "IntRect caret = intersection(drawingRect, clipRect);", the "caret" is empty() when the right padding is "0px". It is because the x-axis of "drawingRect" is 1 pixel shifted wrongly. The 1-pixel shift of "drawingRect" is because the x-axis of the local caret rect is computed wrongly in RenderBlock::localCaretRect(). There is the following line in RenderBlock::localCaretRect(): case alignRight: x = w - (borderRight() + paddingRight()); It should be case alignRight: x = w - (borderRight() + paddingRight()) - 1; So, for a div with width equals to 150, the x-axis when align right should be 149, not 150. I will provide patch and layout test after I get Mac environment. Or if anyone saw this bug and want to go ahead with the patch, you are very well welcomed.
Created attachment 28498 [details] test case
In the attached example, Safari works fine for textarea. Following is the explanation from Mitz "I think in Mac OS X WebKit the textarea’s inner block always has horizontal padding".
The related Chromium bug is: http://code.google.com/p/chromium/issues/detail?id=2777
Created attachment 28713 [details] patch w/ Layout test Did not figure out a better way for the layout test. The current layout test does not show difference before and after the fix. And the pixel test does not show difference except the pixel hash difference.
Another comment about the fix: I am not sure whether the fix should be: x = w - (borderRight() + paddingRight()) - 1; or x = w - (borderRight() + paddingRight()) - caretWidth;
Comment on attachment 28713 [details] patch w/ Layout test r=me, but please put more explanation in the Changelog. I also think that caretWidth is correct (you should temporarily change caretWidth to test).
Thanks for the review. I will upload another patch with more explanation in ChangeLog and change the fix to caretWidth. BTW, could you please explain more on how to temporarily change caretWidth to test? (In reply to comment #6) > (From update of attachment 28713 [details] [review]) > r=me, but please put more explanation in the Changelog. I also think that > caretWidth is correct (you should temporarily change caretWidth to test). >
(In reply to comment #7) > BTW, could you please explain more on how to temporarily change caretWidth to > test? You will need to change it in RenderObject.h, rebuild (it will take a while), and test the behavior manually.
Created attachment 29321 [details] patch w/ Layout test (version 2) Hi Simon, Could you kindly give it a review again? Following are the changes comparing with first version patch. 1. add explanation in ChangLog. 2. changed the x-axis to be minus by caretWidth. Tested in Safari using caretWidth=10, and yes, the x-axis should be minus by caretWidth, not by 1. Thanks mitz to point out the right place.
BTW, I removed the diff of expected png file, since the diff does not make sense.
Comment on attachment 29321 [details] patch w/ Layout test (version 2) > Index: LayoutTests/editing/selection/caret-rtl-3.html > =================================================================== > --- LayoutTests/editing/selection/caret-rtl-3.html (revision 0) > +++ LayoutTests/editing/selection/caret-rtl-3.html (revision 0) > @@ -0,0 +1,35 @@ > +<html> > +<head> > +<script> > +if (window.layoutTestController) > + layoutTestController.dumpEditingCallbacks(); > +</script> > + > +<script> No need to close and re-open the script tag. If you wanted the pixel test to make the result more obvious, you could make the text really big (font-size: 400%) so the caret is larger.
Created attachment 29341 [details] patch w/ Layout test (version 3) Hi Simon, Thanks for your review and pointing the font-size change to make pixel test difference. Mitz also pointed to enlarge font-size before, but I did not try to enlarge it extremely (font-size:1000% wont work). The changes against version 2 patch are all in test part: 1. remove </script><script> in testing html file 2. increase font-size to "font-size:2000%" to show pixel test difference. Could you please review it again? Thanks! Xiaomei
I ran into this bug while debugging another problem. I'm glad you're fixing it! > the x-axis should be minused by the caretWidth. Kind of confusing and doesn't really explain the change. We need to subtract the caretWidth so that the caret at IntRect(x, y, caretWidth, height) is actually inside the block. Mitz had some concerns about the layout test, I'll let him review those.
Comment on attachment 29341 [details] patch w/ Layout test (version 3) r=me, but the test could be improved: * it's not really about editing, so it should live in fast/forms? * the mouseDown()/mouseUp() could just use focus(), which works outside of DRT * increasing the contrast by removing the red border should make the pixel test more sensitive * the test would be even better using outline (so there's no focus ring), and setting overflow: so that in the failing case the caret does not draw at all.
I'm going to land this.
(In reply to comment #15) > I'm going to land this. > I am uploading another patch which incorporate yours and Simon's suggestion. Do you want to wait for that?
Sure thanks.
Created attachment 29543 [details] patch w/ Layout test (version 4) Hi Simon, I've updated the patch to incorporate yours and Justin's suggestions. Could you please review it again? Thanks, Xiaomei
Landed as: http://trac.webkit.org/changeset/42585
I think we have the same problem if the block contains a placeholder br. Will try and construct a test case.