[Webkit-unassigned] [Bug 24168] RTL: Home/End key does not behave correctly in mixed bidi text in RTL document

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24168





------- Comment #12 from xji at chromium.org  2009-04-23 16:10 PDT -------
(In reply to comment #11)
> (In reply to comment #10)
> 
> > > +        if (!startRenderer) {
> > > +            startNode = NULL;
> > > +            return;
> > > +        }
> > > 
> > > Can this ever happen? I know that there is other old code checking for this,
> > > but I don't think this case can happen. Maybe the code should assert that it
> > > doesn't happen. Again, if you leave this code in, please use 0 instead of NULL.
> > 
> > I will run layout test against it to check whether it happens.
> > But I see checking whether renderer exists in many places of the code. I am
> > thinking probably to leave it there. 
> > Or renderer of leave box is different, and should *never* be NULL?
> 
> A Node's renderer() can be null, but, as far as I can tell, an InlineBox's
> renderer() can never be null. Most places in WebCore that access an InlineBox's
> render() don't null check.

Running all the layout test of webkit, and InlineBox's renderer is never NULL.
So, I am removing the null check.


> 
> > I have to admit that I copied most of the logic from startOfLine/endOfLine by
> > understanding the main part of them, but not every path. 
> > I did not quite understand the former logic, and I forgot whether it is ever
> > been hit by the test. As to the latter logic, I think it is hit by the code,
> > and that is why I changed isSameLine() to isSameLogicLine().
> > I will run test again to find out more.
> 
> I think some of that logic my actually apply to home/end but *not* to the
> actual current behavior of startOfLine() and endOfLine(), and it was just
> written assuming that the logical and visual orders are the same. You may be
> able to find cases where startOfLine() and endOfLine() fail with RTL or
> bidirectional text because of the wrong assumptions.
> 

First, following is the case where inSameLine() and isLogicalSameLine() return
different result:

For the test case:
<div dir=rtl contenteditable class="test">abc אבג xyz
דהו def</div>
If you place caret at visually right most point (right of 'c'), which is
position 1 in the line, the logicEndOfLine is position 20 in the line. These 2
positions are inSameLogicalLine(), but not inSame(Visual)Line().

The (visual)startOfLine() of position 20 is position 17, but the
(visual)startOfLine() of position 1 falls into the following if block:
    if (visPos.isNotNull()) {
        // Make sure ....
        Position p = visPos.deepEquivalent();
        if (p.m_offset > c.deepEquivalent().m_offset &&
p.node()->isSameNode(c.deepEquivalent().node())) {
            visPos = c.previous();
            if (visPos.isNull())
                return VisiblePosition();
            visPos = startPositionForLine(visPos);
        }
    }

And returned "VisiblePosition()". 

That is might be a bug in startOfLine() as you mentioned. And that is why I
changed it to isLogicalSameLine().

==========================

Second, as to the 2 'if' block in logicalStartOfLine() and logicalEndOfLine().
I did not find test cases that hit the following 'if' block in
logicalStartOfLine():
    // Make sure the start of line is not greater than the given input
position.  Else use the previous position to 
    // obtain start of line.  This condition happens when the input position is
before the space character at the end 
    // of a soft-wrapped non-editable line. In this scenario,
logicalStartPositionForLine would incorrectly hand back a position
    // greater than the input position.  This fix is to account for the
discrepancy between lines with webkit-line-break:after-white-space 
    // style versus lines without that style, which would break before a space
by default. 
    Position p = visPos.deepEquivalent();
    if (p.m_offset > c.deepEquivalent().m_offset &&
p.node()->isSameNode(c.deepEquivalent().node())) {
        visPos = c.previous();
        if (visPos.isNull())
            return VisiblePosition();
        visPos = logicalStartPositionForLine(visPos);
    }


But I did not find test cases that hit the  following 'if' block in
logicalEndOfLine().   
if (!inSameLogicalLine(c, visPos))

For a long line which needs wrapping, the logical end position for the lines
which is not the last 2 lines might incorrectly hand back the logical beginning
of the next line. So the input position and logical end position are not in the
same logical line.
For example, <div contenteditable dir="rtl"
style="line-break:before-white-space">abcdefg abcdefg abcdefg a abcdefg abcdefg
abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg </div>

For such case, the following statement inside the 'if' does not solve the
problem because every previous point (except the right-most one) would still
return the same logical end position for the line, while the right-most point's
previous point will be null, and a VisiblePosition() will be returned which
makes the END key does not have any effect if the caret is at the right-most of
the line (right of any characters in the line).

    if (!inSameLogicLine(c, visPos)) {
        visPos = c.previous();
        if (visPos.isNull())
            return VisiblePosition();
        visPos = logicEndPositionForLine(visPos);
    }

For such case, I think the above should be changed to:
    // In this case, use the previous position of the computed logical end
position.
    if (!inSameLogicalLine(c, visPos))
        visPos = visPos.previous();


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.