Bug 56854 - computation in InlineTextBox::positionForOffset() might be wrong.
Summary: computation in InlineTextBox::positionForOffset() might be wrong.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-22 12:02 PDT by Xiaomei Ji
Modified: 2011-04-06 01:05 PDT (History)
2 users (show)

See Also:


Attachments
patch w/ layout test (5.61 KB, patch)
2011-04-05 16:15 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.