The default bug view has changed. See this FAQ.

Add web feed control to the toolbar customization palette

RESOLVED FIXED

Status

()

Firefox
RSS Discovery and Preview
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: faaborg, Assigned: Margaret)

Tracking

(Depends on: 1 bug, {icon})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

Attachments

(8 attachments, 21 obsolete attachments)

1.88 KB, patch
dao
: review+
Details | Diff | Splinter Review
747 bytes, image/png
Details
1.03 KB, image/png
Details
6.73 KB, image/png
Details
4.32 KB, image/png
Details
12.42 KB, image/png
Details
10.83 KB, image/png
Details
55.71 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
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.
(Reporter)

Updated

7 years ago
Keywords: icon

Comment 1

7 years ago
I was just about to file this.

Updated

7 years ago
Summary: Add web feed control to the toolbar customization pallet → Add web feed control to the toolbar customization palette

Updated

7 years ago
Depends on: 578967
Version: unspecified → Trunk
(Reporter)

Comment 2

7 years ago
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.
blocking2.0: --- → ?

Comment 3

7 years ago
(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+
Don't toolbar icons have text, or more pointedly [strings]?
Created attachment 475762 [details] [diff] [review]
add "Feed Indicator" string

If it's needed, here it is.
Attachment #475762 - Flags: review?
Attachment #475762 - Flags: feedback?(faaborg)
Attachment #475762 - Flags: approval2.0?
Created attachment 475765 [details] [diff] [review]
actual patch this time

Sorry, other changes found their way into that patch.
Attachment #475762 - Attachment is obsolete: true
Attachment #475765 - Flags: review?
Attachment #475765 - Flags: feedback?(faaborg)
Attachment #475765 - Flags: approval2.0?
Attachment #475762 - Flags: review?
Attachment #475762 - Flags: feedback?(faaborg)
Attachment #475762 - Flags: approval2.0?

Comment 7

7 years ago
(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

7 years ago
(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?
(Reporter)

Comment 9

7 years ago
I would go with "Subscribe" or perhaps "Web Feed" saying indicator seems a little too literal.
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.
Attachment #475765 - Flags: feedback?(faaborg) → feedback-
Created attachment 476420 [details] [diff] [review]
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
Attachment #475765 - Attachment is obsolete: true
Attachment #476420 - Flags: review?(aza)
Attachment #476420 - Flags: approval2.0?
Attachment #475765 - Flags: review?
Attachment #475765 - Flags: approval2.0?

Updated

7 years ago
Whiteboard: [strings]
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)
Attachment #476420 - Flags: review?(aza)
Attachment #476420 - Flags: review+
Attachment #476420 - Flags: approval2.0?
Attachment #476420 - Flags: approval2.0+

Comment 13

7 years ago
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?
Here you are, but i can't do it.

http://blog.stephenhorlander.com/2010/03/29/make-your-own-toolbar-button-glyphs/
You'll want to add a tooltip too.
e.g. "Subscribe to this page".

Comment 17

7 years ago
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.
That's handled automatically.

Comment 19

7 years ago
Created attachment 476905 [details]
RSS Button Glyph

Is this OK?
Attachment #476905 - Flags: feedback?

Comment 20

7 years ago
Created attachment 476933 [details]
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.
Attachment #476933 - Flags: feedback?
Comment on attachment 476420 [details] [diff] [review]
changed string

see comment 15
Attachment #476420 - Flags: review-
Created attachment 477061 [details] [diff] [review]
string with feedButton.tooltip
Attachment #477061 - Flags: review?
Attachment #477061 - Flags: review? → review?(dao)

Updated

7 years ago
Attachment #477061 - Flags: review?(dao) → review+
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.
Attachment #477061 - Flags: review+
Created attachment 477064 [details]
string with feedButton.tooltip moved
Attachment #477061 - Attachment is obsolete: true
Created attachment 477065 [details] [diff] [review]
string with feedButton.tooltip moved
Attachment #477064 - Attachment is obsolete: true
Attachment #477065 - Flags: review?(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.
Created attachment 477066 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477065 - Attachment is obsolete: true
Attachment #477065 - Flags: review?(dao)
Now you changed the ellipsis from the unicode one to three dots. ;-)
Created attachment 477068 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477066 - Attachment is obsolete: true
Created attachment 477069 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477068 - Attachment is obsolete: true
Created attachment 477070 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477069 - Attachment is obsolete: true
Created attachment 477073 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477070 - Attachment is obsolete: true
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.
Attachment #477070 - Attachment is obsolete: false
Attachment #477073 - Flags: review?(dao)
Created attachment 477074 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477070 - Attachment is obsolete: true
Attachment #477073 - Attachment is obsolete: true
Attachment #477073 - Flags: review?(dao)
Created attachment 477075 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis
Attachment #477074 - Attachment is obsolete: true
Attachment #477075 - Flags: review?(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 @@  -->
Sorry, my mercurial has been screwed up. 
Re-check out...
And apologies for many patches and thx for you patience.
Created attachment 477086 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis (checked in)
Attachment #477075 - Attachment is obsolete: true
Attachment #477075 - Flags: review?(dao)
Attachment #477086 - Flags: review?(dao)

Updated

7 years ago
Attachment #476905 - Flags: feedback? → feedback?(shorlander)

Updated

7 years ago
Attachment #476933 - Flags: feedback? → feedback?(shorlander)

Updated

7 years ago
Attachment #477086 - Flags: review?(dao) → review+
Comment on attachment 477086 [details] [diff] [review]
string with feedButton.tooltip moved with ellipsis (checked in)

http://hg.mozilla.org/mozilla-central/rev/809f3ae9b071
Attachment #477086 - Attachment description: string with feedButton.tooltip moved with ellipsis → string with feedButton.tooltip moved with ellipsis (checked in)

Updated

7 years ago
Attachment #476420 - Attachment is obsolete: true
Paul, why do your images have a gray background?

Updated

7 years ago
Whiteboard: [strings]

Comment 41

7 years ago
(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?
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
So the background isn't supposed to be there on any platform.

Comment 44

7 years ago
(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.
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

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

Comment 47

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

Comment 48

7 years ago
Created attachment 477140 [details]
Without the background gradient
Attachment #476905 - Attachment is obsolete: true
Attachment #477140 - Flags: feedback?(shorlander)
Attachment #476905 - Flags: feedback?(shorlander)

Comment 49

7 years ago
Created attachment 477142 [details]
Colour version with background gradient removed
Attachment #476933 - Attachment is obsolete: true
Attachment #477142 - Flags: feedback?(shorlander)
Attachment #476933 - Flags: feedback?(shorlander)

Comment 50

7 years ago
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?
blocking2.0: ? → beta8+

Comment 51

7 years ago
Created attachment 477437 [details]
Windows RSS Icon
Attachment #477140 - Attachment is obsolete: true
Attachment #477142 - Attachment is obsolete: true
Attachment #477140 - Flags: feedback?(shorlander)
Attachment #477142 - Flags: feedback?(shorlander)

Comment 52

7 years ago
Created attachment 477438 [details]
Mac RSS Icon
The Windows one looks a little bit jagged to me.

Comment 54

7 years ago
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?
I dont think that would help, but here you are:
http://tinyurl.com/35x89ne

My display resolution is 1366x768. Notebook lcd.

Comment 56

7 years ago
Created attachment 477472 [details]
Mac RSS Icon

The previous didn't respect the margins sufficiently.
Attachment #477438 - Attachment is obsolete: true

Comment 57

7 years ago
Created attachment 477473 [details]
Mac RSS Icon (Active)

Previously forgotten graphic

Comment 58

7 years ago
(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?
Maybe it's just me, if nobody confirms it forget it.

Comment 60

7 years ago
Created attachment 477477 [details]
Windows RSS Icon

Tweak in an attempt to fix jaggedness issue.
Attachment #477437 - Attachment is obsolete: true
Now it's okay for me!

Comment 62

7 years ago
Created attachment 477480 [details]
Mac RSS Icon (Active)

Removes background gradient.
Attachment #477473 - Attachment is obsolete: true

Comment 63

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

Great news. These are now ready to integrate into the toolbar.PNGs
Created attachment 477616 [details]
Linux Toolbar.png
Created attachment 477617 [details]
Linux Toolbar-small.png
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

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

Comment 68

7 years ago
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.

Updated

7 years ago
Attachment #477480 - Attachment is obsolete: true
(Assignee)

Comment 69

7 years ago
(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

7 years ago
(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.
Created attachment 481985 [details]
Windows Toolbar.png
Created attachment 481986 [details]
Mac Toolbar.png
(Assignee)

Comment 73

7 years ago
Thanks, Stephen! I'll make a patch now.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 74

7 years ago
Created attachment 482350 [details] [diff] [review]
patch

I tested this on Windows 7, Ubuntu, and OSX.
Attachment #482350 - Flags: review?(dolske)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review 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.
Attachment #482350 - Flags: review?(dolske) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dolske] → [can land]
(Assignee)

Comment 76

7 years ago
http://hg.mozilla.org/mozilla-central/rev/fc828f784051
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [can land]

Updated

7 years ago
Depends on: 605072

Updated

7 years ago
No longer depends on: 605072
Depends on: 607324
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?
(Assignee)

Comment 78

7 years ago
(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.
No longer depends on: 607324
>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)
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?
also, I don't have access to a mac, maybe worth checking that it works correctly on this platform

Updated

6 years ago
Blocks: 621081

Comment 82

6 years ago
(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.

Updated

6 years ago
No longer blocks: 621081
Depends on: 621081

Updated

6 years ago
Depends on: 623172
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

6 years ago
(In reply to comment #83)
> cc: jagw40k@free.frThis 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
You need to log in before you can comment on or make changes to this bug.