[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 #10 from xji at chromium.org  2009-04-22 16:39 PDT -------
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


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