Bug 24168 – RTL: Home/End key does not behave correctly in mixed bidi text in RTL document

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xiaomei Ji]

for right-to-left document, in mixed RTL/LTR text, press Home and End key makes cursor showing at beginning or end of the English text rather than the whole line.

Cursor works fine for mixed text in left-to-right document.

[Xiaomei Ji]

Created attachment 27979 [details]
test case -- rtl document with bidi text

Test cases attached.

Steps to reproduce the bug:
1. Open the attached html file.
2. Focus on the editable div
3. Press Home/End key to observe the cursor positioning

[Xiaomei Ji]

Also reported in Chrome:
http://code.google.com/p/chromium/issues/detail?id=4556

[Xiaomei Ji]

I investigated a bit on this bug. Following is my finding. Please correct me if I mis-understood anything.

Given the following bidi text (in visual order) in RTL textbox:
"def FED 123 CBA abc", in which, lower case letter are LTR text, and upper case letter are RTL text.

The position of the characters (with dummy_RTL added at the begin and end of the line) in the line is:
dummy_RTL(20)d(18)e(19)f(17) (16)F(15)E(14)D(13) (12)1(10)2(11)
3(9) (8)C(7)B(6)A(5) (4)a(2)b(3)c(1)dummy_RTL, in which, the number after the character is the visible position in the line.

The InlineBidiResolver constructs bidi runs in visual order.
The node list of the above line is: (def)( FED )(123)( CBA )(abc), in which the node (def) is the first leaf node, and node (abc) is the last node.

node (def) starts at position 17 and ends at position 20.
in "static VisiblePosition startPositionForLine", start position of line is the start position of the start node of the line, that is why 17 is returned, and the cursor is at the (visual) position after 'f' when HOME key is pressed.

Similarly, end position of line is the end position of the last node of the line, that is why 4 is returned, and the cursor is at the (visual) position before 'a' when END key is pressed.

Similar for text "IHG def FED xyz CBA", cursor is visually before 'd' when HOME is pressed, and it is afer 'z' when END is pressed.

Since HOME/END key are logical keys, I am wondering should we use logical node order? so that the first node is "abc" and the last node is "def"? Then, where is the logical node order saved?

Or in static VisiblePosition startPositionForLine(), we can simply check whether the line is in RTL environment, and use the start position of *last* node as the start position of the line if it is in RTL environment.  

[Xiaomei Ji]

Created attachment 29078 [details]
a patch not completely working

Hi Mitz,

The attached patch is not completely working. It is just for your reference if needed.

I tried to provide functions that
return the logic start node (instead of the visual start node), and use the logic start node's start position as the start position for line.

The way I am getting the logic start node is to check whether the enclosing block is in RTL directionality, if it is, the logic start node is the visual end node, otherwise, the logic start node is still the visual start node.

It works fine for a simple block with bidi text, such as <div dir=rtl>abc ABC efg EFG</div> (capital letter represents Hebrew Character).

But it does *not* work for cases such as <div dir=rtl>abc <span> efg hij</span> xyz</div>, for which, the assumption "logic start node is the visual end node if the directionality is RTL" is *not* true.


I would like to get advise on
1. what will be a correct way to get the logic start node of a block?
2. Or I am in a completely wrong direction of fixing it? 

Thanks and appreciated!
Xiaomei

[Xiaomei Ji]

Created attachment 29294 [details]
another test case (inline element with dir="rtl") Home/End key do not work

In the current webkit, 
home/end key works fine for
<div dir=rtl  contenteditable>Lorem <span  style="direction: rtl">ipsum dolor sit</span> amet</div>

But not for
<div dir=rtl  contenteditable>Lorem <span  dir="rtl">ipsum dolor sit</span> amet</div>

[Xiaomei Ji]

Created attachment 29296 [details]
patch w/ Layout test

Looks like I can use the comparison of bidi resolver's lastRun() and logicallyLastRun() to check whether the logic last node is the visual last node or not. 

If a line is an RTL line and its visual last node is not the same as its logic last node, its logic last node is its visual start node, such as bidi text in a RTL div, or <div dir=rtl> abc <span dir="rtl"> efg hij </span> xyz </div>.
Otherwise, its logic last node is the same as its visual last node, such as 
<div dir=rtl> abc <span style="direction:rtl"> efg hij </span> xyz </div>.

The same applies to logic start node.

This patch fixes the attached 2 test cases: bidi text in RTL document and RTL text with inline RTL element.

Please be noted:
1. I am fixing the home/end key in "move" + "right"/"left", not "move" + "forward"/"backward".
But I think it should be fixed in "move" + "foward"/"backward".

2. All tests passed, but there are 22 tests with stderr output.
They looks like environment problem, and I do not think they are related to my change.

Comment on attachment 29296 [details]
patch w/ Layout test

Resetting the review flag because Xiaomei is working on a new patch.

[Xiaomei Ji]

Created attachment 29402 [details]
patch w/ Layout test

Hi Mitz,

Thanks for your inputs on previous patch. They are very helpful!

I changed to reconstruct the logic order from line boxes by reversing L2 phase in Unicode Bidi algorithm.

I am reconstructing the logic order of leave boxes only. I think that should be enough.

But I still have 3 questions:
1. I am implementing the logic "home/end' key functionality in "move" + "right"/"left", but I think it should be done in "move" + "forward"/"backward".

2. The reconstruction is done in editing per home/end operation.
I am not sure whether it should be done in rendering code, after the whole structure of a line is fully constructed? and in-validate it if line structure changed? So it could save some performance overhead.

3. I am caching the logic chain in InlineBox because:
a) I need the chain (at least the 'next') for continuing reverse boxes during reconstruct logic order from low bidi level to high bidi level.
b) I need the chain to get the 'next' (or 'prev') line box if the startBox->renderer()->node() == NULL (or the endBox->renderer()->node() == NULL) in getLogicStartBoxAndNode() (or getLogicEndBoxAndNode()).

Comment on attachment 29402 [details]
patch w/ Layout test

I think you are right: you should change the backward/forward, not the left/right variant, and make the home/end keys map to back/forward.

Please use "logical" instead of the abbreviation "logic".

A typical line contains very few leaf boxes (typically 1, perhaps 2 for contenteditable text with line-break: after-white-space, but hardly ever more than 10), so reconstructing the logical order every time for an operation like home/end is not a problem. Adding two pointer-sized members to InlineBox is not necessary, and has a severe impact on memory use.

I think it would actually be simpler to use a Vector<InlineBox*>, fill it up with the boxes in visual order (i.e. in nextLeafChild() order), then do the reordering in-place in the vector.

You could have one

static void getLeafBoxesInLogicalOrder(RootInlineBox*, Vector<InlineBox*>&)

that did that. You wouldn't even need a helper function for reversing because you could use std::reverse().

+    unsigned char levelLow = 128;
+    unsigned char levelHigh = 0;

I know that the names levelLow and levelHigh are used in BidiResolver, but those are old and rather unusual names. minLevel and maxLevel would be better.

+    InlineBox* firstBox = NULL;
+    InlineBox* lastBox = NULL;

This will go away if you use a Vector-based getLeafBoxesInLogicalOrder(), but I just wanted to point out that WebKit style is to use 0, not NULL, for the null pointer, and that since you follow with a call to reconstructLogicOrder(), which initializes firstBox and lastBox, you do not need to initialize them when defining them anyway.

+    while (1) {
+        if (!startBox)
+            return;

Since there is nothing after the while loop, this can be written simply as
+    while (startBox)

+        RenderObject *startRenderer = startBox->renderer();

The star should go next to RenderObject.

+        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.

+    RootInlineBox *rootBox = rootBoxForLine(c);

The start should go next to the type name.

+    // Generated content (e.g. list markers and CSS :before and :after
+    // pseudoelements) have no corresponding DOM element, and so cannot be
+    // represented by a VisiblePosition.  Use whatever follows instead.

I don't understand this comment. Where is the part where you "use whatever follows"? I think it is actually part of getLogicStartBoxAndNode() now!

+    InlineBox *logicStartBox = NULL;
+    Node *logicStartNode = NULL;

I think the rule should be that getLogicStartBoxAndNode() initializes those. Also, please correct the placement of the stars.

+    if (logicStartBox == NULL || logicStartNode == NULL) {
+        return VisiblePosition();
+    }

The WebKit style of writing this is
+    if (!logicStartBox || !logicStartNode)
+        return VisiblePosition();

without NULL or 0 and without braces around the single-line clause. Also, shouldn't it be enough to only null-test the node?

+    int startOffset = 0;
+    if (logicStartBox->isInlineTextBox()) {
+        InlineTextBox *startTextBox = static_cast<InlineTextBox *>(logicStartBox);
+        startOffset = startTextBox->start();
+    }

I think you can use InlineBox::caretMinOffset() here instead of special-casing InlineTextBox.

+VisiblePosition logicStartOfLine(const VisiblePosition& c)
+{
+    VisiblePosition visPos = logicStartPositionForLine(c);
+    
+    if (visPos.isNotNull()) {

You can change this to an early return.

+    int endOffset = 1;
+    if (logicEndNode->hasTagName(brTag)) {
+        endOffset = 0;
+    } else if (logicEndBox->isInlineTextBox()) {
+        InlineTextBox *endTextBox = static_cast<InlineTextBox *>(logicEndBox);
+        endOffset = endTextBox->start();
+        if (!endTextBox->isLineBreak())
+            endOffset += endTextBox->len();
+    }

And here you can use InlineBox::caretMaxOffset() in the else clause, instead of special-casing InlineTextBox. Can you use caretMaxOffset() to get the desired result for a <br> too?

+bool inSameLogicLine(const VisiblePosition &a, const VisiblePosition &b)

Can inSameLogicLine(a, b) and inSameLine(a, b) ever return a different result?

Finally, I want to ask about thse two cases:

+        // 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, logicStartPositionForLine 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())) {

and

+    // Make sure the end of line is at the same line as the given input position.  Else use the previous position to 
+    // obtain end 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, logicEndPositionForLine would incorrectly hand back a position
+    // in the next line instead. 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. 
+    if (!inSameLogicLine(c, visPos)) {

I don't fully understand the former and I am not sure the latter can happen. Are they covered by your test? Do we need them?

+//            positions.push({ node: sel.anchorNode, offset: sel.anchorOffset});

+ //           positions.push({ node: sel.anchorNode, offset: sel.anchorOffset});

Please remove commented-out code from the test.

Property changes on: LayoutTests/editing/selection/home-end.html
___________________________________________________________________
Name: svn:executable
   + *

This is unnecessary.

Looks like you are in the right direction, but you can do the same with simpler code!

[Xiaomei Ji]

Hi Mitz,

Thanks for your review and detailed feedback.
I am updating the code according to your suggestions.

But I would like to check with you on some comments first.
Please see my replies inline.


> 
> +        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?

> 
> +    if (logicStartBox == NULL || logicStartNode == NULL) {
> +        return VisiblePosition();
> +    }
> 
> The WebKit style of writing this is
> +    if (!logicStartBox || !logicStartNode)
> +        return VisiblePosition();
> 
> without NULL or 0 and without braces around the single-line clause. Also,
> shouldn't it be enough to only null-test the node?

It is related to the above question that whether a leave box's renderer could ever be null. If it could, then, the logicStartBox is *not* NULL, but the logicStartNode is NULL.


> 
> +    int startOffset = 0;
> +    if (logicStartBox->isInlineTextBox()) {
> +        InlineTextBox *startTextBox = static_cast<InlineTextBox
> *>(logicStartBox);
> +        startOffset = startTextBox->start();
> +    }
> 
> I think you can use InlineBox::caretMinOffset() here instead of special-casing
> InlineTextBox.
> 

Do you mean change the above to "int startOffset = logicalStartBox->caretMinOffset()"?
Then, is it the same as the above logic?
Could leaf box be InlineRunBox or InlineFlowBox? In which cases, 
will the caretMinOffset() be 0?

I am a bit confused about those virtual functions in InlineBox (and its derived classes) and RenderObject (and its derived classes). Who will use  RenderText::caretMaxOffset()?

> 
> +    int endOffset = 1;
> +    if (logicEndNode->hasTagName(brTag)) {
> +        endOffset = 0;
> +    } else if (logicEndBox->isInlineTextBox()) {
> +        InlineTextBox *endTextBox = static_cast<InlineTextBox *>(logicEndBox);
> +        endOffset = endTextBox->start();
> +        if (!endTextBox->isLineBreak())
> +            endOffset += endTextBox->len();
> +    }
> 
> And here you can use InlineBox::caretMaxOffset() in the else clause, instead of
> special-casing InlineTextBox. Can you use caretMaxOffset() to get the desired
> result for a <br> too?
> 

Even for the "else if" case (the InlineTextBox), is the above logic the same as caretMaxOffset()?
caretMaxOffset() returns m_start + m_len for InlineTextBox. If the InlineTexBox isLineBreak(), is its m_len == 0? I did not find the related code by just quick browsing.


> +bool inSameLogicLine(const VisiblePosition &a, const VisiblePosition &b)
> 
> Can inSameLogicLine(a, b) and inSameLine(a, b) ever return a different result?
> 

It happens in my test, that is why I introduced isSameLogicLine (using isSameLine wont work). I will dig into the test for more information.


> Finally, I want to ask about thse two cases:
> 
> +        // 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,
> logicStartPositionForLine 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())) {
> 
> and
> 
> +    // Make sure the end of line is at the same line as the given input
> position.  Else use the previous position to 
> +    // obtain end 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,
> logicEndPositionForLine would incorrectly hand back a position
> +    // in the next line instead. 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. 
> +    if (!inSameLogicLine(c, visPos)) {
> 
> I don't fully understand the former and I am not sure the latter can happen.
> Are they covered by your test? Do we need them?
> 

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.

Thanks,
Xiaomei

(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.

> > +    int startOffset = 0;
> > +    if (logicStartBox->isInlineTextBox()) {
> > +        InlineTextBox *startTextBox = static_cast<InlineTextBox
> > *>(logicStartBox);
> > +        startOffset = startTextBox->start();
> > +    }
> > 
> > I think you can use InlineBox::caretMinOffset() here instead of special-casing
> > InlineTextBox.
> > 
> 
> Do you mean change the above to "int startOffset =
> logicalStartBox->caretMinOffset()"?

Yes.

> Then, is it the same as the above logic?
> Could leaf box be InlineRunBox or InlineFlowBox? In which cases, 
> will the caretMinOffset() be 0?

An empty InlineFlowBox can be a leaf, in which case you will get 0.

> I am a bit confused about those virtual functions in InlineBox (and its derived
> classes) and RenderObject (and its derived classes). Who will use 
> RenderText::caretMaxOffset()?

caretMaxOffset() in htmlediting.cpp looks like one place that calls it.

> > +    int endOffset = 1;
> > +    if (logicEndNode->hasTagName(brTag)) {
> > +        endOffset = 0;
> > +    } else if (logicEndBox->isInlineTextBox()) {
> > +        InlineTextBox *endTextBox = static_cast<InlineTextBox *>(logicEndBox);
> > +        endOffset = endTextBox->start();
> > +        if (!endTextBox->isLineBreak())
> > +            endOffset += endTextBox->len();
> > +    }
> > 
> > And here you can use InlineBox::caretMaxOffset() in the else clause, instead of
> > special-casing InlineTextBox. Can you use caretMaxOffset() to get the desired
> > result for a <br> too?
> > 
> 
> Even for the "else if" case (the InlineTextBox), is the above logic the same as
> caretMaxOffset()?
> caretMaxOffset() returns m_start + m_len for InlineTextBox. If the InlineTexBox
> isLineBreak(), is its m_len == 0? I did not find the related code by just quick
> browsing.

Oh, I think you are right! For pre-wrapped text, the \n will have an InlineTextBox with m_len == 1, but you want the offset before the newline character.

> 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.

[Xiaomei Ji]

(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 &#1488;&#1489;&#1490; xyz &#1491;&#1492;&#1493; 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();

[Xiaomei Ji]

Created attachment 29748 [details]
patch w/ Layout test (version 5)

I've incorporated all the changes except that:

1. I am leaving the following block in logicalStartOfLine()
    // Make sure the start of line is not greater than the given input position.  ....
    Position p = visPos.deepEquivalent();
    if (p.m_offset > c.deepEquivalent().m_offset && p.node()->isSameNode(c.deepEquivalent().node())) {
        ....
    }

2. I changed the inSameLogicalLine() block in logicalEndOfLine() to
    if (!inSameLogicalLine(c, visPos))
        visPos = visPos.previous();

Thanks,
Xiaomei

[Yair Yogev]

i don't know if it's related or not to this fix... but i wanted to recommend checking that Ctrl+Home/End isn't affected by this fix by mistake (AFAIK that shortcut is working as it should right now):)

[Yair Yogev]

oh and another thing to check is selection using Shift+Home/End (currently not working properly)

[Xiaomei Ji]

(In reply to comment #14)
> i don't know if it's related or not to this fix... but i wanted to recommend
> checking that Ctrl+Home/End isn't affected by this fix by mistake (AFAIK that
> shortcut is working as it should right now):)
> 

This fix only fix the problem with "HOME" or "END" key, nothing else.
So, ctrl+HOME/END key wont be touched.

[Xiaomei Ji]

(In reply to comment #15)
> oh and another thing to check is selection using Shift+Home/End (currently not
> working properly)
> 

Unfortunately, the current fix only fixes problem with "HOME/END" key.
not including shift+HOME/END key.

I marked 2 FIXME in the code, but I did not plan to fix the problem with shift+HOME/END key in this patch.

Will it acceptable?

[Yair Yogev]

Shift+Home/End is very commonly used so it's going to be the next issue posted then. Of course any fix you post makes us RTL users happier :)

[Xiaomei Ji]

Created attachment 29756 [details]
patch w/ Layout test (version 6)

Comparing with version 5, this patch also fixed the problem in shift+HOME/END and includes corresponding tests.

Comment on attachment 29756 [details]
patch w/ Layout test (version 6)

> +    if (!rootBox->renderer()->style()->visuallyOrdered()) {

It would be nicer to test for the opposite condition and bail out early if it's true.

> +        // Reverse of reordering of the line (L2 according to Bidi spec):
> +        // L2. From the highest level found in the text to the lowest odd level on each line,
> +        // reverse any contiguous sequence of characters that are at that level or higher.
> +
> +        // reversing of L2 is only done up to the lowest odd level

That last comment should also be capitalized and end with a period. I don't understand what "of L2" means in that sentence.

> +            Vector<InlineBox*>::iterator iter = leafBoxesInLogicalOrder.begin();

A Vector<InlineBox*>::iterator is just an InlineBox**, so usually we just write InlineBox**.

> +            Vector<InlineBox*>::iterator end = leafBoxesInLogicalOrder.end();

This can go outside the outer while() loop, because it never changes.

> +    size_t index = len - 1;
> +    while (1) {
> +        endBox = leafBoxesInLogicalOrder[index];
> +        endNode = endBox->renderer()->node();
> +        if (endNode || !index)
> +            return;
> +        --index;
> +    }
> +}

Please use while (true) instead of while (1). Also, I think it would be nicer to make this a for() loop, to be more like getLogicalStartBoxAndNode(). You can do something like

+     for (i = leafBoxesInLogicalOrder.size(); i > 0; --i) {
+         endBox = leafBoxesInLogicalOrder[i - 1];
          ...

> +    // 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);
> +    }

Since we think this cannot happen, please remove the above from logicalStartOfLine().

> +        if (p.node()->renderer() && p.node()->renderer()->isRenderBlock() && p.m_offset == 0)

Please write !p.m_offset instead of p.m_offset == 0 (there are two instances of this).

> +    if (logicalEndNode->hasTagName(brTag)) {
> +        endOffset = 0;
> +    } else if (logicalEndBox->isInlineTextBox()) {

Please remove the braces around the 1-line if clause.

> +bool inSameLogicalLine(const VisiblePosition &a, const VisiblePosition &b)

The ampersands should go next to the type.

r=me but please make the style changes and consider making the other changes too before landing.

[Xiaomei Ji]

Created attachment 29889 [details]
patch w/ Layout test (version 7)

Hi Mitz,

Thanks for the review! Appreciated!

I've updated the visible_units.cpp per your suggestion.
Could you please review it again?

Thanks,
Xiaomei

Comment on attachment 29889 [details]
patch w/ Layout test (version 7)

r=me

> +        InlineTextBox* endTextBox = static_cast<InlineTextBox *>(logicalEndBox);

No space needed before the last *.

[Xiaomei Ji]

Created attachment 29895 [details]
patch w/ Layout test (version 8)

Hi Mitz,

I've removed the space within "InlineTextBox *".
Could you please review it again?

Thanks,
Xiaomei

[Eric Seidel (no email)]

I had to remove tabs from your test cases in order to commit this.  Otherwise the commit went smoothly.  Thank you for the patch!

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/editing/selection/extend-selection-expected.txt
	M	LayoutTests/editing/selection/extend-selection.html
	A	LayoutTests/editing/selection/home-end-expected.txt
	A	LayoutTests/editing/selection/home-end.html
	M	WebCore/ChangeLog
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/editing/visible_units.cpp
	M	WebCore/editing/visible_units.h
Committed r43019

Comment on attachment 29895 [details]
patch w/ Layout test (version 8)

You don't have to ask for a review for such a minor style change if you can get the person landing the patch to include them.

[Justin Garcia]

I think that:

editing/selection/extend-selection.html
editing/selection/home-end.html 

started failing after this change.

(In reply to comment #26)
> I think that:
> 
> editing/selection/extend-selection.html
> editing/selection/home-end.html 

Looks like a result of removing the tabs when landing. Eric, can you look into it?

[Eric Seidel (no email)]

Rolled out due to test failures.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	LayoutTests/editing/selection/home-end-expected.txt
	D	LayoutTests/editing/selection/home-end.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/editing/selection/extend-selection-expected.txt
	M	LayoutTests/editing/selection/extend-selection.html
	M	WebCore/ChangeLog
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/editing/visible_units.cpp
	M	WebCore/editing/visible_units.h
Committed r43028

[Eric Seidel (no email)]

Comment on attachment 29756 [details]
patch w/ Layout test (version 6)

Clearing review flag to remove from commit queue.

[Eric Seidel (no email)]

Comment on attachment 29889 [details]
patch w/ Layout test (version 7)

Clearing review flag to remove from commit queue.

[Eric Seidel (no email)]

I'm not going to make a second attempt to land this. :)  I leave this to mitz...

Landed in <http://trac.webkit.org/projects/webkit/changeset/43032>.

[Xiaomei Ji]

(In reply to comment #32)
> Landed in <http://trac.webkit.org/projects/webkit/changeset/43032>.
> 

Hi Mitz,

Thanks!
I am just about to upload a patch with the tab-removal test and result files.

xiaomei

[Xiaomei Ji]

(In reply to comment #30)
> (From update of attachment 29889 [details] [review])
> Clearing review flag to remove from commit queue.
> 

Hi Eric,

Thanks for the try! and sorry for the inconvenience.

Xiaomei

(In reply to comment #33)
> I am just about to upload a patch with the tab-removal test and result files.

I just replaced each tab with a single space in the tests and updated the results and landed the patch. I don't think any further changes are needed.