Bug 52686

Summary: Setting outerHTML should merge text nodes
Product: WebKit Reporter: Emil A Eklund <eae>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, ayg, commit-queue, eric, tony
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226808
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Emil A Eklund 2011-01-18 17:00:15 PST 
When setting outerHTML adjacent text nodes should be merged. Setting outerText already has this behavior as does the IE implementation of setting outerHTML.
Comment 1 Emil A Eklund 2011-01-18 17:14:40 PST 
Created attachment 79359 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-01-19 00:43:10 PST 
> This matches the behavior of outerText and the IE implementation of outerHTML.

Does HTML5 specify such behavior?
Comment 3 Emil A Eklund 2011-01-19 12:33:01 PST 
> Does HTML5 specify such behavior?

No, HTML5 doesn't specify how text nodes should be handled when setting outerHTML and doesn't cover outerText at all.
Comment 4 Adam Barth 2011-01-19 18:59:43 PST 
Comment on attachment 79359 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:386
>      HTMLElement* parent = static_cast<HTMLElement*>(p);

We don't need to hold a reference to parent here?  It's strange that we re-check parentNode for null below but don't hold a ref here.
Comment 5 Emil A Eklund 2011-01-19 19:25:33 PST 
Comment on attachment 79359 [details]
Patch

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

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

> 
> We don't need to hold a reference to parent here?  It's strange that we re-check parentNode for null below but don't hold a ref here.

Good point, I'll change it to hold a reference to the parent node instead.
Comment 6 Emil A Eklund 2011-01-19 19:57:33 PST 
Created attachment 79543 [details]
Patch
Comment 7 Emil A Eklund 2011-01-26 10:49:27 PST 
Ping?
Comment 8 Tony Chang 2011-04-26 15:26:19 PDT 
Comment on attachment 79543 [details]
Patch

Seems fine.
Comment 9 Tony Chang 2011-04-26 15:27:12 PDT 
Oh, are we worried that we don't collapse chains of text nodes or should we not get into that state because we collapse at every change?  What does IE do in this case?
Comment 10 Emil A Eklund 2011-04-26 15:46:07 PDT 
We shouldn't be able to get into that state. If you can think of any case where we could I'd love to hear about it!

IE always collapses adjacent text nodes.
Comment 11 Emil A Eklund 2011-04-29 16:37:24 PDT 
Created attachment 91765 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2011-04-29 17:23:12 PDT 
Comment on attachment 91765 [details]
Patch for landing

Clearing flags on attachment: 91765

Committed r85374: <http://trac.webkit.org/changeset/85374>
Comment 13 WebKit Commit Bot 2011-04-29 17:23:18 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 14 Aryeh Gregor 2011-05-02 15:42:58 PDT 
Could you please file bugs against HTML5 when you find problems with it, instead of just changing behavior and letting the spec remain incorrect?  You don't have to make an account or anything.  Just go to http://whatwg.org/html5/, find the right part of the spec, and file a comment in the little box at the bottom.

I just filed a bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12584
Comment 15 Emil A Eklund 2011-05-03 13:47:37 PDT 
Certainly. Thanks for the pointer.