300933 - Live Bookmarks should support Atom 1.0

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Jeff Schiller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+

http://www.tbray.org/ongoing/When/200x/2005/07/14/Atom-1.0

According to Tim Bray, it's time for implementers to dig in...I suspect there
may already be a request out there for this but I could not find it.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Phil Ringnalda (:philor)]

Thanks to our indifference to namespaces, we're already 90% there: the only
thing I can see we're missing is support for <link href="..."/> -
rel="alternate" is now the default, and since it's absent in a good percentage
of the early example feeds in
http://www.intertwingly.net/wiki/pie/KnownAtomFeeds it's likely to wind up
absent in most feeds. If around
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#699
we just test for rel="alternate" or no rel, I think we'll be as good with 1.0 as
we are with 0.3. Sam, anything else I'm missing that affects us?

But with our release timing, we really really need that one little change before
1.5 - the Atom community really wants to see 0.3 disappear, so by next year I'd
expect to see tens of millions more 1.0 feeds than 0.3 feeds.

Comment 2

[Asa Dotzler [:asa]]

Vlad, how difficult would it be to get this in for 1.5?

Comment 3

[Sam Ruby]

Titles also have a type attribute that indicate whether they are "text", "html"
or "xhtml".

If SeaMonkey expects plain text, then the content should be left alone,
unescaped, and stripped of all elements respectively.

If SeaMoney expects html, then the content should be escaped, left alone, and
something like the equivalent of innerXml should be used, again respectively.

If any of this is unclear, I'd be glad to write a test case.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

What's SeaMonkey doing with titles?

Comment 5

[Phil Ringnalda (:philor)]

SeaMonkey's waiting for us to port Live Bookmarks over to it, I'd guess ;)

I don't think we have any different problems with Atom 1.0 titles than our
existing (and, interestingly enough, unreported near as I can tell) problems
with Atom 0.3 titles with @type and @mode - either way it's the same situation:

type="text" we like,
type="html" we display <title>I &lt;b>really&lt;/b> like HTML</title> as "I
<b>really</b> like HTML"
type="xml" we fail on: for some reason, FindTextInNode fails to find anything in
<title type="xml"><div xmlns="http://www.w3.org/1999/xhtml">The title</div></title>

Comment 6

[Sam Ruby]

> type="html" we display <title>I &lt;b>really&lt;/b> like HTML</title> as "I
> <b>really</b> like HTML"

There are two common cases for HTML, and two common variations.

One is to use bold and italics as Phil did above.  These should be simply be
stripped.

Second case is to provide access to HTML's named entities.  "Bill de
h&amp;Oacute;ra" is an example.

The two variations are to directly escape, like you see in the examples above. 
Or to use CDATA, thus:

<![CDATA[Bill de h&Oacute;ra]]>

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm, we should probably have a separate bug on the title issue, because it's
pretty tricky -- the best we can ever do is to strip HTML entirely, otherwise we
open up ourselves to potential security holes.  (Why the heck is a "title"
allowed to be full HTML? or XML?  I can see a separate "displayTitle" or
something, but... ugh.)(In reply to comment #1)

> Thanks to our indifference to namespaces, we're already 90% there: the only
> thing I can see we're missing is support for <link href="..."/> -
> rel="alternate" is now the default, and since it's absent in a good percentage
> of the early example feeds in
> http://www.intertwingly.net/wiki/pie/KnownAtomFeeds it's likely to wind up
> absent in most feeds. If around
>
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#699
> we just test for rel="alternate" or no rel, I think we'll be as good with 1.0 as
> we are with 0.3. Sam, anything else I'm missing that affects us?

I may be misremembering, but wasn't the default previously not "alternate", but
instead something else that shouldn't be used as a link?  (I vaguely remember
having code that just looked for a href attribute, and having to add explicit
rel="alternate" checking...)

Comment 8

[Phil Ringnalda (:philor)]

(In reply to comment #7)
> wasn't the default previously not "alternate", but
> instead something else that shouldn't be used as a link?  (I vaguely remember
> having code that just looked for a href attribute, and having to add explicit
> rel="alternate" checking...)

There wasn't a default before, so that would have been the presence of rels
which aren't alternate: if you just take the first link you find, it might be
rel="alternate" or it might be rel="self" or anything else. Now, though,
(rel="alternate" | !rel) is the link we want.

I'll file a new bug on titles, and start the slew of testcases (with Sam cc'ed
to cover me) when I get home: I'd very much like to see link with no rel in
1.8b4 since it's clearly going to break us in a large share of 1.0 feeds, and I
fear we won't get the title part done in time.

Comment 9

[Phil Ringnalda (:philor)]

Attachment: testcase for link @rel

Three <entry>s, defaulted and explicit @rel="alternate" that we should find,
@rel="self" but no @rel="alternate" that we shouldn't.

Comment 10

[Phil Ringnalda (:philor)]

Filed bug 302133 for the title-escaping issues.

Comment 11

[Sam Ruby]

Attachment: Fix that addresses the testcase Phil provided

Tested on Ubuntu 5.04

Comment 12

[Sam Ruby]

(In reply to comment #10)
> Filed bug 302133 for the title-escaping issues.

There probably should be a separate bug for relative URIs / xml:base support

Comment 13

[Phil Ringnalda (:philor)]

Comment on attachment 190544 [details] [diff] [review]
Fix that addresses the testcase Phil provided

Twiddling review bits; it'll need checkin, too, since I don't think Sam's got
the permissions here that he does in Apache and PHP land.

Relative URLs is bug 262222, and the tricky xml:base testcases you've probably
already got would be most welcome.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 190544 [details] [diff] [review]
Fix that addresses the testcase Phil provided

Not quite; other patch inc in a sec

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for empty/not specified rel

The rv test is gratuitious, as all the code truncates right now, but eh.

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190711 [details] [diff] [review]
patch for empty/not specified rel

r+a=shaver

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in slightly modified patch -- no rv check, because we'd never actually
hit it and it just confused the issue (and needed an extra include file to be
defined, etc.).