[Webkit-unassigned] [Bug 28306] double-clicking a word inside <b> beside newline select two words

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #18 from Darin Adler <darin at apple.com>  2009-12-07 10:12:01 PST ---
(From update of attachment 43924)
> +	Bug 28306: double-clicking a word inside <b> beside newline select two words

Tab in change log here.

> +static int collapsedSpaceLength(Node* textNode, int textEnd)
> +{
> +    RenderText* renderer = toRenderText(textNode->renderer());
> +    String str = renderer->text();
> +    int strLength = static_cast<int>(str.length());
> +    for (int i = textEnd; i < strLength; ++i)
> +        if (!isCollapsibleWhitespace(str[i]))
> +            return i - textEnd;
> +    return strLength - textEnd;
> +}

The implementation of this function is wrong. The fact that whitespace is
collapsible is not enough. You need to take style into account. The right way
to do that is to call RenderStyle::isCollapsibleWhitespace, which gives the
correct answer. It is almost never correct to call the global
isCollapsibleWhitespace function.

Also, the two line for body needs to have braces around it.

It is also unnecessarily inefficient to put the text into a string object. I
would instead do this:

    const UChar* characters = renderer->text()->characters();

And then index directly into the characters array.

Also, for local variables we try to use words rather than abbreviations. The
"i" is OK, but "str" is not so great. I also think "length" is fine, since the
length is the length of the text.

To test the different whitespace modes, we need test cases that cover editable
text with the CSS white-space values, normal, pre, pre-wrap, pre-line, and
nowrap, covering both spaces and newline characters, and ideally tabs too,
although they should basically work the same as spaces.

> +    // We expand range to include trailing (collapsed) whitespaces, 
> +    // which is required for word-boundary detection.
> +    if (m_node != m_endNode)
> +        m_positionEndOffset += collapsedSpaceLength(m_node, m_positionEndOffset);

I'm not entirely sure this change is OK. It's kind of strange to hardcode this
behavior down at this low level. For example, there can be leading whitespace
too. Why include trailing whitespace but not leading? But I'm willing to accept
that we need the change if there are enough test cases, including cases where
the newline is just before the word, for example. The testing right now is way
too light.

review- because of the multiple issues above

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