Bug 24527 - caret does not paint the first time you focus on a 0px right padding RTL div
Summary: caret does not paint the first time you focus on a 0px right padding RTL div
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: PC Windows 2000
: P2 Normal
Assignee: Xiaomei Ji
URL:
Keywords:
Depends on:
Blocks: 25228
  Show dependency treegraph
 
Reported: 2009-03-11 15:46 PDT by Xiaomei Ji
Modified: 2009-04-16 14:42 PDT (History)
6 users (show)

See Also:


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+
Details | Formatted Diff | Diff
patch w/ Layout test (version 2) (5.50 KB, patch)
2009-04-07 17:54 PDT, Xiaomei Ji
simon.fraser: review+
Details | Formatted Diff | Diff
patch w/ Layout test (version 3) (5.48 KB, patch)
2009-04-08 12:08 PDT, Xiaomei Ji
simon.fraser: review+
Details | Formatted Diff | Diff
patch w/ Layout test (version 4) (5.26 KB, patch)
2009-04-16 12:03 PDT, Xiaomei Ji
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 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.
Comment 1 Xiaomei Ji 2009-03-11 15:47:30 PDT
Created attachment 28498 [details]
test case
Comment 2 Xiaomei Ji 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".
Comment 3 Xiaomei Ji 2009-03-11 15:50:18 PDT
The related Chromium bug is:
http://code.google.com/p/chromium/issues/detail?id=2777

Comment 4 Xiaomei Ji 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.
Comment 5 Xiaomei Ji 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;
Comment 6 Simon Fraser (smfr) 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).
Comment 7 Xiaomei Ji 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).
> 

Comment 8 mitz 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.
Comment 9 Xiaomei Ji 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.
Comment 10 Xiaomei Ji 2009-04-07 17:56:49 PDT
BTW, I removed the diff of expected png file, since the diff does not make sense.
Comment 11 Simon Fraser (smfr) 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.
Comment 12 Xiaomei Ji 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
Comment 13 Justin Garcia 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.
Comment 14 Simon Fraser (smfr) 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.
Comment 15 Justin Garcia 2009-04-16 11:30:16 PDT
I'm going to land this.
Comment 16 Xiaomei Ji 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? 
Comment 17 Justin Garcia 2009-04-16 11:49:10 PDT
Sure thanks.
Comment 18 Xiaomei Ji 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
Comment 19 Darin Fisher (:fishd, Google) 2009-04-16 13:04:05 PDT
Landed as:  http://trac.webkit.org/changeset/42585
Comment 20 Justin Garcia 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.