[Webkit-unassigned] [Bug 25298] Ctrl + Right/Left arrow move forward/backward through document instead of right/left in RTL text

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

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




--- Comment #16 from mitz at webkit.org  2010-06-16 10:00:54 PST ---
(From update of attachment 58093)
Thanks for your patch! I have some comments on this version:

Does it really make sense to use a word break iterator on a string in visual order? I would think that the word break iterator is meant to be use on a logically-ordered string. For example, for languages where word breaks are based on a dictionary, like Thai, I don’t think this will work.

> +    Node* de = d->documentElement();

Please use a longer name for this variable, like “documentElement”.

> +    while (box) {
> +        node = box->renderer()->node();
> +        // Get strings in visual order.
> +        if (box->direction() == LTR) {
> +            range->setStart(node, box->caretMinOffset(), ec);
> +            startNode ? range->setEnd(node, offset, ec) : range->setEnd(node, box->caretMaxOffset(), ec);

Normally the ternary operator is only used in WebKit when the result is used (for example, as a parameter to a function or assigned to a variable). In other cases, an if statement is used.

> +            SimplifiedBackwardsTextIterator it(range.get());

I am confused by the way you set up the range and use of a backwards text iterator. I am really not sure how adjusting only one end of the range makes sense; the code seems to make some assumptions about how the text iterator works and what it returns first (you never advance the iterator) which it should probably not be making.

> +            string.prepend(it.characters(), it.length());
> +        } else {
> +            startNode ? range->setStart(node, offset, ec) : range->setStart(node, box->caretMinOffset(), ec);
> +            range->setEnd(node, box->caretMaxOffset(), ec);
> +            SimplifiedBackwardsTextIterator it(range.get());
> +            for (int i = 0; i < it.length(); ++i)
> +                string.prepend(it.characters()[i]);

Like I said, I doubt that reversing the string will always give the correct result. Moreover, you cannot reverse a string like this, because it will break surrogate pairs (characters that are encoded by two UTF-16 characters).

> +        prev = previousWordPositionBoundary(string.data(), string.size(), string.size(), DontHaveMoreContext, needMoreContext);

Is it really true that you couldn’t provide more context to the break iterator if needed?

> +VisiblePosition rightWordPosition(const VisiblePosition& c)

A lot of the code in this function is identical or similar to leftWordPosition(). The common code should not be repeated. It should be factored out (probably into file-static functions).

I think it’s great that you’ve added so many tests! Do you know if the results in these tests match the behavior of the native text system on Mac OS X (that is, does it behave like TextEdit?)? What about other platforms’ native text systems?

It’s especially good that you have these tests and the expected results, because I think you need to take a significantly different approach to this bug, and the tests will allow you to verify that the new approach produces the same (or better) results. It seems like the behavior you were going for here is to return the (visually) nearest position to the left (respectively, to the right) that is a word boundary. A naive algorithm could just iterate over word boundaries (logically) forward and backwards along the line, then pick the one nearest to the starting position to the left (resp. to the right). Does that sounds like a correct description of the desired behavior? If so, perhaps the right thing to do would be to start with an implementation of the naive algorithm, then see how it can be made reasonably efficient.

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