303848 - Do a better job of integrating RSS pretty print into Firefox

Status: VERIFIED INVALID

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

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Our newfound pretty print feature looks bolted on and the code isn't all that
nicely integrated. Granted, browser.js is a sewer, but that's no excuse. 

We want to do a few things really quickly to improve the experience for 1.5,
some outlined here:

http://wiki.mozilla.org/Firefox:1.1_RSS_Pretty_Print

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: partial work

... work in progress ...

Comment 2

[Phil Ringnalda (:philor)]

See bug 303252 for why just "instanceof XMLDocument" isn't enough to catch RSS
1.0 served as application/rdf+xml.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress

this is not meant to fix all feedview bugs, just make the code integrate more
acceptably.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: even more progress...

- now provides links to add live bookmark and take you to the "what is a live
bookmark?" info page.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: progress so far (screenshot)

Comment 6

[Asaf Romano (gone)]

Comment on attachment 191952 [details] [diff] [review]
even more progress...

Couldn't the choice between button-for-one-feed to menu-for-multipile-feeds
live in the stylesheet (you already have a livebookmark attribute).

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Why would it live in the stylesheet?

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... (almost done for round 1)

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: closer...

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

This is done for now.

I did make one change that is a potential concern for PLT, and I'm interested
in hearing feedback on it. I removed the check:

if (!(event.originalTarget instanceOf XMLDocument))
  return;

...since this screened a lot of feeds sent as the incorrect type, the
"_isDocumentFeed" check function was more comprehensive. That said, that may
have an unfortunate impact on PLT. Basically, when the "load" event fires, the
_isDocumentFeed function checks the local name and namespace of the root
element of the file against several RSS/ATOM identifying values. 

There may be other ways of detecting common server misconfigurations though -
maybe comparing against XULDocument in addition to XMLDocument?

Thoughts?

Comment 11

[Phil Ringnalda (:philor)]

Pending a fix for bug 256084, you have to use XULDocument for correct servers,
not misconfiguration. But even though the last time I saw a widescale check
(about a year ago), 20% of feeds were sent as text/plain (or a more horrible
text/html), I'd really rather see an (XML|XUL)Document check and evangelism than
see us pretty-print http://www.niallkennedy.com/blog/uploads/atom1.txt

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

So what happens now for application/rdf+xml?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 192120 [details] [diff] [review]
patch

>Index: browser/base/content/browser.js

>@@ -2730,17 +2728,19 @@ function OpenSearch(tabName, searchStr, 

>-          getBrowser().loadOneTab(defaultSearchURL);
>+          var newTab = getBrowser().addTab(defaultSearchURL);
>+          if (!gPrefService.getBoolPref("browser.tabs.loadInBackground"))
>+            getBrowser().selectedTab = newTab;

This regresses bug 249136. Guess you just haven't updated since then?

> function SwitchDocumentDirection(aWindow) {
>-  aWindow.document.dir = (aWindow.document.dir == "ltr" ? "rtl" : "ltr");
>-
>+  aWindow.document.dir = (aWindow.document.dir == "ltr" ? "rtl" : "ltr")

Just thought I'd make sure that you don't forget to not checkin these changes
when merging.

Comment 14

[Phil Ringnalda (:philor)]

Which now? With your patch, application/rdf+xml and text/plain and text/html all
get pretty-printed (unless I'm missing something); with instanceof XMLDocument
(the way feedview started) rdf+xml is one big block of unpretty text content
with the XUL mime-type; with bug 303252 checking both XULDocument and
XMLDocument, rdf+xml gets pretty-printed, and all genuine XUL as content has to
take a trip through feedviewIsFeed/_isDocumentFeed (but at least not
*everything* does, and genuine text/plain doesn't get pretty-mangled).

Something that worries me, but I'm away from my build machine and can't see if
I'm misunderstanding:

Is the flow now "discover feed(s) in HTML, click menu to show feed
pretty-printed, add livemark from a link in the pretty-print content"? That
would mean a very long way around through the bookmarks manager to manually add
a Live Bookmark for something that didn't pretty-print (fatal XML error, bad
mime-type if we don't sniff everything), or for that matter for something that
dislikes us enough to do
document.getElementById("addLiveBookmarkLink").style.display="none", so if we
have to make that our only way to add a Live Bookmark, maybe pretty-printing
text/plain isn't so bad. Better than having 1 in 5 livemarks require "Copy URL,
Bookmarks, Manage Bookmarks, File, New Live Bookmark, Paste". (My 'vote' would
be to stick Add Live Bookmark in a browsermessage button instead of content.)

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 192120 [details] [diff] [review]
patch

Phil's fix for the rdf stuff is enough (it fixes your blog, among others), so
let's not get overzealous, text/plain feeds shouldn't get prettyprinted. 
Please also see gavin's comment.

Also, I can't play with this patch since patch is choking on the long busted
line endings in some of the removals.  I don't suppose you have a patch that
applies cleanly?

I'm also not happy with the flow to add a live bookmark, if Phil's right (due
to the patch issues, I couldn't play with this, but the spec on the wiki
suggests it).  Are we now saying that live bookmarks are less desirable than
prettyprinting?

Comment 16

[Phil Ringnalda (:philor)]

Maybe a 90% solution for bad mime-types: give the autodiscovery menu a
(handwaving) alternate path into pretty-printing, so that instead of just a
normal load and _validate(), things we've autodiscovered only have to pass
through _isDocumentFeed() to get pretty-printed.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)
> I'm also not happy with the flow to add a live bookmark, if Phil's right (due
> to the patch issues, I couldn't play with this, but the spec on the wiki
> suggests it).  Are we now saying that live bookmarks are less desirable than
> prettyprinting?

I don't think we're saying that at all; as expressed on the wiki, the idea was
that the single click from the web page would lead to the prettyprinted version,
which would have an embedded control that would add a livebookmark. This would
do three things: first, it would keep the number of clicks/typing to the same as
we have now; second, it would actually make it easier for a user to see what
sort of stuff would be added to the LiveBookmark; third, it provides a much more
discoverable way of adding LiveBookmarks by taking it out of the status bar.

Now, the problem pointed out in comment 14 is a slightly different issue as I
read it: if we somehow mess up the reading of the feed, then there's no longer
any easy way to add it as a LiveBookmark. That'd suck.

Comment 18

[Phil Ringnalda (:philor)]

Add one more to the list of comment 14 woes: I finally hand-hacked a version of
the patch, and took it to http://philringnalda.com where there wasn't any way to
add my Atom feed (served as application/atom+xml) as a livemark, and once I
switch to application/rss+xml for my RSS, there wouldn't be any way to add
anything. Unless you think bug 155730 will be fixed in the 1.5 timeframe, the
autodiscovery menu better have some way of loading application/atom+xml and
application/rss+xml directly, that doesn't involve letting the browser throw up
an unknown content type dialog.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

I've also been alerted to bug 294715, which points out that some software (such
as Wordpress) publishes feeds using the feed: protocol, which FFx 1.5 doesn't
support, which would again get us to the same hard-to-add-LiveBookmark mess.

I'm adding that one as a dependency for now.

Comment 20

[Phil Ringnalda (:philor)]

Example for my disquiet about the "equal numbers in different formats means
they're duplicates" heuristic: http://plasmasturm.org/ (one Atom weblog feed,
one RSS linklog feed, and with the current patch we just completely hide the RSS
feed).

How hard would it be to shoehorn a "button with a dropdown menu" like the Back
button into the urlbar? That should wind up being familiar to future IE
switchers, since they are saying that "In future updates to IE 7, we will be
making changes to optimize for the most common scenario of having a single feed.
Getting to that first (or only) feed in the list will be one-click away. Getting
to the others will be one or two more clicks."

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

I actually think this patch offers a far more discoverable way to live bookmark
feeds than the existing UI, which almost no one offers. 

I'm adding back the check for instanceof XMLDocument and checking this in today. 

Comment 22

[Asaf Romano (gone)]

Comment on attachment 192120 [details] [diff] [review]
patch


-	   buttonAccesskeyString =
browserBundle.getString("xpinstallDisabledWarningButton.accesskey");
-	   buttonString =
browserBundle.getString("xpinstallDisabledWarningButton");
+	   buttonKey =
browserBundle.getString("xpinstallDisabledWarningButton.accesskey");
+	   buttonAccesskeyString =
browserBundle.getString("xpinstallDisabledWarningButton");
	   getBrowser().showMessage(browser, iconURL, messageString,
buttonString,
				    null, "xpinstall-install-edit-prefs",
				    null, "top", false, buttonAccesskeyString);

please drop this change (and the ones gavin mentioned)

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

Yes - I'll make sure not to commit unrelated things. My tree is slightly out of
date. 

I discussed this with beltzner again in person today and to my chorus of "No one
notices the Livemark icon in the corner of the window, so the Add Livemark link
is much more noticable even if it does introduce an extra click - it's not the
end of the world" he adds: "pages don't often use the title attribute for <link>
tags properly and as such the material shown in the livemark button menu is
never particularly useful - it is more useful to see the content of the feed and
understand that is the content being livemarked"

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

Landed (sans extraneous material and added back instanceof XMLDocument check).

Further convergence with the wiki page can be tracked in separate bugs. 

Comment 25

[Bryan]

When viewing the "pretty print" page the "info" column on the upper right hand
side of the page that states "This page contains X articles." wraps the words
"What are Live Bookmarks?"  Like this:

What are Live 
Bookmarks?

Can this be fixed?

~B

Comment 26

[Anbo Motohiko]

I cannot add some rss feeds as live bookmark which have own XSLT. e.g.:
http://diary.noasobi.net/rss.rdf

I'm using pacifica-trunk, build ID: 2005081118.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

This checkin seems to have broken a lot:

* feeds don't get initialized; apparently Feed.init() is commented out with <!--
//-->;
* even if feeds did get initialized, dates wouldn't appear because
initializeDates calls xmlDate instead of this._xmlDate (i'll fix this and
perhaps the other issues as well in my date-fixing patch for bug 304362);
* the sidebar disappears as soon as it appears (not sure what this is yet).

Comment 28

[Myk Melez [:myk] [@mykmelez]]

I fixed several regressions from this bug in my patch for bug 304362 (attachment
192491 [review]).

Comment 29

[Kevin Gerich]

Attachment: Pinstripe CSS changes

The URLbar on the Mac looks broken without these CSS rules.

Comment 30

[Bryan]

The address bar RSS icon breaks if you customize your toolbars to remove the
address bar and put it back for that FF session.

1) Launch FF (Latest Build)
2) Navigate to http://www.news.com/
3) Notice the RSS Icon in the address bar
4) Customize toolbar to remove the address bar and click "Done"
5) Add the address bar back (Customize again)
6) Reload http://www.news.com/ and notice the RSS icon in the address bar is missing
7) Navigate to another site that publishes RSS and notice that the RSS icon in
the address bar is still missing
8) Close FF
9) Launch FF
10) Navigate to http://www.news.com/
11) Notice that the RSS Icon in the address bar is back!

~B

Comment 31

[Frederic Bezies]

Pinstripe changes are allright. Let's hope it will be added soon :)

Comment 32

[Aaron Leventhal]

Is there a way to "view this page's feeds" with the keyboard?

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

reopening to handle after backout

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: backout patch

remove this for now, I'll get back to this later.

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

it may be easier for me to fix these issues over the weekend, but I'm leaving
the backout patch for now in case we want to get this right away.

Comment 36

[Gary Elshaw]

Just a note. Aside from the rss button being blank on the mac (bug 304436),
there is also a slight problem with the button being slightly cut-off on secure
sites. please see https://bugzilla.mozilla.org/attachment.cgi?id=192655

OS X 10.4.2 Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050813 Firefox/1.0+

Comment 37

[Ben Goodger (use ben at mozilla dot org for email)]

Update: We're not backing this out after all. 

Myk is going to check in his patch for 304362 and I'm going to tackle these issues:

- cannot tab from URL bar
- not all feeds (e.g. bbc) properly pretty printed
- make sure xp experience is consistent (no weird UI)

on Monday. 

Aaron, this feature was never keyboard accessible before. Should we add a "View
Feed" or some such thing to the View menu? Beltzner?

-Ben

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

Now pulling a 1.8 tree to investigate the remaining bugfixes. 

Comment 39

[Asaf Romano (gone)]

Comment on attachment 192501 [details] [diff] [review]
Pinstripe CSS changes

checked this in (branch & trunk).

Comment 40

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #37)
> Feed" or some such thing to the View menu? Beltzner?

The View menu is more about functions that change the user's view of the
currently loaded content, and not generally used for following links or chasing
down related content.

I do see a couple of ways (listed in first-to-my-head order) of adding in
keyboard-access, if it's felt that it's truly neccessary:

1. Add the "RSS" button as a tab target so that users can ctrl+l, tab their way
to success (downside: inserts another tab before search bar field)
2. Insert something in the page info dialog (this is similar to how keyboard
users get info about security)
3. Add something to the "Go" menu (my least favourite option)

Since it wasn't keyboard accessible before, and since really, people should be
including explicit links to their RSS feeds as well, I'm not sure how neccessary
this is for things like Sec 508. Aaron?

Comment 41

[Phil Ringnalda (:philor)]

*** Bug 304788 has been marked as a duplicate of this bug. ***

Comment 42

[Aaron Leventhal]

I just noticed this comment.

First, it's totally necessary that everything is section 508 compliant. We
weren't 508 compliant at all in previous versions of Firefox but we need to be
100% compliant now.

> 1. Add the "RSS" button as a tab target so that users can ctrl+l, tab
I don't really like adding an extra item in the tab order, because the button
will go away when there is no feed view, and that will mess with users' muscle
memory keyboard operation. Also, for some reason I think it's going to be
difficult to add these buttons, which are in the middle of a textfield, to the
tab order.

> 2. Insert something in the page info dialog (this is similar to how keyboard
> users get info about security)
I don't like the way we do it for security a lot right now because it's not very
discoverable that you'd need to go there to get this info. But it's better than
nothing.

> 3. Add something to the "Go" menu (my least favourite option)
I don't like this any more than item 2, so since you don't like it let's nix that.

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #42)
> I don't really like adding an extra item in the tab order, because the button
> will go away when there is no feed view, and that will mess with users' muscle

Yeah, none of the options are without their downside, but as we discussed in
IRC, I think this is the one that makes the most sense in terms of
discoverability and seems to be the least problematic option.

(for those following at home, muscle memory was decided to be a valid concern,
but since there's a keyboard shortcut for web search (ctrl+k) and since we're
already in a situation where the extistence or non-existence of tabs may change
the number of tabs from url-bar to content, we already have this problem already)

Comment 44

[Asher]

Right now the XML file appears to be parsed and then the feedview is displayed.
 This has the appearance of being a bit hacked, especially for larger feeds, as
it will show the ugly code and then snap to the prettyprint view. 

Is there a way to hold the XML rendering or show some other kind of screen (like
a blank prettyprint) when it is rendering the feed?  Or is that a new bug
already reported?

Comment 45

[Axel Hecht]

(In reply to comment #44)
> Right now the XML file appears to be parsed and then the feedview is displayed.
>  This has the appearance of being a bit hacked, especially for larger feeds, as
> it will show the ugly code and then snap to the prettyprint view. 
> 
> Is there a way to hold the XML rendering or show some other kind of screen (like
> a blank prettyprint) when it is rendering the feed?  Or is that a new bug
> already reported?

This is bug 303400.

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

I'm marking this INVALID, since we're going to lose this feature. 

Comment 47

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.

Comment 48

[Tanner M. Young [:tmyoung]]

Verified Invalid.