RTL: Caret can paint over letters when in the end of a line in textareas

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yair Yogev]

Created attachment 73845 [details]
testcase

So far i was only able to reproduce the issue under Windows but the issue seems fonts specific (fixed width vs variable-width fonts maybe) so it is possible that with different configurations the issue will also reproduce under other OSs.

Test case attached + screenshot taken with Safari + WebKit-r71499.
Use the End key to get to the end of the Hebrew text (the left most position) and the caret will be painted over the last letter.

Using some other font size\combination of letters the caret can even paint before the last letter (which is very confusing) - screenshot of that case from Gmail chat also attached.

[Yair Yogev]

Created attachment 73846 [details]
testcase screenshot

[Yair Yogev]

Created attachment 73847 [details]
screenshot from gchat

The caret is in the end of the line, but paints one letter before that

[Yair Yogev]

relevant Chromium database issue: http://crbug.com/44588

[Xiaomei Ji]

Created attachment 85494 [details]
patch w/ layout test

[Dave Hyatt]

Comment on attachment 85494 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=85494&action=review

You got the vertical text case wrong, so might want to make a vertical text test (you'll have to use a normal contenteditable div for that though, since textareas don't go vertical.)

> Source/WebCore/rendering/RenderText.cpp:533
> +        rightEdge = cb->width();

Should be rightEdge = cb->logicalWidth();

> Source/WebCore/rendering/RenderText.cpp:536
> +        rightEdge = max(static_cast<float>(cb->width()), rootRight);

Should be logicalWidth().

[Xiaomei Ji]

Created attachment 85977 [details]
patch w/ layout test

Thanks for the review! Updated patch per feedback in comment #5.

[Xiaomei Ji]

*** Bug 55970 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Comment on attachment 85977 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=85977&action=review

> LayoutTests/fast/forms/cursor-at-editable-content-boundary-expected.txt:26
> +last cursor in textarea_rtl: 210
> +last cursor in textarea_ltr: 240
> +last cursor in textarea_rtl_no_wrap: 175
> +last cursor in textarea_ltr_no_wrap: 229
> +last cursor in vertical-rl: 249
> +last cursor in vertical-rl-no-wrap: 130
> +last cursor in vertical-lr: 351
> +last cursor in vertical-lr-no-wrap: 54

These numbers are likely to have platform/port-specific values.  Is there anyway to verify these numbers without dumping them on the expected result?

[Xiaomei Ji]

(In reply to comment #8)
> (From update of attachment 85977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85977&action=review
> 
> > LayoutTests/fast/forms/cursor-at-editable-content-boundary-expected.txt:26
> > +last cursor in textarea_rtl: 210
> > +last cursor in textarea_ltr: 240
> > +last cursor in textarea_rtl_no_wrap: 175
> > +last cursor in textarea_ltr_no_wrap: 229
> > +last cursor in vertical-rl: 249
> > +last cursor in vertical-rl-no-wrap: 130
> > +last cursor in vertical-lr: 351
> > +last cursor in vertical-lr-no-wrap: 54
> 
> These numbers are likely to have platform/port-specific values.  Is there anyway to verify these numbers without dumping them on the expected result?

I have not thought of a way to make it cross platform.
For the bug itself to show up in different platforms, I need to tweak the width and font parameters. For example, this exactly case wont break in Chrome windows. Maybe there is a way to fix the font and font size so that both the bug and expected result is cross-platform?

[Ryosuke Niwa]

Comment on attachment 85977 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=85977&action=review

>>> LayoutTests/fast/forms/cursor-at-editable-content-boundary-expected.txt:26
>>> +last cursor in vertical-lr-no-wrap: 54
>> 
>> These numbers are likely to have platform/port-specific values.  Is there anyway to verify these numbers without dumping them on the expected result?
> 
> I have not thought of a way to make it cross platform.
> For the bug itself to show up in different platforms, I need to tweak the width and font parameters. For example, this exactly case wont break in Chrome windows. Maybe there is a way to fix the font and font size so that both the bug and expected result is cross-platform?

Is there some condition in which this bug reproduces?  e.g. textarea's width must be about the width of the text inside.  If this test doesn't work on all platforms, then it should be under LayoutTests/platform/<whatever platform this test works>/fast/forms/

[Xiaomei Ji]

(In reply to comment #10)
> (From update of attachment 85977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85977&action=review
> 
> >>> LayoutTests/fast/forms/cursor-at-editable-content-boundary-expected.txt:26
> >>> +last cursor in vertical-lr-no-wrap: 54
> >> 
> >> These numbers are likely to have platform/port-specific values.  Is there anyway to verify these numbers without dumping them on the expected result?
> > 
> > I have not thought of a way to make it cross platform.
> > For the bug itself to show up in different platforms, I need to tweak the width and font parameters. For example, this exactly case wont break in Chrome windows. Maybe there is a way to fix the font and font size so that both the bug and expected result is cross-platform?
> 
> Is there some condition in which this bug reproduces?  e.g. textarea's width must be about the width of the text inside.  If this test doesn't work on all platforms, then it should be under LayoutTests/platform/<whatever platform this test works>/fast/forms/

by doesn't work, I mean it does not break in other platforms simply because of the font differences. Then, should we put it under platform?

[Ryosuke Niwa]

(In reply to comment #11)
> by doesn't work, I mean it does not break in other platforms simply because of the font differences. Then, should we put it under platform?

We should figure out a way to reproduce the issue on all platforms.  How about if we computed the width of text and adjusted the width of textarea?

[Xiaomei Ji]

(In reply to comment #12)
> (In reply to comment #11)
> > by doesn't work, I mean it does not break in other platforms simply because of the font differences. Then, should we put it under platform?
> 
> We should figure out a way to reproduce the issue on all platforms.  How about if we computed the width of text and adjusted the width of textarea?

Yes, that will work for no-wrap case if we set the textarea's padding and margin as 0.
by computing the distance of caretRect in 1st and last character, and check the result against the textarea width. 
But what to check against in the no-wrap case?

[Xiaomei Ji]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > by doesn't work, I mean it does not break in other platforms simply because of the font differences. Then, should we put it under platform?
> > 
> > We should figure out a way to reproduce the issue on all platforms.  How about if we computed the width of text and adjusted the width of textarea?
> 
> Yes, that will work for no-wrap case if we set the textarea's padding and margin as 0.
> by computing the distance of caretRect in 1st and last character, and check the result against the textarea width. 
> But what to check against in the no-wrap case?

what to check against in the wrap case?

[Ryosuke Niwa]

(In reply to comment #14)
> what to check against in the wrap case?

What if we compared the delta to compute width of each characters using caret rect and compared that with the case when the entire text appears in single line?  Character widths (differences in caret rect) should match, right?

[Xiaomei Ji]

(In reply to comment #15)
> (In reply to comment #14)
> > what to check against in the wrap case?
> 
> What if we compared the delta to compute width of each characters using caret rect and compared that with the case when the entire text appears in single line?  Character widths (differences in caret rect) should match, right?

Looks like it is doable for horizontal writing mode, with the fixes of bug 56854 and another minor fix in RenderText::localCaretRect() (then, character width matches).

Vertical text has some problems though. Using the following example:
<div contenteditable id="vertical-rl" style="padding:0; margin:0; height:175px; -webkit-writing-mode: vertical-rl; font: 8px;">&#1491;&#1490;&#1500;&#1495;&#1499; &#1490;&#1491;&#1499; &#1500;&#1495;&#1497;&#1491;&#1490;&#1499; &#1497;&#1495;&#1506;&#1491;&#1491;</div>

The size of the Hebrew text is <172px, but with height of 175px, the text is rendered as 2 lines when page is first loaded. But it renders as one line when reload the page. Looks like a bug.

[Xiaomei Ji]

Created attachment 88852 [details]
patch w/ layout test

in the case of auto wrapping and nowrap,  the width of each character computed by using caret rect does not match.
nowrapping failed and caret overlaps with character, I think, due to bug 56854 and other float conversion/rounding/hacking.

[Dave Hyatt]

Comment on attachment 88852 [details]
patch w/ layout test

r=me

[Xiaomei Ji]

Committed r83348: <http://trac.webkit.org/changeset/83348>

[Yair Yogev]

using Google Chrome 12.0.734.0 (81371) canary build with WebKit 534.29 (trunk@83605)
it doesn't look like the issue is fixed ?
using the original testcase, the caret is over the last letter after pressing the End key.

[Xiaomei Ji]

Comment on attachment 88852 [details]
patch w/ layout test

clean up flags.

[Xiaomei Ji]

the patch was rolled out due to the test is failed in almost every platforms except leopard and snow  leopard. Need further investigation.

[Xiaomei Ji]

Created attachment 89712 [details]
patch w/ layout test

For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.

(In reply to comment #23)
> Created an attachment (id=89712) [details]
> patch w/ layout test
> 
> For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.

Where does the font come from? Is its license compatible with the WebKit Open Source project?

[Xiaomei Ji]

(In reply to comment #24)
> (In reply to comment #23)
> > Created an attachment (id=89712) [details] [details]
> > patch w/ layout test
> > 
> > For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.
> 
> Where does the font come from? Is its license compatible with the WebKit Open Source project?

It is an open source font, should be Apache license. It is used in Android.

[Xiaomei Ji]

Created attachment 89820 [details]
patch w/ layout test

For layout test, add and load DroidSansHebrew font (it is a open source font, Apache license) to avoid font differences between div/textarea defined in user agent for different platforms.

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Created an attachment (id=89712) [details] [details] [details]
> > > patch w/ layout test
> > > 
> > > For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.
> > 
> > Where does the font come from? Is its license compatible with the WebKit Open Source project?
> 
> It is an open source font, should be Apache license. It is used in Android.

Thanks for clarifying this. The Apache license is not compatible with the WebKit Open Source project.

[Xiaomei Ji]

(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #23)
> > > > Created an attachment (id=89712) [details] [details] [details] [details]
> > > > patch w/ layout test
> > > > 
> > > > For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.
> > > 
> > > Where does the font come from? Is its license compatible with the WebKit Open Source project?
> > 
> > It is an open source font, should be Apache license. It is used in Android.
> 
> Thanks for clarifying this. The Apache license is not compatible with the WebKit Open Source project.

Thanks that you caught this.

What open license is compatible with WebKit open source project?
GPL? MPL? 
How about OFL (http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=OFL)?

Maybe I can use the following,  which is GPL 2 compliant?
http://culmus.sourceforge.net/

Or do you have any suggestions?

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > (In reply to comment #23)
> > > > > Created an attachment (id=89712) [details] [details] [details] [details] [details]
> > > > > patch w/ layout test
> > > > > 
> > > > > For layout test, add and load DroidSansHebrew font to avoid font differences between div/textarea defined in user agent for different platforms.
> > > > 
> > > > Where does the font come from? Is its license compatible with the WebKit Open Source project?
> > > 
> > > It is an open source font, should be Apache license. It is used in Android.
> > 
> > Thanks for clarifying this. The Apache license is not compatible with the WebKit Open Source project.
> 
> Thanks that you caught this.
> 
> What open license is compatible with WebKit open source project?

According to the Committer Guidelines, “All code committed to the WebKit SVN repository must be governed by either a BSD-style license (as listed below) or the LGPL 2.1, unless otherwise cleared by Apple in writing.”

> GPL? MPL? 
> How about OFL (http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=OFL)?
> 
> Maybe I can use the following,  which is GPL 2 compliant?
> http://culmus.sourceforge.net/
> 
> Or do you have any suggestions?

I have a couple of suggestions:
1. Use a modified copy of one of the SVG fonts already in the LayoutTests directory, for example, LayoutTests/svg/custom/resources/SVGFreeSans.svg with a few glyph elements added for RTL characters (by duplicating existing glyphs)
2. Use a generic family (e.g. sans-serif) and have platform-dependent results (I am not sure what in the test requires it to use the same font on all platforms).

[Xiaomei Ji]

> I have a couple of suggestions:
> 1. Use a modified copy of one of the SVG fonts already in the LayoutTests directory, for example, LayoutTests/svg/custom/resources/SVGFreeSans.svg with a few glyph elements added for RTL characters (by duplicating existing glyphs)
> 2. Use a generic family (e.g. sans-serif) and have platform-dependent results (I am not sure what in the test requires it to use the same font on all platforms).

The test itself checks whether the length of the text is the same as the difference between first caret rect and last caret rect.

It is expected to pass in all platforms.

But there is one case that failed in Mac (http://webkit.org/b/56854). When there is failure, the length of the text and the difference of caret range are printed out, then, the number will be different in different platforms.

Actually, looks like even with the same font and same font size, the length computed in different platforms might be different (I tried safari in snow-leopard and chromium in linux using DroidSansHebrew, the length computed are slightly different).

[Xiaomei Ji]

(In reply to comment #30)
> > I have a couple of suggestions:
> > 1. Use a modified copy of one of the SVG fonts already in the LayoutTests directory, for example, LayoutTests/svg/custom/resources/SVGFreeSans.svg with a few glyph elements added for RTL characters (by duplicating existing glyphs)
> > 2. Use a generic family (e.g. sans-serif) and have platform-dependent results (I am not sure what in the test requires it to use the same font on all platforms).
> 
> The test itself checks whether the length of the text is the same as the difference between first caret rect and last caret rect.
> 
> It is expected to pass in all platforms.
> 
> But there is one case that failed in Mac (http://webkit.org/b/56854). When there is failure, the length of the text and the difference of caret range are printed out, then, the number will be different in different platforms.
> 
> Actually, looks like even with the same font and same font size, the length computed in different platforms might be different (I tried safari in snow-leopard and chromium in linux using DroidSansHebrew, the length computed are slightly different).

I think I will use sans-serif. 
Another question as to the failure in Mac, should I check in the failed expectation? Or I should split the test and mark the failed test as skipped in Mac? (there is bug filed: http://webkit.org/b/56854).

(In reply to comment #31)
> Another question as to the failure in Mac, should I check in the failed expectation? Or I should split the test and mark the failed test as skipped in Mac? (there is bug filed: http://webkit.org/b/56854).

Please check in the failure result. This makes it easier to detect further regressions or progression. Thanks!

[Xiaomei Ji]

Created attachment 89859 [details]
patch w/ layout test

Use sans-serif for layout test.

[Dave Hyatt]

Comment on attachment 89859 [details]
patch w/ layout test

r=me

[Xiaomei Ji]

Committed r84659: <http://trac.webkit.org/changeset/84659>