Closed
Bug 303848
Opened 19 years ago
Closed 19 years ago
Do a better job of integrating RSS pretty print into Firefox
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(Not tracked)
VERIFIED
INVALID
People
(Reporter: bugs, Assigned: bugs)
References
Details
(Whiteboard: [l10n impact])
Attachments
(4 files, 5 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mconnor
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Updated•19 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 1•19 years ago
|
||
... work in progress ...
Comment 2•19 years ago
|
||
See bug 303252 for why just "instanceof XMLDocument" isn't enough to catch RSS 1.0 served as application/rdf+xml.
Assignee | ||
Comment 3•19 years ago
|
||
this is not meant to fix all feedview bugs, just make the code integrate more acceptably.
Attachment #191927 -
Attachment is obsolete: true
Assignee | ||
Comment 4•19 years ago
|
||
- now provides links to add live bookmark and take you to the "what is a live bookmark?" info page.
Assignee | ||
Updated•19 years ago
|
Attachment #191938 -
Attachment is obsolete: true
Assignee | ||
Comment 5•19 years ago
|
||
Comment 6•19 years ago
|
||
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).
Updated•19 years ago
|
Updated•19 years ago
|
Summary: Do a better job of integrating pretty print into Firefox → Do a better job of integrating RSS pretty print into Firefox
Assignee | ||
Comment 7•19 years ago
|
||
Why would it live in the stylesheet?
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #191952 -
Attachment is obsolete: true
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #192019 -
Attachment is obsolete: true
Updated•19 years ago
|
No longer blocks: branching1.8
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 10•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #192043 -
Attachment is obsolete: true
Attachment #192120 -
Flags: review?
Comment 11•19 years ago
|
||
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
Assignee | ||
Comment 12•19 years ago
|
||
So what happens now for application/rdf+xml?
Comment 13•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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-
Comment 16•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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
Comment 20•19 years ago
|
||
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."
Assignee | ||
Comment 21•19 years ago
|
||
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•19 years ago
|
||
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)
Assignee | ||
Comment 23•19 years ago
|
||
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"
Assignee | ||
Comment 24•19 years ago
|
||
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: 19 years ago
Resolution: --- → FIXED
Comment 25•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
I fixed several regressions from this bug in my patch for bug 304362 (attachment 192491 [review]).
Comment 29•19 years ago
|
||
The URLbar on the Mac looks broken without these CSS rules.
Attachment #192501 -
Flags: approval1.8b4?
Comment 30•19 years ago
|
||
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
Updated•19 years ago
|
Attachment #192501 -
Flags: approval1.8b4? → approval1.8b4+
Comment 31•19 years ago
|
||
Pinstripe changes are allright. Let's hope it will be added soon :)
Comment 32•19 years ago
|
||
Is there a way to "view this page's feeds" with the keyboard?
Assignee | ||
Comment 33•19 years ago
|
||
reopening to handle after backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 34•19 years ago
|
||
remove this for now, I'll get back to this later.
Assignee | ||
Comment 35•19 years ago
|
||
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•19 years ago
|
||
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+
Updated•19 years ago
|
Assignee | ||
Comment 37•19 years ago
|
||
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
Assignee | ||
Comment 38•19 years ago
|
||
Now pulling a 1.8 tree to investigate the remaining bugfixes.
Comment 39•19 years ago
|
||
Comment on attachment 192501 [details] [diff] [review] Pinstripe CSS changes checked this in (branch & trunk).
Comment 40•19 years ago
|
||
(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•19 years ago
|
||
*** Bug 304788 has been marked as a duplicate of this bug. ***
Comment 42•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
(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.
Assignee | ||
Comment 46•19 years ago
|
||
I'm marking this INVALID, since we're going to lose this feature.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → INVALID
Updated•19 years ago
|
Flags: blocking1.8b4+
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•