596731 - Add web feed control to the toolbar customization palette

Status: RESOLVED FIXED

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

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

While subscribing to pages isn't a very common action for mainstreme users, there are a small number of users who very reguarly subscribe to feeds, or visually check to see if a feed exists.  To enable this funtionality in primary UI, we should add an optional control to the toolbar customization pallete.

Comment 1

[Paul [pwd]]

I was just about to file this.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

I would like this to get picked up during the polish phase.  I don't think it actually needs to block release but want to get it on people's radar.  If we had a flag for wanted+ that would be the most fitting.

Comment 3

[Dave Garrett]

(In reply to comment #2)
> If we had a flag for wanted+ that would be the most fitting.

https://wiki.mozilla.org/Releases/Flags
says status2.0 can be set to wanted+

Comment 4

[Phil Ringnalda (:philor)]

Don't toolbar icons have text, or more pointedly [strings]?

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: add "Feed Indicator" string

If it's needed, here it is.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: actual patch this time

Sorry, other changes found their way into that patch.

Comment 7

[Paul [pwd]]

(In reply to comment #2)
> I would like this to get picked up during the polish phase.  I don't think it
> actually needs to block release but want to get it on people's radar.  If we
> had a flag for wanted+ that would be the most fitting.

I believe this should actually block the removal of the location bar icon. If for no other reason than to appease the disgruntled users that have spoken up.

Comment 8

[Paul [pwd]]

(In reply to comment #6)
> Created attachment 475765 [details] [diff] [review]
> actual patch this time
> 
> Sorry, other changes found their way into that patch.

What's missing from this patch? From what I can tell, the function of the button plus the graphics aremissing. The button will have to have three graphics I assume? One for tab/title bar, one or navigation bar and one for addons bar right?

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

I would go with "Subscribe" or perhaps "Web Feed" saying indicator seems a little too literal.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 475765 [details] [diff] [review]
actual patch this time

I would prefer the text to be "Subscribe" but the overall objective is to just get this landed.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: changed string

(In reply to comment #8)
> What's missing from this patch? From what I can tell, the function of the
> button plus the graphics aremissing. The button will have to have three
> graphics I assume? One for tab/title bar, one or navigation bar and one for
> addons bar right?

Yeah, I was under the impression that they first need to beat the string freeze first, so this only has the Customize pallet's new string for the button. The rest of the functionality already exists in the browser, just needs moved around.

(In reply to comment #10)
> Comment on attachment 475765 [details] [diff] [review]
> actual patch this time
> 
> I would prefer the text to be "Subscribe" but the overall objective is to just
> get this landed.

Done

Comment 12

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

Comment on attachment 476420 [details] [diff] [review]
changed string

r+uir=beltzner

This will only work if the toolbar button is actually created, though.

(note: Aza is not a reviewer)

Comment 13

[Paul [pwd]]

Who will take care of the actual button? I remember there was a PSD file for the new buttons, any chance anyone who has it could knock something up quickly?

Comment 14

[Csaba Kozák [:WonderCsabo]]

Here you are, but i can't do it.

http://blog.stephenhorlander.com/2010/03/29/make-your-own-toolbar-button-glyphs/

Comment 15

[Dão Gottwald [:dao]]

You'll want to add a tooltip too.

Comment 16

[Dão Gottwald [:dao]]

e.g. "Subscribe to this page".

Comment 17

[Paul [pwd]]

Based on the availability of the PSD, I can give the graphical side of the button a go. However if the button is pulled down to the Addon Bar, it's supposed to lose it's 'button-ness', and the same goes for being elevated up to the tab bar. Is that handled automatically or are three different graphics required.

Comment 18

[Dão Gottwald [:dao]]

That's handled automatically.

Comment 19

[Paul [pwd]]

Attachment: RSS Button Glyph

Is this OK?

Comment 20

[Paul [pwd]]

Attachment: RSS Button Glyph (Active)

I just got to thinking about active/inactive state, as such here is an orange version. I'm not sure what the plan is here, whether to just disable the button or colour the button, but I thought I'd provide an avenue. I really have no idea if I've done this right at all, hence the feedback request, along with any instructions that need be.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 476420 [details] [diff] [review]
changed string

see comment 15

Comment 22

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 477061 [details] [diff] [review]
string with feedButton.tooltip

feedButton.tooltip actually exists already -- bug 578967 didn't remove it when it should. So all we need to do is move it next to feedButton.label.

Comment 24

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved

Comment 25

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 477065 [details] [diff] [review]
string with feedButton.tooltip moved

This removes the ellipsis from the tooltip -- I guess we want to keep it since clicking the button won't subscribe to the page immediately but open the UI to do so.

Comment 27

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 28

[Dão Gottwald [:dao]]

Now you changed the ellipsis from the unicode one to three dots. ;-)

Comment 29

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 30

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 31

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 32

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 477070 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis

This looks good. However, does the patch even apply? tabViewButton2.tooltip doesn't exist anymore, for instance.

Comment 34

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 35

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis

Comment 36

[Dão Gottwald [:dao]]

That patch unfortunately doesn't have any context whatsoever and also fails to apply: malformed patch a/browser/locales/en-US/chrome/browser/browser.dtd @@ -125,1 +125,1 @@  -->

Comment 37

[Csaba Kozák [:WonderCsabo]]

Sorry, my mercurial has been screwed up. 
Re-check out...
And apologies for many patches and thx for you patience.

Comment 38

[Csaba Kozák [:WonderCsabo]]

Attachment: string with feedButton.tooltip moved with ellipsis (checked in)

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 477086 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis (checked in)

http://hg.mozilla.org/mozilla-central/rev/809f3ae9b071

Comment 40

[Dão Gottwald [:dao]]

Paul, why do your images have a gray background?

Comment 41

[Paul [pwd]]

(In reply to comment #40)
> Paul, why do your images have a gray background?

Should they not do? I literally, took the PSD, copy and pasted some paths from an old RSS icon and exported. Should I disable the grey and repost?

Comment 42

[Dão Gottwald [:dao]]

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/Toolbar.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/Toolbar.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/Toolbar.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/Toolbar-small.png

Comment 43

[Dão Gottwald [:dao]]

So the background isn't supposed to be there on any platform.

Comment 44

[Paul [pwd]]

(In reply to comment #43)
> So the background isn't supposed to be there on any platform.

Thank you, that helps a lot. Do I need to submit them on the above grids or do I submit them as individual 18x18 pngs? Also should I do as above (winstripe/pinstripe/gnomestripe/gnomestripe-small?

My apologies for requesting so much guidance here.

Comment 45

[Dão Gottwald [:dao]]

In order to have something that's directly reviewable you'd have to integrate the icons into these images. Otherwise somebody else will have to do that.

Comment 46

[Paul [pwd]]

Okey doke. Is there a PSD for the above images, or should I just apply to the above PNGs?

Comment 47

[Axel Hecht [:Pike]]

FWIW, I think that pre-landing strings for bugs that aren't release blockers is a bad idea.

Comment 48

[Paul [pwd]]

Attachment: Without the background gradient

Comment 49

[Paul [pwd]]

Attachment: Colour version with background gradient removed

Comment 50

[Paul [pwd]]

So if these are deemed OK, I just need the pinstripe colour scheme and then to provide a 21x21 version on top of the above?

Comment 51

[Paul [pwd]]

Attachment: Windows RSS Icon

Comment 52

[Paul [pwd]]

Attachment: Mac RSS Icon

Comment 53

[Csaba Kozák [:WonderCsabo]]

The Windows one looks a little bit jagged to me.

Comment 54

[Paul [pwd]]

Sorry those are late. I spoke to SHorlander on IRC and the Linux images are
done, so it was just a matter of the Windows/Mac which are attached above.

I attempted to integrate into the toolbar.png's, but had a problem with that,
so sadly I might have to leave that to someone else unless I find the PSD's for
those files.

(In reply to comment #53)
> The Windows one looks a little bit jagged to me.

I'm honestly not seeing that on my monitor, can you give me a screenshot of what you're seeing please?

Comment 55

[Csaba Kozák [:WonderCsabo]]

I dont think that would help, but here you are:
http://tinyurl.com/35x89ne

My display resolution is 1366x768. Notebook lcd.

Comment 56

[Paul [pwd]]

Attachment: Mac RSS Icon

The previous didn't respect the margins sufficiently.

Comment 57

[Paul [pwd]]

Attachment: Mac RSS Icon (Active)

Previously forgotten graphic

Comment 58

[Paul [pwd]]

(In reply to comment #55)
> I dont think that would help, but here you are:
> http://tinyurl.com/35x89ne
> 
> My display resolution is 1366x768. Notebook lcd.

I'm not seeing the jaggedness you've referred to. Anyone else seeing it?

Comment 59

[Csaba Kozák [:WonderCsabo]]

Maybe it's just me, if nobody confirms it forget it.

Comment 60

[Paul [pwd]]

Attachment: Windows RSS Icon

Tweak in an attempt to fix jaggedness issue.

Comment 61

[Csaba Kozák [:WonderCsabo]]

Now it's okay for me!

Comment 62

[Paul [pwd]]

Attachment: Mac RSS Icon (Active)

Removes background gradient.

Comment 63

[Paul [pwd]]

(In reply to comment #61)
> Now it's okay for me!

Great news. These are now ready to integrate into the toolbar.PNGs

Comment 64

[Stephen Horlander [:shorlander]]

Attachment: Linux Toolbar.png

Comment 65

[Stephen Horlander [:shorlander]]

Attachment: Linux Toolbar-small.png

Comment 66

[Alex Faaborg [:faaborg] (Firefox UX)]

Similar to code review, I think we should probably have Stephen do a quick review of any PSD source files just to make sure everything is in perfect order.  Also, contributions with source files for images (!), how awesome is that :)

Comment 67

[Paul [pwd]]

Is it OK to them up and add them as a single attachment?

Comment 68

[Paul [pwd]]

Just to bring everyone up to speed. I sent shorlander the PSDs and asked him if he could integrate the new images into the various toolbar.png files.

Comment 69

[:Margaret Leibovic]

(In reply to comment #68)
> Just to bring everyone up to speed. I sent shorlander the PSDs and asked him if
> he could integrate the new images into the various toolbar.png files.

What's the current status of this? I can make the button if I have the updated Toolbar.png.

Comment 70

[Paul [pwd]]

(In reply to comment #69)
> (In reply to comment #68)
> > Just to bring everyone up to speed. I sent shorlander the PSDs and asked him if
> > he could integrate the new images into the various toolbar.png files.
> 
> What's the current status of this? I can make the button if I have the updated
> Toolbar.png.

SHorlander has the PSDs now. I can email them to you if you like, however I don't have the PSD of the Toolbar.PNGs, so you'd have to talk to Steven for those.

Comment 71

[Stephen Horlander [:shorlander]]

Attachment: Windows Toolbar.png

Comment 72

[Stephen Horlander [:shorlander]]

Attachment: Mac Toolbar.png

Comment 73

[:Margaret Leibovic]

Thanks, Stephen! I'll make a patch now.

Comment 74

[:Margaret Leibovic]

Attachment: patch

I tested this on Windows 7, Ubuntu, and OSX.

Comment 75

[Justin Dolske [:Dolske]]

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

>+    var feedButton = document.getElementById("feed-button");


A little sadmaking to be doing this often, but it was already that way, shouldn't be slow, and caching would probably be tricky.

Comment 76

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/fc828f784051

Comment 77

[Pascal Chevrel:pascalc]

I expected the button to have a disabled state for sites without a rss/atom feed, currently the same image is displayed for all sites, is that the expected behaviour for this bug or should I file a separate bug for that?

Comment 78

[:Margaret Leibovic]

(In reply to comment #77)
> I expected the button to have a disabled state for sites without a rss/atom
> feed, currently the same image is displayed for all sites, is that the expected
> behaviour for this bug or should I file a separate bug for that?

The button should be disabled for sites without an rss/atom feed. Your problem may be caused by bug 597644.

Comment 79

[Pascal Chevrel:pascalc]

>The button should be disabled for sites without an rss/atom feed. Your problem
>may be caused by bug 597644.

I have a disabled state in Windows XP but not in Linux (latest nightlies)

Comment 80

[Pascal Chevrel:pascalc]

Using the Dom inspector I can see that there is no css rule defined on the Linux version for the disabled version of the button while the XP version of the feed icon inherits from this rule in chrome://browser/skin/browser.css:

.toolbarbutton-1[disabled="true"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
.toolbarbutton-1[disabled="true"] > .toolbarbutton-icon {
  opacity: .5;
}

Since we are not using a GTK icon for the feed (I don't think there is a stock icon available for that) but an image, my understanding is that we need to define the disabled state of the image by ourselves like it is for windows, maybe for example:

#feed-button.toolbarbutton-1[disabled="true"] > .toolbarbutton-icon {
  opacity: .5;
}

Should a new bug be opened for that or should this bug be reopened?

Comment 81

[Pascal Chevrel:pascalc]

also, I don't have access to a mac, maybe worth checking that it works correctly on this platform

Comment 82

[Hideo Oshima]

(In reply to comment #79)
> >The button should be disabled for sites without an rss/atom feed. Your problem
> >may be caused by bug 597644.
> 
> I have a disabled state in Windows XP but not in Linux (latest nightlies)

I filed bug 604972 in the Linux case.

Comment 83

[Jean-Alexandre Anglès d'Auriac]

This icon is taking a lot of place, and is totally useless when there is no feed on the web page. I think it should reverse to the old behavior of an icon inside the address bar if put next to it.

Comment 84

[Paul [pwd]]

(In reply to comment #83)
> cc: [e-mail] icon is taking a lot of place, and is totally useless when there is no
> feed on the web page. I think it should reverse to the old behavior of an icon
> inside the address bar if put next to it.

If you'd really like this backed out, file a separate bug, no need to spam this one. Alternatively, here's a userstyle to achieve what you're after: http://userstyles.org/styles/38569/firefox-4-feeds-icon-in-location-bar