346009 - better subscription UI for feed preview

Status: RESOLVED FIXED

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

Description

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

In the newsgroup I'd proposed (based on information from a usability study from Google) that we switch the feed subscription page to look more like this:

 |                                                          |
 | Subscribe to this feed using [%lastUsedOption  |v]       |
 |                                                          |
 |  [ ] Always use %lastUsedOption to subscribe to feeds    |
 |                                                          |
 |                   ( Subscribe Now )                      |
 '----------------------------------------------------------'

This requires that we maintain the last used option for the feedreader; dunno if we do that now.

An alternative that requires fewer changes to the existing UI would be to do something like:

 This is a preview of the web feed. You can use [ &lastUsed; |v] to (Subscribe)

With a "Manage Feed Readers ..." option at the bottom that launches the existing modal dialog.

Mano has said he might have time to look at this potential usability win and polish.

Comment 1

[Asaf Romano (gone)]

Attachment: WIP

Some notes:
  1. The default reader code here isn't tested at all (might even cause the whole patch not to work on windows, will test tomorrow).
  2. This inclues a back out of bug 344651 - both because I wanted to attach this patch asap and because we'll have to revise its changes (an "ask" mode doesn't make sense anymore).
  3. There is no way to test/bake this on trunk due to some xul-in-xhtml regressions (the checkbox and button are not "clickable" for example).
  4. Not fully-localizable yet.
  5. This doesn't include the necessary updates the feeds prefpanel UI.
  6. Needs some code cleanup.

Nevertheless, asking beltzner for ui-review on this patch before I continue to work on it.

Comment 2

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

Comment on attachment 231355 [details] [diff] [review]
WIP

This is the right direction, yes. I don't think we need the bit of text describing what a feed is; less text is probably better. That also means we could just have a single solid yellow background instead of a section of white:
 
Perhaps:

 .---. Subscribe to this feed using ( # Live Bookmarks |v)
 |--.| 
 |o ||                                   ( Subscribe Now )
 '---'

 [ ] Always subscribe to feeds this way and skip this preview.


The checkbox there is odd to me, since there's no "OK" button that really associates with it - when the user clicks "Subscribe Now", it's not like the page disappears or anything. My only other thought at this time would be to use an "Options ..." button instead of the checkbox and have that button lead to the prefpanel.

Connor? Ben?

Comment 3

[Asaf Romano (gone)]

The "what is a feed" text appears only the first time a feed is previewed (I changed the way this is implemented so one-time _after_ using this patch).

Note the prefs are saved only if you do subscribe.

Comment 4

[Asaf Romano (gone)]

Attachment: use wrappedJSObject, still WIP

the previous patch was broken in builds which have the fix for bug 345991.

Comment 5

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

Mano, we really (really!) want this, but will not block on it under the new understanding that blocking flags mean "can't ship the product without this". Marking blocking- shouldn't be taken as an indication of lack of interest or willingness to take a patch. :)

Also: should we consider adding a success indicator? Might not be needed, since we'll either be showing the LiveBookmark dialog, or the other app/website will take focus ...

Comment 6

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

I think saying "we really want this" is a little strong. 

I'm still not convinced of the value offered by this added complexity. I know some people love Live Bookmarks (I used to), but this distinction still seems arbitrary, since there are plenty of River of News style readers out there too (like the personalized homepages).

Comment 7

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

I would say now on reflection that this UI really sort of obscures from the user the range of choices available to them in this space, which is probably OK if there was a good default, but I don't believe that Live Bookmarks are necessarily the best default. 

The thing I liked about the Options dialog was that it showed clearly a number of choices that were available, not hidden by a dropdown. I sort of cringed to have to show this to everyone the first time through, it brought flashbacks of "This site wants to open a popup do you want to set popup blocking preferences now?" but I could figure out no way to offer the choice I felt users needed here. 

Note: this _is_ one of those cases where choice is likely - much more so than Anti-Phishing provider!

Comment 8

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

(In reply to comment #7)
> I would say now on reflection that this UI really sort of obscures from the
> user the range of choices available to them in this space, which is probably 

So we can make the first time through this UI have an unselectable "Choose your feed reader" item in the drop-down and disable subscribe. I agree with your gut instinct to not show a pop-up dialog the first time we hit the subscribe button.

> if there was a good default, but I don't believe that Live Bookmarks are
> necessarily the best default. 

Agree here. We do want to expose choice.

Comment 9

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

I hate the popup menu. It is so compact and lacks the luxury of space. 

My original idea was to show the options dialog inline in the page, where it would have seemed less like a configuration choice. 

I imagined a workflow based on your suggestion of having a non-selectable choose reader item in the list. It goes like this:

1. visit feed page
2. click on subscribe 
3. nothing happens
4. realize 'choose your default reader...' is showing in the select
5. choose a default reader
6. click subscribe again

Ow.

Comment 10

[Asaf Romano (gone)]

Sure we could disable the button.

Another option is using a list (similar to the one in the options dialog) but this would take way too much space imho.

Comment 11

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

(In reply to comment #9)
> I hate the popup menu. It is so compact and lacks the luxury of space. 

Yet carries the benefit of not being a modal dialog for the simple operation of selecting a subscription mechanism from a list of existing ones.

> My original idea was to show the options dialog inline in the page, where it
> would have seemed less like a configuration choice. 

I think the drop-down widget is the best approximation of this. I'd totally support a more AJAX-y style interaction where a more robust (and luxuriously spaced) selection UI is presented, but that seems like a larger change than we have time for.

> I imagined a workflow based on your suggestion of having a non-selectable
> choose reader item in the list. It goes like this:
[...]
> Ow.
> 

Yeah, I thought of that after hitting the "Submit" button on that comment. Ow, indeed, though I don't think it's disastrous, and if anything it argues for piking an apporpriate default. However we already render a different UI when it's the user's first time there, though, right? We can leverage that to show the first time dialog as:

 .---. This is a feed of frequently changing content on this site.
 |--.| You can subscribe to this feed using a feed reader to recieve updates
 |o || when this content changes.                                 
 '---'
       First, pick a feed reader to use (# Firefox Live Bookmarks |v) (?)
       [ ] Always subscribe to feeds this way and skip this preview.

       Then click to ( Subscribe Now )

That's pretty text-y, but makes it abundantly clear. The (?) button could be a "What's a feed reader ..." which could launches a window that is similar to the one you have the describes what different feed readers are in more detail. 

My goal here is to keep the experience informative, but very lightweight, especially for subsequent feed previews for users who do want to be able to quickly switch between mechanisms.

Comment 12

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

Again, my complaint about just defaulting to live bookmarks. 

Someone should write a patch and show some screenshots of what this looks like. Remember: it has to look good!

What's there now has a high level of visual polish, and since I don't think any of these solutions offer an order of magnitude improvement over what's there now. An improvement, sure. 

Comment 13

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

(In reply to comment #12)
> Again, my complaint about just defaulting to live bookmarks. 

I have thought about this a lot over the past few days, and I think you're wrong here. We have to default to something, and actually, we currently default to Live Bookmarks - we just make the user go through a second dialog to get there, and then make them go through that dialog again if they want to change it later. 

As for the chosen default, I think it's absolutely the right one. It would be incorrect of us to assume that the user has a feed reader installed or an account with any of the services we package, and thus Live Bookmarks is the most sensible option.

> What's there now has a high level of visual polish, and since I don't think 
> any of these solutions offer an order of magnitude improvement over what's 
> there now. An improvement, sure. 

As I understand it, the changes we'd be making would be moving some elements around and removing a modal dialog, so I'm not sure how it changes the level of visual polish. I'm actually still happiest with the design in comment 2, with the additional strings that are currently shown the first time that a feed is loaded.

Comment 14

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

Mano, were you gonna post some screenshots of the WIP? I can't do Windows builds atm ...

Comment 15

[Asaf Romano (gone)]

Attachment: screenshot (OS X)

This is less pretty on windows (screenshot later), the selected item does have its icon (shown in the menulist widget), but the menuitems inside the popup don't have icons.

Comment 16

[Asaf Romano (gone)]

Attachment: almost ready

still some hardcoded strings AND:

The UI is inconsistent right now: while the preview UI exposes the live bookmarks option as one of the readers, the options UI doesn't (see the radiogroup in the feed preferences pane).

Comment 17

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

Looking good. What happened to the RSS icon, though?

(In reply to comment #16)
> The UI is inconsistent right now: while the preview UI exposes the live
> bookmarks option as one of the readers, the options UI doesn't (see the
> radiogroup in the feed preferences pane).

I think that the feed preferences pane UI should consolidate Ben's current modal dialog design to take advantage of the extra space. Something like:

When I click on a web feed:

 (o) Show me a preview and ask me which Feed Reader to use
 ( ) Subscribe to the feed using:
       .---------------------------------------.
       | [#] Live Bookmarks                    |
       | [V] Vienna                            |
       | [B] Bloglines                         |
       | [Y] My Yahoo!                         |
       | [G] Google Reader                     |
       '---------------------------------------'
                             (Add New Reader...)

Comment 18

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

(In reply to comment #17)
Mano and I talked on IRC, and it seems that we only remember a single system based RSS reader at the moment, I guess to avoid having to manage the list (ie: add and remove) or something. At any rate, we decided to modify the prefpane to be more like:

 When I click on a web feed:
 
  (o) Show me a preview and ask me which Feed Reader to use
  ( ) Subscribe to the feed using:
        .---------------------------------------.
        | [#] Live Bookmarks                    |
        |---------------------------------------|
        | [V] Vienna     (Select Application...)|
        |---------------------------------------|
        | [B] Bloglines                         |
        | [Y] My Yahoo!                         |
        | [G] Google Reader                     |
        '---------------------------------------' 

The order there isn't right IIRC, actually. I think the application comes last?

Comment 19

[Jeff Walden [:Waldo]]

(In reply to comment #18)
> The order there isn't right IIRC, actually. I think the application comes last?

I think the application should come first, because the user has to have done something to have it on his system.  Unless Dell/etc. start shipping feed readers with new systems, the user must have expressed a preference for the application by installing it, whereas he's expressed no such preference for the web-based readers with which we'll end up shipping.

Honestly, tho, this is pretty much going to be a set-and-forget preference, so I don't think it requires too much agonizing.

Comment 20

[Asaf Romano (gone)]

Attachment: more...

Comment 21

[Asaf Romano (gone)]

Attachment: patch

Things left to do post beta 2:
  - Re-write and re-enable default system feed reader code.
  - Restore the feed icon in non-first-run mode feed preview
  - style "No Application Selected" as disabled.

Comment 22

[Mike Connor [:mconnor]]

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


+<!-- XXXmano this has to be in one line. Otherwise you would see
+     how much XUL-in-XHTML sucks -->

meaning its ugly?  or it parses weird?

+<div id="feedSubscribeLine"><xul:vbox><xul:hbox align="center"><xul:description>&subscribeUsing;</xul:description><xul:menulist id="handlersMenuList"><xul:menupopup menugenerated="true" id="handlersMenuPopup"><xul:menuitem id="liveBookmarksMenuItem" label="Live Bookmarks" src="chrome://browser/skin/page-livemarks.png" selected="true"/><xul:menuseparator/></xul:menupopup></xul:menulist></xul:hbox><xul:hbox><xul:checkbox id="alwaysUse" checked="false"/></xul:hbox><xul:hbox align="center"><xul:spacer flex="1"/><xul:button label="&feedSubscribeNow;" id="subscribeButton"/></xul:hbox></xul:vbox></div></div>
     
     <!-- XXXben - get rid of me when the feed processor is bug free! -->
     
oh, the irony!

     switch (handler) {
     case "client":
-      try {
-        var clientApp = 
-            prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);
-      }
-      catch (e) {
-        return;
-      }
+      var clientApp = 
+          prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);

how do we know this isn't going to throw?


+  _chooseClientApp: function FW__chooseClientApp() {
+    try {
+      var fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
+      fp.init(this._window, "Subscription Options", Ci.nsIFilePicker.modeOpen);

not localized!

+    var handler = "bookmarks";
+    try {
+      handler = prefs.getCharPref(PREF_SELECTED_READER);
+    }
+    catch (ex) { }

this shouldn't be a char pref, likely.  Can you file a followup on fixing that?
  
+          var selectedApp = null;
+          try {
+            var selectedApp = prefs.getComplexValue(PREF_SELECTED_APP,
+                                                    Ci.nsILocalFile);
+          } catch(ex) { }

I think the first declaration isn't needed here.... :)


+    var selectedHandler = this._document.getElementById("handlersMenuList")
+                               .wrappedJSObject.selectedItem;

nit: alignment


 var Module = {
   QueryInterface: function M_QueryInterface(iid) {
Index: browser/components/preferences/feeds.js
+#ifndef XP_MACOSX
+var Cc = Components.classes;
+var Ci = Components.interfaces;
+var Cr = Components.results;
+var TYPE_MAYBE_FEED = "application/vnd.mozilla.maybe.feed";
+#endif

I had to add const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; to the #ifndef block here for the prefpane to work.


+
+/*
+ * Preferences:
+ *
+ * browser.feeds.handler
+ * - "bookmarks", "reader" (clarified further using the .default preference),
+ *   or "ask" -- indicates the default handler being used to process feeds
+ * browser.feeds.handler.default
+ * - "bookmarks", "client" or "web" -- indicates the chosen feed reader used
+ *   to display feeds, either transiently (i.e., when the "use as default"
+ *   checkbox is unchecked, corresponds to when browser.feeds.handler=="ask")
+ *   or more permanently (i.e., the item displayed in the dropdown in Feeds
+ *   preferences)
+ * browser.feeds.handler.webservice
+ * - the URL of the currently selected web service used to read feeds
+ * browser.feeds.handlers.application
+ * - nsILocalFile, stores the current client-side feed reading app if one has
+ *   been chosen
+ */

please leave blank lines between for readability

+const PREF_SELECTED_APP = "browser.feeds.handlers.application";
+const PREF_SELECTED_WEB = "browser.feeds.handlers.webservice";
+const PREF_SELECTED_ACTION = "browser.feeds.handler";
+const PREF_SELECTED_READER = "browser.feeds.handler.default";

picky picky, but line up the = and its easier to read

 
-  _attachPreferenceBindings: function ()
-  {
-    var feedClick = document.getElementById("feedClick");
-    feedClick.setAttribute("preference", "browser.feeds.handler");
+  /**
+   * Initializes this.
+   */
+  init: function () {
+    var _delayedPaneLoad = function(self) {
+      self._initFeedReaders();
+      self.updateSelectedReader();
+    }
+    setTimeout(_delayedPaneLoad, 0, this);
 
-    var reader = document.getElementById("chooseFeedReader");
-    reader.setAttribute("preference", "browser.feeds.handler.default");
+    // For web readers, we need to call setAutoHandler if the
+    // preview page should be skiped (i.e. PREF_SELECTED_ACTION="reader")

skipped

r=me with those fixed, really great work, I've tested this and it seems to work will on Windows and Mac branch, so with the fixes done I think this should be safe to take as long as it doesn't break tinderboxes

Comment 23

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

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

a=beltzner on behalf of drivers; we'd rather get this into a nightly before code freeze than at the last minute

Comment 24

[Asaf Romano (gone)]

Attachment: comments addressed

Comment 25

[Asaf Romano (gone)]

(In reply to comment #22)
> (From update of attachment 233905 [details] [diff] [review] [edit])
> 
> +<!-- XXXmano this has to be in one line. Otherwise you would see
> +     how much XUL-in-XHTML sucks -->
> 
> meaning its ugly?  or it parses weird?
> 

The later, I opened bug 348830 and noted it in the comment.


>      switch (handler) {
>      case "client":
> -      try {
> -        var clientApp = 
> -            prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);
> -      }
> -      catch (e) {
> -        return;
> -      }
> +      var clientApp = 
> +          prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);
> 
> how do we know this isn't going to throw?

I'm catching this in the caller instead.

> 
> +    var handler = "bookmarks";
> +    try {
> +      handler = prefs.getCharPref(PREF_SELECTED_READER);
> +    }
> +    catch (ex) { }
> 
> this shouldn't be a char pref, likely.  Can you file a followup on fixing that?

I don't think we should change the prefs scheme over again.

Comment 26

[Asaf Romano (gone)]

1.8 branch:
mozilla/browser/components/feeds/jar.mn 1.1.2.4
mozilla/browser/components/feeds/content/options.js delete
mozilla/browser/components/feeds/content/options.xul delete
mozilla/browser/components/feeds/content/subscribe.xhtml 1.1.2.8
mozilla/browser/components/feeds/public/nsIFeedWriter.idl 1.1.2.3
mozilla/browser/components/feeds/src/FeedConverter.js 1.1.2.17
mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.12
mozilla/browser/components/preferences/feeds.js 1.1.2.2
mozilla/browser/components/preferences/feeds.xul 1.1.2.2
mozilla/browser/locales/jar.mn 1.25.2.23
mozilla/browser/locales/en-US/chrome/browser/feeds/options.dtd delete
mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.dtd 1.1.2.3
mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.properties 1.1.2.3
mozilla/browser/locales/en-US/chrome/browser/preferences/feeds.dtd 1.1.2.2
mozilla/browser/locales/en-US/chrome/browser/preferences/Attic/feeds.properties 1.1.2.1

Comment 27

[Asaf Romano (gone)]

Attachment: as checked in

Comment 28

[Jeff Walden [:Waldo]]

(In reply to comment #22)
> +/*
> + * Preferences:
> + *
> + * browser.feeds.handler
> + * - "bookmarks", "reader" (clarified further using the .default preference),
> + *   or "ask" -- indicates the default handler being used to process feeds
...
> + * browser.feeds.handlers.application
> + * - nsILocalFile, stores the current client-side feed reading app if one has
> + *   been chosen
> + */
> 
> please leave blank lines between for readability

This is counter to the style followed throughout the entire codebase.  (Which is really just browser/components/preferences because it's the only part that documents prefs like this, but saying it that way sounds more impressive. ;-) )  I've even requested once that such blank lines be removed from a checkin to fit prevailing style.

I don't really dislike the blank-lines way, but if you want blank lines here, please insert them in all the other files that document preferences in this way.  (should be just b/c/preferences/*)

Comment 29

[Justin Wood (:Callek)]

re: c#26

mozilla/browser/locales/en-US/chrome/browser/preferences/Attic/feeds.properties

Umm, ignore me if I'm being ignorant here, but is this right?

Comment 30

[Asaf Romano (gone)]

branch-only file, yeah.

Comment 31

[Asaf Romano (gone)]

Attachment: Trunk patch

Note this is semi-broken due to bug 349477, but since you can still use the keybaord to activate the elements (plus I've included a workaround for the subscribe button), I'm going to check this in.

This also includes the patches for bug 348968 and bug 349318.

Comment 32

[Asaf Romano (gone)]

mozilla/browser/components/feeds/jar.mn 1.5
mozilla/browser/components/feeds/content/options.js delete
mozilla/browser/components/feeds/content/options.xul delete
mozilla/browser/components/feeds/content/subscribe.xhtml 1.9
mozilla/browser/components/feeds/public/nsIFeedWriter.idl 1.3
mozilla/browser/components/feeds/src/FeedConverter.js 1.18
mozilla/browser/components/feeds/src/FeedWriter.js 1.11
mozilla/browser/components/preferences/feeds.js  1.2
mozilla/browser/components/preferences/feeds.xul 1.2
mozilla/browser/locales/jar.mn 1.56; previous revision
mozilla/browser/locales/en-US/chrome/browser/feeds/options.dtd delete
mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.dtd 1.4
mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.properties 1.4
mozilla/browser/locales/en-US/chrome/browser/preferences/feeds.dtd 1.2
mozilla/browser/locales/en-US/chrome/browser/preferences/feeds.properties 1.2
mozilla/browser/themes/winstripe/browser/feeds/subscribe.css 1.10

I will open new bugs for the issues left.

Comment 33

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060821 Minefield/3.0a1 ID:2006082102 [cairo]

Subscribeto this feed useng "", drop-down menu is broken.
http://img87.imageshack.us/img87/5108/ddgz9.jpg

Comment 34

[Asaf Romano (gone)]

See comment 31, on trunk you can only use the menulist (and the checkbox) by the keyboard

Comment 35

[pal-moz]

(In reply to comment #34)
> See comment 31, on trunk you can only use the menulist (and the checkbox) by
> the keyboard
> 

ok,I re-tried and noticed,
click on drop-down (triangle mark) dose not work,
but click on text (Live Bookmarks, Bloglines ...) works, I can choose.

is this the intended behavior now ?

Comment 36

[Asaf Romano (gone)]

Sigh, no, please read comment 31.

Comment 37

[Steffen Wilberg]

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