Bug 8393 – <br>s created by createMarkup aren't valid xhtml

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

They need to be self closing: <br />

[James Cox]

also elements need to be lowercase as XML is case sensitive....

[Levi Weintraub]

Created attachment 8256 [details]
Patch + Layout Tests + Detailed Changelog Entry

[Eric Seidel (no email)]

Comment on attachment 8256 [details]
Patch + Layout Tests + Detailed Changelog Entry

I don't think this is right.  I don't like the lowering of the nodeNames().

I also think it could have a better name for the test.  We can talk about this in person.

[Dave Hyatt]

Comment on attachment 8256 [details]
Patch + Layout Tests + Detailed Changelog Entry

Elements and attributes in XML are case-sensitive.  For generic non-HTML elements, the output should not be lower-cased. It should be left alone.

For HTML elements, nodeName has been hacked to return an upper-case string.  In both HTML and XHTML, this is not what should be used for HTML elements.  We want to output in lower-case for HTML elements regardless of document type.

Rather than having to .lower() over and over it would be better to put an accessor on HTML element that didn't .upper() the result.  

Right now you're calling nodeName() which uppers and then .lowering that.

[Darin Adler]

Comment on attachment 8256 [details]
Patch + Layout Tests + Detailed Changelog Entry

The bug in the original code is that it is using nodeName(). That function uppercases the local names. Instead you should use localName() on that call site. You should *not* call lower() since there can be non-HTML tags and attribute names that are mixed case, and lowercasing them is incorrect.

I believe the change to shouldSelfClose is also incorrect, although you should check that one with Maciej and Eric who have worked in this area in the past. I don't think we want to generate self-closing tags at all when generating HTML.

[Levi Weintraub]

Created attachment 8331 [details]
Patch + Changelog

The confusion seems to come from the expectation for createMarkup to generate xhtml when operating on an html document. This is not desired behavior. There was an issue with the case being .uppered() for html documents, and this has been patched by substituting localName for nodeName as per Darin's comment.

When operating on an xhtml document, createMarkup generates valid <br>s.

[Eric Seidel (no email)]

Comment on attachment 8331 [details]
Patch + Changelog

r=me!  we discussed in person some additional testing strategies.  But the code is OK to land.

[Dave Hyatt]

Are you calling .localName on XML elements too?  That would not be right.  localName excludes the prefix, so you're losing information when serializing.

[Levi Weintraub]

Created attachment 8461 [details]
Patch

Attempted to land a fix in r14516. That change addressed mjs's point about prefixes.

Change from r14516 to remove ugly non-virtual dispatch (my bad).

[Dave Hyatt]

Comment on attachment 8461 [details]
Patch

Yeah this is good.  r=me.

[Levi Weintraub]

Patch landed in r14537!