Bug 23232 – Problems with Selection::appendTrailingWhitespace()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Roman]

There are some problems with Selection::appendTrailingWhitespace():

1. May set endpoint to null node, causing crash in WebCore::Selection::toRange.

(This is because VisiblePosition::next() doesn't necessarily advance to same place as VisiblePosition::characterAfter(), especially if it is a non-visible character.).

2. Should match NBSP as whitespace.

3. Should search for whitepsace in sibling nodes too.
For example:
  <div><span>doubleclickme</span> </div>
Should grow selection to include the whitespace outside of the span element (to be consistent with IE/FF).

<background>
Selection::appendTrailingWhitespace() is a codepath used only by Chromium, see http://trac.webkit.org/changeset/38735 which introduced it.
</background>

[Eric Roman]

Created attachment 26594 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

* Modified LayoutTests/editing/selection/doubleclick-whitespace.html to include additional test cases.
* Added a separate layout test for the crasher.

[Darin Adler]

Comment on attachment 26594 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

> +    Node *n = pos.node();

WebKit coding style requires that you do Node* n rather than Node *n.

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

Same here.

> +    Node *boundary = n->enclosingBlockFlowElement();

And here.

> +    if (!searchRange.get())

There's no need for .get() here. You can use the ! operator with a RefPtr without calling it.

> +    for (; !charIt.atEnd() && charIt.length() > 0; charIt.advance(1)) {

There's no need to check atEnd() here. Once you're at the end, length is guaranteed to return 0. In WebKit style we'd normally code this without the "> 0" also.

> +        if (isSpaceOrNewline(c) || c == noBreakSpace)
> +            m_end = charIt.range()->endPosition();
> +        else
> +            break;

Maybe the should be written the other way around, with the break first, and the m_end part outside the if statement, to make the looping logic easier to understand?

Is there a reason the tab character isn't included here? As you can see in RenderStyle::isCollapsibleWhiteSpace, it's considered a form of space.

Do you need to handle newlines that actually render as hard line breaks differently from newlines that are treated as general whitespace? I guess your test 4 is supposed to be implementing that.

Patch looks good and it's *almost* a review+, but I'd like you to resolve the issues I mention above and post a new patch if you don't mind.

[Eric Roman]

Created attachment 26627 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

Thanks for the review comments!


> WebKit coding style requires that you do Node* n rather than Node *n.

Fixed.
Sorry about that.
I lifted this code from visible_units.cpp (nextBoundary), which must be using an older style.

> There's no need for .get() here. You can use the ! operator with a RefPtr
> without calling it.

Done.

> There's no need to check atEnd() here. Once you're at the end, length is
> guaranteed to return 0. In WebKit style we'd normally code this without the ">
> 0" also.

Done.
Changed to:

  for (; charIt.length(); charIt.advance(1)) {

> Maybe the should be written the other way around, with the break first, and the
> m_end part outside the if statement, to make the looping logic easier to
> understand?

Done.
Moved the break case to be first, so no "else" is needed.
Much cleaner, thanks for the suggestion.

As for moving the m_end outside of the loop, I have not done that since it hurts readability some, for the case where there is no whitespace (and m_end is not to be changed).

> Is there a reason the tab character isn't included here? As you can see in
> RenderStyle::isCollapsibleWhiteSpace, it's considered a form of space.

The tab character is in fact included -- it is matched by "isSpaceOrNewline(c)".

Added "test6" to "doubleclick-whitespace.html",
to ensure that tabs (0x9) are included in the selection.

[Darin Adler]

Comment on attachment 26627 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

r=me

[Eric Roman]

Created attachment 26635 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

Updated the patch to fix some indentation in doubleclick-whitespace.html.

There were two javascript functions where I had 2 space indentation instead of 4 space indentation.

This is the only change over the previous (reviewed) patch.

[Dimitri Glazkov (Google)]

http://trac.webkit.org/changeset/39831