Support XHTML in feed titles

RESOLVED FIXED in seamonkey2.38

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

Trunk
seamonkey2.38

SeaMonkey Tracking Flags

(seamonkey2.38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(From Bug 436801 comment #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
Posted patch Patch v1.0 Proposed fix (obsolete) — Splinter Review
Most of this patch are the tests - copied from Firefox.
Attachment #8616793 - Flags: superreview-
Attachment #8616793 - Flags: review?(neil)
Comment on attachment 8616793 [details] [diff] [review]
Patch v1.0 Proposed fix

(Didn't look at the tests)

>+   *          a nsIFeedTextConstruct
Nit: an (395 matches for " a nsI" but 956 for " an nsI")

>+   *          The feed container, a nsIFeedContainer
Ditto

>-        this._setContentText(a, entry.title);
>+        let span = this._document.createElementNS(HTML_NS, "span");
>+        a.appendChild(span);
>+        this._setContentText(span, entry.title);
Why this change?
> (Didn't look at the tests)
Me neither. I just copied them straight from Firefox.

Passed: 74
Failed: 0
Todo: 0

And they all passed.

>>+   *          a nsIFeedTextConstruct
> Nit: an (395 matches for " a nsI" but 956 for " an nsI")
>>+   *          The feed container, a nsIFeedContainer
> Ditto
Fixed.

>>-        this._setContentText(a, entry.title);
>>+        let span = this._document.createElementNS(HTML_NS, "span");
>>+        a.appendChild(span);
>>+        this._setContentText(span, entry.title);
> Why this change?

In Bug 436801 comment 4:

> 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.

I assume he knew what he was talking about.
Attachment #8616793 - Attachment is obsolete: true
Attachment #8616793 - Flags: review?(neil)
Attachment #8622385 - Flags: review?(neil)
Comment on attachment 8622385 [details] [diff] [review]
Patch v2.0 fix grammar neet.

Oh, I get it now, _setContentText wants to set the xml:base on the element, but we can't afford to do that for the a element because then its href might be wrong.
Attachment #8622385 - Flags: review?(neil) → review+
a=me for CLOSED TREE
http://hg.mozilla.org/comm-central/rev/cd3b2d6056ab
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.38
You need to log in before you can comment on or make changes to this bug.