Bug 24527

Summary: caret does not paint the first time you focus on a 0px right padding RTL div
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: HTML EditingAssignee: Xiaomei Ji <xji>
Status: RESOLVED FIXED    
Severity: Normal CC: justin.garcia, mitz, playmobil, progame+wk, simon.fraser, xji
Priority: P2    
Version: 525.x (Safari 3.2)   
Hardware: PC   
OS: Windows 2000   
Bug Depends on:    
Bug Blocks: 25228    
Attachments:
Description Flags
test case
none
patch w/ Layout test
simon.fraser: review+
patch w/ Layout test (version 2)
simon.fraser: review+
patch w/ Layout test (version 3)
simon.fraser: review+
patch w/ Layout test (version 4) simon.fraser: review+

Xiaomei Ji
Reported 2009-03-11 15:46:47 PDT
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.
Attachments
test case (272 bytes, text/html)
2009-03-11 15:47 PDT, Xiaomei Ji 		no flags
Details
patch w/ Layout test (5.11 KB, patch)
2009-03-17 18:17 PDT, Xiaomei Ji 		simon.fraser: review+
DetailsFormatted DiffDiff
patch w/ Layout test (version 2) (5.50 KB, patch)
2009-04-07 17:54 PDT, Xiaomei Ji 		simon.fraser: review+
DetailsFormatted DiffDiff
patch w/ Layout test (version 3) (5.48 KB, patch)
2009-04-08 12:08 PDT, Xiaomei Ji 		simon.fraser: review+
DetailsFormatted DiffDiff
patch w/ Layout test (version 4) (5.26 KB, patch)
2009-04-16 12:03 PDT, Xiaomei Ji 		simon.fraser: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Xiaomei Ji
Comment 1 2009-03-11 15:47:30 PDT
Created attachment 28498 [details] test case
Xiaomei Ji
Comment 2 2009-03-11 15:49:03 PDT
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".
Xiaomei Ji
Comment 3 2009-03-11 15:50:18 PDT
The related Chromium bug is: http://code.google.com/p/chromium/issues/detail?id=2777
Xiaomei Ji
Comment 4 2009-03-17 18:17:47 PDT
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.
Xiaomei Ji
Comment 5 2009-04-07 14:02:13 PDT
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;
Simon Fraser (smfr)
Comment 6 2009-04-07 14:15:39 PDT
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).
Xiaomei Ji
Comment 7 2009-04-07 14:36:21 PDT
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). >
mitz
Comment 8 2009-04-07 15:03:56 PDT
(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.
Xiaomei Ji
Comment 9 2009-04-07 17:54:58 PDT
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.
Xiaomei Ji
Comment 10 2009-04-07 17:56:49 PDT
BTW, I removed the diff of expected png file, since the diff does not make sense.
Simon Fraser (smfr)
Comment 11 2009-04-07 17:59:01 PDT
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.
Xiaomei Ji
Comment 12 2009-04-08 12:08:35 PDT
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
Justin Garcia
Comment 13 2009-04-15 18:00:07 PDT
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.
Simon Fraser (smfr)
Comment 14 2009-04-15 22:28:53 PDT
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.
Justin Garcia
Comment 15 2009-04-16 11:30:16 PDT
I'm going to land this.
Xiaomei Ji
Comment 16 2009-04-16 11:48:30 PDT
(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?
Justin Garcia
Comment 17 2009-04-16 11:49:10 PDT
Sure thanks.
Xiaomei Ji
Comment 18 2009-04-16 12:03:57 PDT
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
Darin Fisher (:fishd, Google)
Comment 19 2009-04-16 13:04:05 PDT
Landed as: http://trac.webkit.org/changeset/42585
Justin Garcia
Comment 20 2009-04-16 14:42:50 PDT
I think we have the same problem if the block contains a placeholder br. Will try and construct a test case.
Note You need to log in before you can comment on or make changes to this bug.