Bug 52268 – Setting outerText should convert CR/LF to <br>

• Component: DOM
• Status: RESOLVED FIXED

[Emil A Eklund]

When setting Element.outerText line breaks in the text should be converted to <br> elements.

[Emil A Eklund]

Created attachment 78634 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/HTMLElement.cpp:379:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/html/HTMLElement.cpp:391:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/html/HTMLElement.cpp:393:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/html/HTMLElement.cpp:396:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/html/HTMLElement.cpp:493:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/html/HTMLElement.cpp:506:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 6 files


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

[Emil A Eklund]

Created attachment 78645 [details]
Patch

Fixed style violations in moved code.

[Eric Seidel (no email)]

Comment on attachment 78645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78645&action=review

> Source/WebCore/html/HTMLElement.cpp:381
> +    ec = 0;

I think this is generally the callers responsibility in WebCore.

> Source/WebCore/html/HTMLElement.cpp:390
> +                fragment->appendChild(Text::create(document(), text.substring(lineStart, i - lineStart)), ec);

If this can run arbitrary javascript, "this" could get deleted, no?  Do we need to suspend mutation events during this?

> Source/WebCore/html/HTMLElement.cpp:399
> +            lineStart = i + 1;

I find it difficult to read this loop and understand what its doing.  I can't tell if that's a variable naming problem, the way the blocks are split up, or just my own thick-headedness at this hour.

> Source/WebCore/html/HTMLElement.cpp:498
> +        textPrev->appendData(textNode->data(), ec);

Does this cause JS to run?  If so, our pointers could go invalid.

> Source/WebCore/html/HTMLElement.cpp:511
> +        RefPtr<Text> textNext = static_cast<Text*>(next.get());
> +        RefPtr<Text> textNode = static_cast<Text*>(node);
> +        textNode->appendData(textNext->data(), ec);

Seems we just did this above.  Maybe there is code to share here?

[Emil A Eklund]

Created attachment 78735 [details]
Patch

Thanks for the review, see comments inline.


> > Source/WebCore/html/HTMLElement.cpp:381
> > +    ec = 0;
> I think this is generally the callers responsibility in WebCore.

Thanks, fixed.

> > Source/WebCore/html/HTMLElement.cpp:390
> > +                fragment->appendChild(Text::create(document(), text.substring(lineStart, i - lineStart)), ec);
>
> If this can run arbitrary javascript, "this" could get deleted, no?  Do we need to suspend mutation events during this?

As the fragment isn't attached to the document yet I don't see how one would listen to that mutation event. The text-node-append-data-remove-crash.html tests this.

> > Source/WebCore/html/HTMLElement.cpp:399
> > +            lineStart = i + 1;
> I find it difficult to read this loop and understand what its doing.  I can't tell if that's a variable
> naming problem, the way the blocks are split up, or just my own thick-headedness at this hour.

I agree, it's not the easiest code to read. I moved it out of the setInnerText method and didn't want to make too many changes to it.

> > Source/WebCore/html/HTMLElement.cpp:498
> > +        textPrev->appendData(textNode->data(), ec);
>
> Does this cause JS to run?  If so, our pointers could go invalid.

We hold RefPtrs for both nodes so it should be safe. This code has been replaced with a call to mergeWithNextTextNode in the latest patch.

> > Source/WebCore/html/HTMLElement.cpp:511
> > +        RefPtr<Text> textNext = static_cast<Text*>(next.get());
> > +        RefPtr<Text> textNode = static_cast<Text*>(node);
> > +        textNode->appendData(textNext->data(), ec);
> Seems we just did this above.  Maybe there is code to share here?

Good idea, broke out the merging logic into a helper funciton.

[Emil A Eklund]

Created attachment 78736 [details]
Patch

Removed unnecessary import.

[Eric Seidel (no email)]

Comment on attachment 78736 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78736&action=review

I think the biggest trouble with this patch is that you're inheriting some less-than-perfectly designed code, which has historically been poorly tested.  It's difficult for me to draw the line between your current definite improvement of that code, and the ideal for said code.  This current iteration is better than your last one, and *certainly* way beter than what we had before.  If I were writing this patch, I'd want to go one more round.  But I'm also open to the idea of landing this and iterating further in a separate patch or at a later time.

> Source/WebCore/html/HTMLElement.cpp:387
> +        if (c == '\n' || c == '\r') {

I feel like this should be an early contineue, but that gets a bit ugly with the need for prev = c.

> Source/WebCore/html/HTMLElement.cpp:403
> +    if (length > lineStart)
> +        fragment->appendChild(Text::create(document(), text.substring(lineStart, length - lineStart)), ec);

This is repeated from above, but missing the ec check.  Is that intentional?  how do we exercise this case?

> Source/WebCore/html/HTMLElement.cpp:460


Should just early return instead of making a long if block.

> Source/WebCore/html/HTMLElement.cpp:498
> +    RefPtr<Node> prev = previousSibling();
> +    RefPtr<Node> next = nextSibling();
> +    if (text.isEmpty() && (!prev || !prev->isTextNode()) && (!next || !next->isTextNode())) {
> +        parent->replaceChild(Text::create(document(), ""), this, ec);
> +        return;
> +    }

I'm not sure I understand this quirk or why it needs to be a separate if.  I assume it's tested?

> Source/WebCore/html/HTMLElement.cpp:506
> +    if (text.contains('\r') || text.contains('\n'))
> +        newChild = textToFragment(text, ec);
> +    else
> +        newChild = Text::create(document(), text);

I would just have put this if inside textToFragment, making ti just reutrn a Text node if there are no \n, \r.

[Emil A Eklund]

Thanks for the feedback Eric, I'll do another round and try to clean it up some more.

[Emil A Eklund]

Created attachment 78769 [details]
Patch

Rewrote the textToFragment method and made all the changes you suggested except for your last comment about making textToFragment return a text node for strings without a line break. The textToFragment method is used by setInnerText which requires a fragment. I didn't want to make this patch any larger by changing that but I'd be happy to make that change in a later patch if you think it's worthwhile.

[Eric Seidel (no email)]

Comment on attachment 78769 [details]
Patch

Thanks.

[WebKit Commit Bot]

Comment on attachment 78769 [details]
Patch

Rejecting attachment 78769 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
outTests/fast/dom/set-outer-text.html
patching file LayoutTests/fast/dom/text-node-append-data-remove-crash-expected.txt
(Stripping trailing CRs from patch.)
patching file LayoutTests/fast/dom/text-node-append-data-remove-crash.html
Hunk #1 FAILED at 13.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/dom/text-node-append-data-remove-crash.html.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7592004

[Emil A Eklund]

Created attachment 78835 [details]
Patch

Converted line endings for text-node-append-data-remove-crash.html to unix style in order to make the submit queue happy.

[WebKit Commit Bot]

Comment on attachment 78835 [details]
Patch

Rejecting attachment 78835 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build'..." exit_code: 2

Last 500 characters of output:
DE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/HTMLElement.o /mnt/git/webkit-commit-queue/Source/WebCore/html/HTMLElement.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/7503017

[Emil A Eklund]

Created attachment 78851 [details]
Patch

Made mergeWithNextTextNode function static to make XCode happy.

[Dimitri Glazkov (Google)]

Comment on attachment 78851 [details]
Patch

Let's try again! :)

[Eric Seidel (no email)]

Comment on attachment 78851 [details]
Patch

Yay!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 78851 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 78851 [details]
Patch

Clearing flags on attachment: 78851

Committed r75738: <http://trac.webkit.org/changeset/75738>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.