Bug 52686 - Setting outerHTML should merge text nodes
: Setting outerHTML should merge text nodes
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-18 17:00 PST by Emil A Eklund
Modified: 2011-05-03 13:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.88 KB, patch)
2011-01-18 17:14 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2011-01-19 19:57 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (7.01 KB, patch)
2011-04-29 16:37 PDT, 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-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.