Move innerHTML and outerHTML to Element

Move innerHTML and outerHTML to Element

Per the DOM Parsing specification, innerHTML and outerHTML should be
available on Element, not HTMLElement. Since HTMLElement and SVGElement
are the only interfaces that inherit from Element, the only Web visible
difference should be that innerHTML and outerHTML are now available for
SVG elements.

For <svg>.innerHTML = "<!-- svg elements here -->" to insert elements
with correct namespace URI, one adjustment has to be made to the tree
builder step of the html parsing algorithm. The changes are described in
the spec change <http://html5.org/r/7768>;. The new concept of
'adjustedCurrentNode' is used when processing and when deciding to
process foreign content and makes sure that parsed elements go into the
correct namespace.

Some layout tests that (perhaps accidentally) use innerHTML and
outerHTML on SVG elements are adjusted slightly.

BUG=311080
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160869

[davve, 2013-10-25 13:53:33]

This started out as a simple fix but grew slightly more complex as I wanted to
avoid creating https://bugzilla.mozilla.org/show_bug.cgi?id=886390 in Blink.

I believe this would make Blink the first browser engine to fully support
(AFAIK) innerHTML and outerHTML on SVG elements.

It would be useful to get some try-bot runs on this. Perhaps there are issues I
can address before you start reviewing properly.

[Erik Dahlström (inactive), 2013-10-25 14:31:36]

On 2013/10/25 13:53:33, davve wrote:
> This started out as a simple fix but grew slightly more complex as I wanted to
> avoid creating https://bugzilla.mozilla.org/show_bug.cgi?id=886390 in Blink.
> 
> I believe this would make Blink the first browser engine to fully support
> (AFAIK) innerHTML and outerHTML on SVG elements.
> 
> It would be useful to get some try-bot runs on this. Perhaps there are issues
I
> can address before you start reviewing properly.

I think this will make many svg-authors happy, so a +1 from me on the direction
of this patch.

How about insertAdjacentHTML?
http://domparsing.spec.whatwg.org/#insertadjacenthtml()

I'm not sure if this is an exhaustive list, but should the aim be to also handle
innerText/outerText/insertAdjacentText on Element instead of HTMLElement? (to be
clear, I don't think this patch should do that, but it could be considered for a
followup task)

[eseidel, 2013-10-25 17:39:45]

So this will use the xml parser when in an xml doc and the HTML parser in an
html doc, correct?

Uncomfortably exciting. :)

[pdr., 2013-10-25 17:57:13]

Wow, this is fantastic.

1x LGTM.

@eseidel or @abarth, can you give an additional LGTM for the parser changes?

[davve, 2013-10-25 18:00:09]

On 2013/10/25 17:39:45, eseidel wrote:
> So this will use the xml parser when in an xml doc and the HTML parser in an
> html doc, correct?

Correct. document.isHTMLDocument() means the HTML parser; otherwise the XML
parser is used (for details, see createFragmentForInnerOuterHTML in markup.cpp).
I didn't have to make any changes to that part since innerHTML has always worked
for xhtml. What we might want to do here is to add some xml-based tests as well.

[arv (Not doing code reviews), 2013-10-25 19:15:31]

This is great.

https://codereview.chromium.org/44333002/diff/1/LayoutTests/fast/innerHTML/in...
File LayoutTests/fast/innerHTML/innerHTML-svg-read.html (right):

https://codereview.chromium.org/44333002/diff/1/LayoutTests/fast/innerHTML/in...
LayoutTests/fast/innerHTML/innerHTML-svg-read.html:31: shouldBe(tests[i][0],
tests[i][1]);
FYI. If you used shouldBeEqualToString you could skip one level of escaping

[davve, 2013-10-25 19:50:44]

https://codereview.chromium.org/44333002/diff/1/LayoutTests/fast/innerHTML/in...
File LayoutTests/fast/innerHTML/innerHTML-svg-read.html (right):

https://codereview.chromium.org/44333002/diff/1/LayoutTests/fast/innerHTML/in...
LayoutTests/fast/innerHTML/innerHTML-svg-read.html:31: shouldBe(tests[i][0],
tests[i][1]);
On 2013/10/25 19:15:31, arv wrote:
> FYI. If you used shouldBeEqualToString you could skip one level of escaping
Thank you!

[davve, 2013-10-28 16:54:49]

On 2013/10/25 18:00:09, davve wrote:
> What we might want to do here is to add some xml-based tests as well.

I have now added an XHTML test and indeed found an (existing) bug. When creating
elements with createElementNS and appending them to DOM, fragment parsing will
not pick up the namespace of the dynamically created element, because the
XMLDocumentParser constructor for fragments only look at the namespace
attributes and assumes an attribute was needed to switch namespace from parent
to child. Reported as http://crbug.com/312320. It is possible this patch makes
the bug more visible but I don't think we should consider it a blocker.

[abarth-chromium, 2013-10-29 04:04:41]

This generally looks good, but I'd like to look at the CL after you fix the two
issues below.

https://codereview.chromium.org/44333002/diff/240001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/44333002/diff/240001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2224: container =
toHTMLTemplateElement(this)->content();
This is a bad cast.  Just because an element has a local name of templateTag
doesn't mean that it's an HTMLTemplateElement.  You need to check the full
qualified name.  Perhaps call hasTagName(templateTag) ?  Please add a test for
this bug.

https://codereview.chromium.org/44333002/diff/240001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2244: parent->replaceChild(fragment.release(), this,
es);
We don't need to protect |this| during this function?  I suppose we've had this
code for a while so it's not likely to be a problem.

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.cpp (left):

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:335: container =
toHTMLTemplateElement(this)->content();
This check used to be ok because we already knew that |this| was an HTMLElement.

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:381: RefPtr<HTMLStackItem>
contextNodeRef;
contextNodeRef  <-- What does the "Ref" in this name mean?  Perhaps just
contextNode?

Why do we even need this variable?

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:1004: HTMLStackItem*
HTMLTreeBuilder::adjustedCurrentStackItem(RefPtr<HTMLStackItem>&
contextStackItem) const
Why not just return a PassRefPtr<HTMLStackItem>.  Then you don't need this crazy
parameter.

[davve, 2013-10-29 09:15:37]

https://codereview.chromium.org/44333002/diff/240001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/44333002/diff/240001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2224: container =
toHTMLTemplateElement(this)->content();
On 2013/10/29 04:04:42, abarth wrote:
> This is a bad cast.

*blush*. Good eyes. I've added a crash test.

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/44333002/diff/240001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:1004: HTMLStackItem*
HTMLTreeBuilder::adjustedCurrentStackItem(RefPtr<HTMLStackItem>&
contextStackItem) const
On 2013/10/29 04:04:42, abarth wrote:
> Why not just return a PassRefPtr<HTMLStackItem>.  Then you don't need this
crazy
> parameter.

Just me showing my inexperience with the RefPtr-family. (re-reads
http://www.webkit.org/coding/RefPtr.html ). Thanks for the sanity check, the
function now returns a PassRefPtr<HTMLStackItem>.

[abarth-chromium, 2013-10-29 16:27:46]

LGTM

Thanks!

[commit-bot: I haz the power, 2013-10-29 16:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/44333002/300001

[commit-bot: I haz the power, 2013-10-29 16:28:05]

Failed to apply patch for Source/core/dom/Element.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Element.h
  Hunk #1 FAILED at 82.
  1 out of 1 hunk FAILED -- saving rejects to file Source/core/dom/Element.h.rej

Patch:       Source/core/dom/Element.h
Index: Source/core/dom/Element.h
diff --git a/Source/core/dom/Element.h b/Source/core/dom/Element.h
index
547ba773a4d42afd11c0369e8aa9097f456d33c2..1ad1a4be3e0e0aeacd875842b239eb06a68bce16
100644
--- a/Source/core/dom/Element.h
+++ b/Source/core/dom/Element.h
@@ -82,6 +82,11 @@ public:
     static PassRefPtr<Element> create(const QualifiedName&, Document*);
     virtual ~Element();
 
+    String innerHTML() const;
+    String outerHTML() const;
+    void setInnerHTML(const String&, ExceptionState&);
+    void setOuterHTML(const String&, ExceptionState&);
+
     DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);
     DEFINE_ATTRIBUTE_EVENT_LISTENER(beforecopy);
     DEFINE_ATTRIBUTE_EVENT_LISTENER(beforecut);

[commit-bot: I haz the power, 2013-10-29 19:23:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davve@opera.com/44333002/370001

[commit-bot: I haz the power, 2013-10-29 23:19:59]

Change committed as 160869

[eseidel, 2013-11-12 21:05:22]

https://codereview.chromium.org/44333002/diff/370001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/44333002/diff/370001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:2691: RefPtr<HTMLStackItem>
adjustedCurrentNode = adjustedCurrentStackItem();
This change looks like it's adding a malloc here just to check the namespace... 
That seems very bad.

I think http://jsperf.com/parse-html-type may be affected by this change.  I am
pretty sure we have some PerformanceTests/ for innerHTML already which I would
expect to show this malloc?

[davve, 2013-11-13 13:15:16]

On 2013/11/12 21:05:22, eseidel wrote:
>
https://codereview.chromium.org/44333002/diff/370001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLTreeBuilder.cpp (right):
> 
>
https://codereview.chromium.org/44333002/diff/370001/Source/core/html/parser/...
> Source/core/html/parser/HTMLTreeBuilder.cpp:2691: RefPtr<HTMLStackItem>
> adjustedCurrentNode = adjustedCurrentStackItem();
> This change looks like it's adding a malloc here just to check the
namespace... 
> That seems very bad.

Good catch! I've reported
https://code.google.com/p/chromium/issues/detail?id=318711 to investigate this
further.