Default NamepaceURI must be gotten from the topmost parent before the SVG <foreignObject>

[Said Abou-Hallawa]

Created attachment 382855 [details]
test case

Open the attached test case.

Expected: The svg should have two <foreignObject> elements. Inside each element there should be an <h2> element followed by a <table> element.
Result: There is only one <foreignObject> element visible. The other element is not displayed.

In the WebInspector copy the second <foreignObject> to the clipboard and paste it in a text editor. This will be the result. Notice the wrong namespace in the tags <h2> and <table>

<?xml version="1.0"?>
<foreignObject xmlns="http://www.w3.org/2000/svg" x="200" width="200" height="100">
  <div xmlns="http://www.w3.org/1999/xhtml">
    <section>
      <h2 xmlns="http://www.w3.org/2000/svg">Header 2</h2>
      <table xmlns="http://www.w3.org/2000/svg">
        <thead>
          <tr>
            <th>Cell 2-1</th>
            <th>Cell 2-2</th>
            <th>Cell 2-3</th>
          </tr>
        </thead>
      </table>
    </section>
  </div>
</foreignObject>

The bug happens only when calling innerHTML. And it happens because the XMLDocumentParser sets the namespace "http://www.w3.org/2000/svg" to the elements, it creates from the fragment, if the page is an SVG document.

This bug can be seen also when showing the results of the SVG WPT tests in the browser. For example, when opening this test http://web-platform-tests.live/svg/painting/parsing/fill-opacity-valid.svg WebKit, the results section will not be shown.

[Said Abou-Hallawa]

Created attachment 382866 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 382866 [details]
Patch

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

Don't get what you mean by pasting. Neither test case does paste in the sense of editing code.

Are you talking about the fragment parsing?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:585
>      Vector<Element*> elemStack;

Can we make this use Ref<Element> instead while we're at?

[Ryosuke Niwa]

If this is meant to fix a case where the user copy & pastes content, then we need to be adding a test which uses execCommand('insertHTML', ~).

[Said Abou-Hallawa]

Created attachment 382879 [details]
Patch

[Said Abou-Hallawa]

Created attachment 382880 [details]
pasting test case (should not work)

[Said Abou-Hallawa]

(In reply to Ryosuke Niwa from comment #3)
> If this is meant to fix a case where the user copy & pastes content, then we
> need to be adding a test which uses execCommand('insertHTML', ~).

The patch is about setting innerHTML property of the HTMLElement which parses the text through XMLDocumentParser.

Pasting text through execCommand('insertHTML', ~) should not work inside an SVG document. In the debug build, HTMLConstructionSite::HTMLConstructionSite() asserts the document is either isHTMLDocument() or isXHTMLDocument(). If this assertion is disabled, paring the text will be done by HTMLConstructionSite via HTMLDocumentParser which does not set the namespaces.

I attached a test case which works in the WebKit release build while it does not work in other browsers.

[Ryosuke Niwa]

Comment on attachment 382879 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:586
> +    while (parentElement && !is<SVGForeignObjectElement>(parentElement)) {

What's the point of collecting all these elements here?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:604
> +                m_defaultNamespaceURI = attribute.value();
> +            else if (attribute.prefix() == xmlnsAtom())
> +                m_prefixToNamespaceMap.set(attribute.localName(), attribute.value());

Then traversing them top-down to find the last element which specifies xmlns?
I think a way simpler way to do this to keep traversing parent nodes until we find the first ancestor element which specifies xmlns.
That would automatically stop before foreignElement because the element underneath it would specify xmlns.

Such an approach would match https://html.spec.whatwg.org/multipage/xhtml.html#parsing-xhtml-fragments better as well.

[Said Abou-Hallawa]

Created attachment 382948 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 382948 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:587
> +    for (auto* ancestor = parentElement; ancestor && !is<SVGForeignObjectElement>(ancestor); ancestor = ancestor->parentElement()) {

Is it really appropriate for this generic XML-related code to know about SVGForeignObjectElement?

[Said Abou-Hallawa]

Created attachment 382971 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 382948 [details]
Patch

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

>> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:587
>> +    for (auto* ancestor = parentElement; ancestor && !is<SVGForeignObjectElement>(ancestor); ancestor = ancestor->parentElement()) {
> 
> Is it really appropriate for this generic XML-related code to know about SVGForeignObjectElement?

I moved calculating the defaultNamespaceURI and prefixToNamespaceMap to XMLDocumentParser::parseDocumentFragment() which eventually calls XMLDocumentParser::create(). In parseDocumentFragment(), we handle parsing the text of innerHTML for the <script> and <style> elements.

[Said Abou-Hallawa]

Created attachment 382973 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 382973 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:277
> +    for (auto* ancestor = contextElement; ancestor && !is<SVGForeignObjectElement>(ancestor); ancestor = ancestor->parentElement()) {

I don't think we need SVGForeignObjectElement check anymore, do we?
Because div has xmlns, we'd stop there, right?

[Simon Fraser (smfr)]

Comment on attachment 382973 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:285
> +                prefixToNamespaceMap.set(attribute.localName(), attribute.value());

Is this prefix to namespace map something that we know at compile time?

[Said Abou-Hallawa]

Created attachment 382997 [details]
Prefix test case (Asserts in debug build)

[Said Abou-Hallawa]

Comment on attachment 382973 [details]
Patch

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

>> Source/WebCore/xml/parser/XMLDocumentParser.cpp:277
>> +    for (auto* ancestor = contextElement; ancestor && !is<SVGForeignObjectElement>(ancestor); ancestor = ancestor->parentElement()) {
> 
> I don't think we need SVGForeignObjectElement check anymore, do we?
> Because div has xmlns, we'd stop there, right?

This patch is not correct. We should stop at the root-most element when building prefixToNamespaceMap. But we should stop at the SVGForeignObjectElement when getting defaultNamespaceURI. I will upload another patch.

>> Source/WebCore/xml/parser/XMLDocumentParser.cpp:285
>> +                prefixToNamespaceMap.set(attribute.localName(), attribute.value());
> 
> Is this prefix to namespace map something that we know at compile time?

No. The web developer can define at any parent level, the prefix name space. For example the root <svg> element can be defined like that,

    <svg xmlns="http://www.w3.org/2000/svg" xmlns:h="http://www.w3.org/1999/xhtml">

And the innerHTML text can be like that:

       log.innerHTML = "<h:h2>Header</h:h2>";

XMLDocumentParser has to resolve the 'h' prefix to "http://www.w3.org/1999/xhtml". See the attached test case which does not work properly in WebKit but works correctly in other browsers.

[Said Abou-Hallawa]

Created attachment 383070 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 383070 [details]
Patch

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

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:275
> +    for (auto* ancestor = contextElement; ancestor; ancestor = ancestor->parentElement()) {

Use ElementAncestorIterator?

> Source/WebCore/xml/parser/XMLDocumentParser.cpp:286
> +    AtomString defaultNamespaceURI;
> +    for (auto* ancestor = contextElement; ancestor && !is<SVGForeignObjectElement>(ancestor); ancestor = ancestor->parentElement()) {

I don't think we need to traverse ancestors twice like this.
Do something like this in the loop above.

if (is<SVGForeignObjectElement>(ancestor))
    stopLookingForDefaultNamespaceURI = true;
if (!stopLookingForDefaultNamespaceURI) {
    auto value = ancestor->getAttribute(XMLNSNames::xmlnsAttr);
    if (!value.isNull())
        defaultNamespaceURI = value;
}

So sad we have to add a special rule for foreignElement...

[Said Abou-Hallawa]

Created attachment 383095 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383095 [details]
Patch

Clearing flags on attachment: 383095

Committed r252230: <https://trac.webkit.org/changeset/252230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57008214>