Bug 56854

Summary: computation in InlineTextBox::positionForOffset() might be wrong.
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: enrica, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
patch w/ layout test none

Description Xiaomei Ji 2011-03-22 12:02:44 PDT
Following line inside "float InlineTextBox::positionForOffset(int offset) const" might be wrong:

    // FIXME: Do we need to add rightBearing here?
    return f.selectionRectForText(TextRun(text->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_expansion, trailingExpansionBehavior(), !isLeftToRightDirection(), m_dirOverride),
                                  IntPoint(logicalLeft(), 0), 0, from, to).maxX();

convert "logicalLeft()" to "int" in "IntPoint(logicalLeft(), 0)" should not be done.

For example,
    <textarea id="textarea_rtl_no_wrap" style="padding:0; margin:0; white-space:nowrap; width: 100px; font: 8px" dir="rtl">&#1491;&#1490;&#1500;&#1495;&#1499; &#1490;&#1491;&#1499; &#1500;&#1495;&#1497;&#1491;&#1490;&#1499; &#1497;&#1495;&#1506;&#1491;&#1491;</textarea>

the width of the text is 120, the logicalLeft() is -19.775, which is round to -19. Then, the positionForOffset(0) will be "120 - 19 = 101" which is greater than the textarea width: 100px.
Comment 1 Xiaomei Ji 2011-04-05 16:15:39 PDT
Created attachment 88331 [details]
patch w/ layout test

Dan, Enrica,

Could you please help on verifying the layout test result on editing/selection/mixed-editability-10.html and platform/mac/editing/input/caret-primary-bidi.html?

I tried moving caret in caret-primary-bidi.html, and I did not see any where that the caret becomes invisible. Seems the result is acceptable.
But I am not sure on mixed-editability-10.html. The result changed for the first <div> with one regression and one progression.
Comment 2 Xiaomei Ji 2011-04-05 17:28:32 PDT
The patch is not completed.
It breaks mixed-editability-10.html in the sense that when clicking left of the span, the caret overlaps with the span, the caret should be placed before the span.

The code in the patch itself seems reasonable.
But there are probably some other code pieces that do float/int conversion or float rounding or other float hacking that need to be taken care of.
Comment 3 Ryosuke Niwa 2011-04-06 01:05:49 PDT
Comment on attachment 88331 [details]
patch w/ layout test

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

> LayoutTests/platform/mac/editing/input/caret-primary-bidi-expected.txt:3
> -0: 124,508,0,28
> +0: 125,508,0,28
>  1: 21,564,0,28
>  2: 36,564,0,28

These output makes no sense to me.  We should come up with a better output here.