Bug 203868

Summary: Default NamepaceURI must be gotten from the topmost parent before the SVG <foreignObject>
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: DOMAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, ews-watchlist, gyuyoung.kim, rniwa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 200143    
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
pasting test case (should not work)
none
Patch
none
Patch
none
Patch
none
Prefix test case (Asserts in debug build)
none
Patch
none
Patch none

Said Abou-Hallawa
Reported 2019-11-05 15:51:11 PST
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.
Attachments
test case (1.27 KB, image/svg+xml)
2019-11-05 15:51 PST, Said Abou-Hallawa 		no flags
Details
Patch (6.26 KB, patch)
2019-11-05 16:59 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (6.51 KB, patch)
2019-11-05 18:17 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
pasting test case (should not work) (1.07 KB, image/svg+xml)
2019-11-05 18:19 PST, Said Abou-Hallawa 		no flags
Details
Patch (6.63 KB, patch)
2019-11-06 12:48 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (9.60 KB, patch)
2019-11-06 15:44 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (9.82 KB, patch)
2019-11-06 15:52 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Prefix test case (Asserts in debug build) (1.46 KB, image/svg+xml)
2019-11-06 18:19 PST, Said Abou-Hallawa 		no flags
Details
Patch (14.34 KB, patch)
2019-11-07 13:59 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (14.66 KB, patch)
2019-11-07 17:10 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-11-05 16:59:24 PST
Created attachment 382866 [details] Patch
Ryosuke Niwa
Comment 2 2019-11-05 17:04:54 PST
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
Comment 3 2019-11-05 17:05:23 PST
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
Comment 4 2019-11-05 18:17:35 PST
Created attachment 382879 [details] Patch
Said Abou-Hallawa
Comment 5 2019-11-05 18:19:02 PST
Created attachment 382880 [details] pasting test case (should not work)
Said Abou-Hallawa
Comment 6 2019-11-05 18:30:55 PST
(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 7 2019-11-05 19:11:37 PST
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
Comment 8 2019-11-06 12:48:32 PST
Created attachment 382948 [details] Patch
Simon Fraser (smfr)
Comment 9 2019-11-06 12:49:49 PST
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
Comment 10 2019-11-06 15:44:49 PST
Created attachment 382971 [details] Patch
Said Abou-Hallawa
Comment 11 2019-11-06 15:49:24 PST
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
Comment 12 2019-11-06 15:52:10 PST
Created attachment 382973 [details] Patch
Ryosuke Niwa
Comment 13 2019-11-06 17:10:44 PST
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 14 2019-11-06 17:14:40 PST
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
Comment 15 2019-11-06 18:19:20 PST
Created attachment 382997 [details] Prefix test case (Asserts in debug build)
Said Abou-Hallawa
Comment 16 2019-11-06 18:26:31 PST
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
Comment 17 2019-11-07 13:59:29 PST
Created attachment 383070 [details] Patch
Ryosuke Niwa
Comment 18 2019-11-07 14:16:02 PST
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
Comment 19 2019-11-07 17:10:27 PST
Created attachment 383095 [details] Patch
WebKit Commit Bot
Comment 20 2019-11-07 18:52:48 PST
Comment on attachment 383095 [details] Patch Clearing flags on attachment: 383095 Committed r252230: <https://trac.webkit.org/changeset/252230>
WebKit Commit Bot
Comment 21 2019-11-07 18:52:50 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 22 2019-11-07 18:53:15 PST
<rdar://problem/57008214>
Note You need to log in before you can comment on or make changes to this bug.