Bug 23232

Summary: Problems with Selection::appendTrailingWhitespace()
Product: WebKit Reporter: Eric Roman <eroman>
Component: WebCore Misc.Assignee: Eric Roman <eroman>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Rewrite appendTrailingWhitespace() using CharacterIterator.
darin: review-
Rewrite appendTrailingWhitespace() using CharacterIterator.
darin: review+
Rewrite appendTrailingWhitespace() using CharacterIterator. none

Description Eric Roman 2009-01-10 17:22:40 PST
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>
Comment 1 Eric Roman 2009-01-10 17:36:16 PST
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.
Comment 2 Darin Adler 2009-01-11 08:39:05 PST
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.
Comment 3 Eric Roman 2009-01-12 02:01:33 PST
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.
Comment 4 Darin Adler 2009-01-12 09:03:34 PST
Comment on attachment 26627 [details]
Rewrite appendTrailingWhitespace() using CharacterIterator.

r=me
Comment 5 Eric Roman 2009-01-12 10:44:30 PST
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.
Comment 6 Dimitri Glazkov (Google) 2009-01-12 13:08:54 PST
http://trac.webkit.org/changeset/39831