[Webkit-unassigned] [Bug 24303] Using keyboard select RTL text, Highlight goes to opposite direction from FF&IE

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28815|review?                     |review-
               Flag|                            |




------- Comment #16 from mitz at webkit.org  2009-03-26 10:49 PDT -------
(From update of attachment 28815)
> Looks like FF does not across block boundary when extending selection.
> But currently, Webkit across block boundary while extending selection if the
> blocks are parent-child. It can extend selection inward or outward.
> For example: 
> <div> whatever <div dir="rtl"> just a test</div> end</div>
> selection can be extended from the block "just a test" to "whatever ... end",
> and vice versa.
>
> In the current fix, when checking the directionality of enclosing block 
> (SelectionController::directionOfEnclosingBlock()), I used the starting point
> of the selection (m_sel.base().node()), not the ending point
> (m_sel.extend().node()).

What does Firefox do when the selection already crosses a block boundary and
the extent is in a block that has different directionality from the base? I
think using the directionality of the block enclosing the extent makes more
sense, because the extent is what is being modified, although I realize that it
may be weird when crossing into a different direction, like you said.

> +TextDirection SelectionController::directionOfEnclosingBlock() {
> +    Node *n = m_sel.base().node();

The * should go next to "Node".

> +    Node* enclosingBlockNode = enclosingBlock(n);
> +    if (enclosingBlockNode) {

You can write this as
       if (Node* enclosingBlockNode = enclosingBlock(n))...

or better yet, you can save some indentation by reversing the condition and
turning this into an early return:
       if (!enclosingBlockNode)
           return LTR;

> +        WebCore::RenderObject* renderer = enclosingBlockNode->renderer();

Since this function is in the WebCore namespace, you shouldn't add the
"WebCore::" prefix.

> +        if (renderer) {
> +            if (renderer->style()->direction() == WebCore::RTL) {
> +                return RTL;
> +            }

The WebKit coding style is to omit braces around single-line blocks like the
above. But the inner if statement is unnecessary. You can simply write
           if (renderer)
               return renderer->style()->direction();

> +        case CharacterGranularity:
> +            if (directionOfEnclosingBlock() == RTL) {
> +                pos = pos.previous(true);
> +            } else {
> +                pos = pos.next(true);
> +            }

Please remove the braces around those single-line blocks.

In existing WebCore code, sometimes the code for the RTL case comes first and
sometimes it comes second. In new code, I would rather make the LTR case come
first.

> +        case WordGranularity:
> +            if (directionOfEnclosingBlock() == RTL) {
> +                pos = previousWordPosition(pos);
> +            } else {
> +                pos = nextWordPosition(pos);
> +            }

Ditto re: braces and order.


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