Extra layout on keypress after a space (problem with rebalanceWhitespaceAt in InsertTextCommand)

• Component: HTML Editing
• Status: REOPENED ---

[Justin Garcia]

When a insert a character after a space, we do two layouts:

insertTextIntoNode(textNode, offset, text); => setNeedsLayout
rebalanceWhitespaceAt(startPosition); => replaceTextInNode => updateLayout
then
rebalanceWhitespaceAt(startPosition); => replaceTextInNode => setText => setNeedsLayout
setEndingSelection(Selection(endingSelection().end(), endingSelection().affinity()))) => updateLayout

I think we have two ways to go here:

Make the decision about what we need to turn adjacent spaces into before we do any DOM operations (since this requires a fresh layout), and do whitespace modifications with all of the other DOM operations that the command performs.
Turn all spaces into non-breaking spaces (I believe we can do this because of the non-breaking-space-mode:space we have on editable regions) and then turn them into patterns at serialization time.

ojan Vafai <[e-mail]>

[Justin Garcia]

<rdar://problem/6744306>

[Justin Garcia]

Created attachment 43452 [details]
patch

[Darin Adler]

Comment on attachment 43452 [details]
patch

Looks pretty good. I have a few thoughts.

I am concerned about copying the function named isWhitespace in two different source files. Can we put this into PlatformString.h or StringImpl.h instead? What's the right name for it to distinguish it from other functions of similar name?

> +void InsertTextCommand::insertTextIntoNodeAndRebalanceWhitespace(const String& textToInsert, PassRefPtr<Text> node, unsigned offset)

This function does not take ownership of the text node, so the argument type should be Text*, not PassRefPtr<Text>.

> +    unsigned whitespaceStart, whitespaceEnd;
> +    if (!extentOfWhitespaceAt(node.get(), offset, whitespaceStart, whitespaceEnd)) {
> +    
> +        VisiblePosition visiblePosition(Position(node.get(), offset));
> +        String rebalancedText = stringWithRebalancedWhitespace(textToInsert, isStartOfParagraph(visiblePosition), isEndOfParagraph(visiblePosition));
> +    
> +        insertTextIntoNode(node.get(), offset, rebalancedText);
> +        return;
> +    }

There should not be a blank line after the if statement.

> +    
> +    ASSERT(whitespaceEnd >= whitespaceStart);
> +    unsigned length = whitespaceEnd - whitespaceStart + 1;

Why the +1 here? It seems like the "end" value may be inclusive, which seems like a mistake.

I can't tell by reading this.

> +    void insertTextIntoNodeAndRebalanceWhitespace(const String& text, PassRefPtr<Text> node, unsigned offset);

As I said above, the type of the node should be Text*. The names "text" and "node" for the arguments are justified here because of the clash of type names with roles of the arguments. The argument names help make it clear.

> +// Finds the extent of whitespace around a particular offset, as long as it is in a node with collapsable whitespace.

The word "collapsible" is misspelled here.

The comment could be clearer on what "as long as" means.

There are multiple modes of collapsible whitespace. For example, pre-wrap collapses other whitespace, but not "\n". To handle those correctly, I think RenderStyle::isCollapsibleWhiteSpace needs to be used instead of a local isWhiteSpace function.

> +    if (node->length() == 0)
> +        return false;

Do we really need a special case for empty text nodes? Is this a common case that needs performance optimization? Does the code below fail for empty text nodes?

I'd like to see an assertion about offset being <= node->length() and also get a sense of whether callers really guarantee that or not.

> +    if (offset == text.length() || !isWhitespace(text[offset])) {

Because of how String indexing works, I don't think you need the "offset == text.length" check here. Getting a character past the end returns 0, and that will return false from isWhitespace.

> +        if (offset == 0 || !isWhitespace(text[offset - 1]))
> +            // If neither text[offset] nor text[offset - 1] are some form of whitespace, no rebalancing is necessary.
> +            return false;

Need braces around this.

> +    while (start > 0 && isWhitespace(text[start - 1]))
> +        start--;
> +        
> +    for (unsigned i = end + 1; i < text.length(); i++) {
> +        if (!isWhitespace(text[i]))
> +            break;
> +        end = i;
> +    }

I don't understand why these loops are structured differently, since they both do the same thing. It would be slightly nicer to make them match.

I don't understand how this function can possibly do the right thing if there are adjacent text nodes that also have whitespace in them.

review- because at least some of my comments seem worth doing, especially the use of RenderStyle::isColllapsibleWhiteSpace

I'd like to see more test cases -- I worry that we are making performance faster but correctness suffer in various edge cases.

[Justin Garcia]

Created attachment 43622 [details]
patch

> I am concerned about copying the function named isWhitespace in two different
> source files. 

Fixed, it didn't need to be in InsertTextCommand at all.

> Can we put this into PlatformString.h or StringImpl.h instead?
> What's the right name for it to distinguish it from other functions of similar
> name?

It's a strange function, I'd rather leave it in htmlediting.cpp for now.  I'm not sure that it needs to check for tabs and newlines, since those will be removed as insignificant whitespace before we get to whitespace rebalancing.  But this is part of the old code that I'd like to address after.

> > +void InsertTextCommand::insertTextIntoNodeAndRebalanceWhitespace(const String& textToInsert, PassRefPtr<Text> node, unsigned offset)
> 
> This function does not take ownership of the text node, so the argument type
> should be Text*, not PassRefPtr<Text>.

OK.  But when does a function take ownership of the node?  insertTextIntoNode(...) takes in a PassRefPtr<Node>, even though it seems like it only calls a function that takes ownership of the node (the create function).

> There should not be a blank line after the if statement.

FIxed.

> > +    ASSERT(whitespaceEnd >= whitespaceStart);
> > +    unsigned length = whitespaceEnd - whitespaceStart + 1;
> 
> Why the +1 here? It seems like the "end" value may be inclusive, which seems
> like a mistake.

whitespaceEnd was strangely the offset *of* the last whitespace character, not the offset after the last whitespace character, thus the plus 1.  Fixed this.

> > +// Finds the extent of whitespace around a particular offset, as long as it is in a node with collapsable whitespace.
> 
> The word "collapsible" is misspelled here.
> 
> The comment could be clearer on what "as long as" means.
> 
> There are multiple modes of collapsible whitespace. For example, pre-wrap
> collapses other whitespace, but not "\n". To handle those correctly, I think
> RenderStyle::isCollapsibleWhiteSpace needs to be used instead of a local
> isWhiteSpace function.

The comment now reads: "Find the extent of whitespace around a particular offset.  If whitespace characters (' ') are preserved, then we don't do whitespace rebalancing and this function returns false." and the function name is "extentOfWhitespaceForRebalancingAt".

> Do we really need a special case for empty text nodes? Is this a common case
> that needs performance optimization? Does the code below fail for empty text
> nodes?

We don't need it, I'll remove it.

> I'd like to see an assertion about offset being <= node->length() and also get
> a sense of whether callers really guarantee that or not.

They can't.  They get their offsets from VisiblePositions, which can technically have invalid offsets.

> > +    if (offset == text.length() || !isWhitespace(text[offset])) {
> 
> Because of how String indexing works, I don't think you need the "offset ==
> text.length" check here. Getting a character past the end returns 0, and that
> will return false from isWhitespace.

Seems strange to rely on this detail, and no index checking would look odd since it looks like straight array indexing at first glance.

> > +    while (start > 0 && isWhitespace(text[start - 1]))
> > +        start--;
> > +        
> > +    for (unsigned i = end + 1; i < text.length(); i++) {
> > +        if (!isWhitespace(text[i]))
> > +            break;
> > +        end = i;
> > +    }
> 
> I don't understand why these loops are structured differently, since they both
> do the same thing. It would be slightly nicer to make them match.

Fixed.

> I'd like to see more test cases -- I worry that we are making performance
> faster but correctness suffer in various edge cases.

Added a few test cases that insert content in and around multiple spaces.

[Enrica Casucci]

Created attachment 49506 [details]
Patch2

I took the updated patch from Justin that addresses Darin's comments and merge it.

[WebKit Review Bot]

Attachment 49506 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/htmlediting.cpp:447:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

Created attachment 49510 [details]
Patch3

Fixed style issue.

[Enrica Casucci]

Committed revision 55263.

[Csaba Osztrogonác]

This patch broke fast/forms/textarea-type-spaces-pretty-diff.html test on all buildbot.

Tiger: http://build.webkit.org/results/Tiger%20Intel%20Release/r55265%20%289128%29/fast/forms/textarea-type-spaces-pretty-diff.html
Leopard(debug): http://build.webkit.org/results/Leopard%20Intel%20Debug%20%28Tests%29/r55264%20%2810851%29/fast/forms/textarea-type-spaces-pretty-diff.html

[Csaba Osztrogonác]

I had to roll out r55263 to make buildbots and commit bot happier: http://trac.webkit.org/changeset/55271 .

[Csaba Osztrogonác]

The patch broke fast/events/key-events-in-input-text.html too: 

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r55270%20%286022%29/fast/events/key-events-in-input-text-pretty-diff.html

[Enrica Casucci]

(In reply to comment #11)
> The patch broke fast/events/key-events-in-input-text.html too: 
> 
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r55270%20%286022%29/fast/events/key-events-in-input-text-pretty-diff.html

I'm aware of the problem and I'm preparing a patch to fix the failing tests.

[Eric Seidel (no email)]

I'm unsure what the status of this bug is.  It looks like this got landed and then rolled out.  Presumably it was landed again?  Should this bug be closed?

[Ryosuke Niwa]

(In reply to comment #13)
> I'm unsure what the status of this bug is.  It looks like this got landed and then rolled out.  Presumably it was landed again?  Should this bug be closed?

I don't think it was landed again as far as http://trac.webkit.org/browser/trunk/Source/WebCore/editing/CompositeEditCommand.cpp?annotate=blame&rev=75835 tells me.

[Darin Adler]

Comment on attachment 49510 [details]
Patch3

Bug is still open. Need to find a way to fix this.