Closed Bug 303848 Opened 15 years ago Closed 15 years ago

Do a better job of integrating RSS pretty print into Firefox

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED INVALID

People

(Reporter: bugs, Assigned: bugs)

References

Details

(Whiteboard: [l10n impact])

Attachments

(4 files, 5 obsolete files)

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
Version: unspecified → Trunk
Attached patch partial work (obsolete) — Splinter Review
... work in progress ...
See bug 303252 for why just "instanceof XMLDocument" isn't enough to catch RSS
1.0 served as application/rdf+xml.
Attached patch more progress (obsolete) — Splinter Review
this is not meant to fix all feedview bugs, just make the code integrate more
acceptably.
Attachment #191927 - Attachment is obsolete: true
Attached patch even more progress... (obsolete) — Splinter Review
- now provides links to add live bookmark and take you to the "what is a live
bookmark?" info page.
Attachment #191938 - Attachment is obsolete: true
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).
Blocks: branching1.8
Flags: blocking1.8b4?
Whiteboard: [l10n impact]
Summary: Do a better job of integrating pretty print into Firefox → Do a better job of integrating RSS pretty print into Firefox
Why would it live in the stylesheet?
Attachment #191952 - Attachment is obsolete: true
Attached patch closer... (obsolete) — Splinter Review
Attachment #192019 - Attachment is obsolete: true
No longer blocks: branching1.8
Flags: blocking1.8b4? → blocking1.8b4+
Blocks: 257247
Blocks: 303644
Attached patch patchSplinter Review
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?
Attachment #192043 - Attachment is obsolete: true
Attachment #192120 - Flags: review?
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

So what happens now for application/rdf+xml?
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.
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 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?
Attachment #192120 - Flags: review? → review-
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.
(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.
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.
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.
Depends on: 294715
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."
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 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)
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"
Landed (sans extraneous material and added back instanceof XMLDocument check).

Further convergence with the wiki page can be tracked in separate bugs. 
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
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.
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).
I fixed several regressions from this bug in my patch for bug 304362 (attachment
192491 [review]).
The URLbar on the Mac looks broken without these CSS rules.
Attachment #192501 - Flags: approval1.8b4?
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
Attachment #192501 - Flags: approval1.8b4? → approval1.8b4+
Depends on: 304457
Pinstripe changes are allright. Let's hope it will be added soon :)
Blocks: 304436
Is there a way to "view this page's feeds" with the keyboard?
reopening to handle after backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch backout patchSplinter Review
remove this for now, I'll get back to this later.
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.
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+
Depends on: 304622
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
Depends on: 304705
Now pulling a 1.8 tree to investigate the remaining bugfixes. 
Depends on: 304597
Comment on attachment 192501 [details] [diff] [review]
Pinstripe CSS changes

checked this in (branch & trunk).
(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?
*** Bug 304788 has been marked as a duplicate of this bug. ***
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.
(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)
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?
(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.
Depends on: 304962
I'm marking this INVALID, since we're going to lose this feature. 
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → INVALID
No longer depends on: 257247
Flags: blocking1.8b4+
No longer depends on: 304461
No longer depends on: 294715
No longer depends on: 304807
No longer depends on: 304843
No longer depends on: 304458
Resetting QA Contact to default.
QA Contact: nobody → rss.preview
Verified Invalid.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.