Bug 5404 - fix createMarkup to support XML
Summary: fix createMarkup to support XML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 3275
  Show dependency treegraph
 
Reported: 2005-10-17 15:42 PDT by Eric Seidel (no email)
Modified: 2019-02-06 09:03 PST (History)
1 user (show)

See Also:


Attachments
Fixes createMarkup's XML support. (8.51 KB, patch)
2005-10-17 15:48 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch to fix toString() and createMarkup() to better support xml (14.54 KB, patch)
2005-10-20 13:14 PDT, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
One patched test (1.07 KB, patch)
2005-10-20 15:05 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Another new test case, using innerHTML (1.95 KB, patch)
2005-10-24 22:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch w/ darin's suggested fixes. (14.62 KB, patch)
2005-10-24 23:01 PDT, Eric Seidel (no email)
mjs: review-
Details | Formatted Diff | Diff
Patch addressing mjs's compatibility concerns. (15.51 KB, patch)
2005-10-25 12:18 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Additional innerHTML test cases (both for xhtml and html) (3.43 KB, patch)
2005-10-25 12:20 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-10-17 15:42:18 PDT
The attached patch fixes createMarkup to better support serializing XML.
Comment 1 Eric Seidel (no email) 2005-10-17 15:48:05 PDT
Created attachment 4386 [details]
Fixes createMarkup's XML support.

This fix is covered at least by XSLT test cases soon to land.
Comment 2 Eric Seidel (no email) 2005-10-20 13:14:05 PDT
Created attachment 4426 [details]
Patch to fix toString() and createMarkup() to better support xml
Comment 3 Darin Adler 2005-10-20 13:50:10 PDT
Comment on attachment 4426 [details]
Patch to fix toString() and createMarkup() to better support xml

I noticed that the doesHTMLForbidEndTag function doesn't do the right thing for
XML.

Otherwise, patch looks pretty good. Need test cases to test all the things
fixed here.
Comment 4 Eric Seidel (no email) 2005-10-20 15:05:00 PDT
Created attachment 4431 [details]
One patched test
Comment 5 Eric Seidel (no email) 2005-10-24 22:52:19 PDT
Created attachment 4468 [details]
Another new test case, using innerHTML
Comment 6 Eric Seidel (no email) 2005-10-24 22:54:32 PDT
Comment on attachment 4468 [details]
Another new test case, using innerHTML

NOTE, view this as source if you want to see what it really looks like.
Comment 7 Eric Seidel (no email) 2005-10-24 23:01:07 PDT
Created attachment 4469 [details]
Patch w/ darin's suggested fixes.
Comment 8 Maciej Stachowiak 2005-10-25 00:43:32 PDT
r- for some issues Eric and I discussed. HTML tags that have no children but aren't assumed empty, such 
as <span>, will serialize without a close tag, this is wrong. I also strongly suggested that XHTML should 
be emitted as HTML-compatible when possible, that is, self-closing tags should be used only for tags 
where an end tag would be forbidden in HTML.
Comment 9 Darin Adler 2005-10-25 08:48:08 PDT
Comment on attachment 4468 [details]
Another new test case, using innerHTML

Clearing review flag from the test case. It's fine, but we don't want this to
show up as a "bug with a reviewed patch".
Comment 10 Eric Seidel (no email) 2005-10-25 12:18:20 PDT
Created attachment 4478 [details]
Patch addressing mjs's compatibility concerns.
Comment 11 Eric Seidel (no email) 2005-10-25 12:20:17 PDT
Created attachment 4479 [details]
Additional innerHTML test cases (both for xhtml and html)
Comment 12 Darin Adler 2005-10-25 22:11:39 PDT
Comment on attachment 4478 [details]
Patch addressing mjs's compatibility concerns.

It's strange to forward-declare a function inline as you do
doesHTMLForbidEndTag and shouldSelfClose. If the thing isn't defined until
after it's used, it won't get inlined, but if it's defined before it's used, it
doesn't need to be forward-declared.

Patch looks fine, r=me.
Comment 13 Maciej Stachowiak 2005-10-25 22:17:05 PDT
+// Rules of self-closure
+// 1. all html elements in html documents close with >
+// 2. all elements w/ children close with >
+// 3. all non-html elements w/o children close with />
+static inline bool shouldSelfClose(const NodeImpl *node)

A few comments on this:

1) I do not think this is a complete statement of the rules. You should also mention that HTML elements 
in an XML document which do not allow a close tag in HTML should use the self-closing syntax.

2) Technically, XML refers to tags like <foo/> as "minimized", not "self-closing", perhaps you would 
like to comply with the official terminology and call this function shouldMinimize (and make other 
corresponding changes.

I'll leave the patch reviewed and leave it to your judgment how to address these remarks.
Comment 14 Maciej Stachowiak 2005-10-25 22:17:56 PDT
Also I think what Darin said about the inliner is wrong in the latest gcc. As long as a function has an inline 
declaration in the same compilation unit, it will get inlined.
Comment 15 Eric Seidel (no email) 2005-10-26 23:31:43 PDT
Fixed!
Comment 16 Lucas Forschler 2019-02-06 09:03:00 PST
Mass moving XML DOM bugs to the "DOM" Component.