Bug 52268 - Setting outerText should convert CR/LF to <br>
Summary: Setting outerText should convert CR/LF to <br>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 18:08 PST by Emil A Eklund
Modified: 2011-01-13 14:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.74 KB, patch)
2011-01-11 18:09 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2011-01-11 20:22 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (12.58 KB, patch)
2011-01-12 14:52 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (12.37 KB, patch)
2011-01-12 14:53 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (12.02 KB, patch)
2011-01-12 18:26 PST, Emil A Eklund
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (13.32 KB, patch)
2011-01-13 11:34 PST, Emil A Eklund
dglazkov: review+
Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2011-01-13 13:47 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2011-01-11 18:08:23 PST
When setting Element.outerText line breaks in the text should be converted to <br> elements.
Comment 1 Emil A Eklund 2011-01-11 18:09:52 PST
Created attachment 78634 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-11 18:13:48 PST
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.
Comment 3 Emil A Eklund 2011-01-11 20:22:35 PST
Created attachment 78645 [details]
Patch

Fixed style violations in moved code.
Comment 4 Eric Seidel (no email) 2011-01-11 23:10:07 PST
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?
Comment 5 Emil A Eklund 2011-01-12 14:52:29 PST
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.
Comment 6 Emil A Eklund 2011-01-12 14:53:41 PST
Created attachment 78736 [details]
Patch

Removed unnecessary import.
Comment 7 Eric Seidel (no email) 2011-01-12 15:06:40 PST
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.
Comment 8 Emil A Eklund 2011-01-12 16:15:38 PST
Thanks for the feedback Eric, I'll do another round and try to clean it up some more.
Comment 9 Emil A Eklund 2011-01-12 18:26:35 PST
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.
Comment 10 Eric Seidel (no email) 2011-01-12 18:47:57 PST
Comment on attachment 78769 [details]
Patch

Thanks.
Comment 11 WebKit Commit Bot 2011-01-12 18:49:27 PST
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
Comment 12 Emil A Eklund 2011-01-13 11:34:14 PST
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.
Comment 13 WebKit Commit Bot 2011-01-13 12:22:06 PST
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
Comment 14 Emil A Eklund 2011-01-13 13:47:24 PST
Created attachment 78851 [details]
Patch

Made mergeWithNextTextNode function static to make XCode happy.
Comment 15 Dimitri Glazkov (Google) 2011-01-13 13:52:34 PST
Comment on attachment 78851 [details]
Patch

Let's try again! :)
Comment 16 Eric Seidel (no email) 2011-01-13 13:53:14 PST
Comment on attachment 78851 [details]
Patch

Yay!
Comment 17 WebKit Commit Bot 2011-01-13 14:48:24 PST
The commit-queue encountered the following flaky tests while processing attachment 78851 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2011-01-13 14:49:58 PST
Comment on attachment 78851 [details]
Patch

Clearing flags on attachment: 78851

Committed r75738: <http://trac.webkit.org/changeset/75738>
Comment 19 WebKit Commit Bot 2011-01-13 14:50:05 PST
All reviewed patches have been landed.  Closing bug.