436801 - XHTML not properly supported in Feed titles

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[wooptoo]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008052921 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008052921 Minefield/3.0

Some time ago I made an Atom 1.0 test feed to examine the differences in rendering between various feed readers.
The feed can be found here: http://cmyk.wooptoo.com/exp/atom_vs_rss/atom.xml
and results here: http://cmyk.wooptoo.com/exp/atom_vs_rss/results

The only bug I found in Firefox 3, was that it couldn't interpret (or display?) XHTML elements in feed titles. Tags like <em> or <strong> are simply ignored.

I just wanted to know if this is an intentional behavior, or whether it is really a bug.
For example, Google Reader renders XHTML tags in feed titles properly. This might happen because it's a web-based reader.

Reproducible: Always

Comment 1

[Tanner M. Young [:tmyoung]]

Have you tested to see if this still happens in Firefox 3.5?

Comment 2

[wooptoo]

Yes, this still happens in Firefox 3.5
You can also find the Atom test feed here: http://pastebin.com/f5583531a (the site mentioned above is not always online).

Comment 3

[Tanner M. Young [:tmyoung]]

CCing sayrer who will know if this is the intended behavior and if it can be fixed.

Comment 4

[Nicholas Wilson]

Attachment: Write HTML content into the title instead of unnecessarily flattening it to plain text

This is a simple, uncontroversial fix.

Regarding xml:base, the base is correctly used for resolving the <a> placed on the feed title, but markup in the title (like inline image references) will not have the correct base set. This is an essentially non-existent problem. If you want it fixed (to 'do it right' while we are at it) it would need some wrapper spans.

Comment 5

[Robert Sayre]

Comment on attachment 471141 [details] [diff] [review]
Write HTML content into the title instead of unnecessarily flattening it to plain text

r=me, but this needs a test too. xml:base can be handled another day.

Comment 6

[Nicholas Wilson]

Attachment: Added some unit tests; fixed xml:base behaviour

Today is another day. xml:base is easy to handle. There are also four unit tests to ensure that the base URI is added, and four to check that the title elements correctly contain the markup.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 471530 [details] [diff] [review]
Added some unit tests; fixed xml:base behaviour

Review of attachment 471530 [details] [diff] [review]:
-----------------------------------------------------------------

These patches still apply cleanly.  After all this time I hate to pick nits, but... If you're not able or interested in making these changes anymore, let me know and I'll make them and land.

It's hard to read the tests due to all the very long lines.  Please add line breaks and indentation to make them more readable, like:

<title>
  <div>
    ...
  </div>
</title>

and 

is(document.getElementById("..."),
   "some very long string",
   "this should be that");

::: browser/components/feeds/src/FeedWriter.js
@@ +210,5 @@
>        "  element.removeChild(element.firstChild);" +
>        "element.appendChild(textNode);";
> +    if (text.base) {
> +      this._contentSandbox.spec = text.base.spec;
> +      codeStr += "element.setAttributeNS('"+XML_NS+"', 'base', spec);";

Please put spaces around the concatenation terms:

"element.setAttributeNS('" + XML_NS + "', 'base', spec);";

Comment 8

[Drew Willcoxon :adw]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3126e00ec91

The innerHTML comparisons in the test failed because some attributes were serialized in a different order, so I made the test check the DOM instead.

Thanks for the patch!  Sorry it took so long to review and land.

Comment 9

[Drew Willcoxon :adw]

Whoops, shouldn't have marked fixed yet since it's not on m-c.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3126e00ec91