Closed Bug 596731 Opened 10 years ago Closed 10 years ago

Add web feed control to the toolbar customization palette

Categories

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

defect
Not set
normal

Tracking

(blocking2.0 beta8+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: faaborg, Assigned: Margaret)

References

Details

(Keywords: icon)

Attachments

(8 files, 21 obsolete files)

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.
Keywords: icon
I was just about to file this.
Summary: Add web feed control to the toolbar customization pallet → Add web feed control to the toolbar customization palette
Depends on: 578967
Version: unspecified → Trunk
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: --- → ?
(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]?
Attached patch add "Feed Indicator" string (obsolete) — Splinter Review
If it's needed, here it is.
Attachment #475762 - Flags: review?
Attachment #475762 - Flags: feedback?(faaborg)
Attachment #475762 - Flags: approval2.0?
Attached patch actual patch this time (obsolete) — Splinter Review
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?
(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.
(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?
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-
Attached patch changed string (obsolete) — Splinter Review
(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?
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+
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?
You'll want to add a tooltip too.
e.g. "Subscribe to this page".
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.
Attached image RSS Button Glyph (obsolete) —
Is this OK?
Attachment #476905 - Flags: feedback?
Attached image RSS Button Glyph (Active) (obsolete) —
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-
Attachment #477061 - Flags: review? → review?(dao)
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+
Attached file string with feedButton.tooltip moved (obsolete) —
Attachment #477061 - Attachment is obsolete: true
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.
Attachment #477065 - Attachment is obsolete: true
Attachment #477065 - Flags: review?(dao)
Now you changed the ellipsis from the unicode one to three dots. ;-)
Attachment #477066 - Attachment is obsolete: true
Attachment #477068 - Attachment is obsolete: true
Attachment #477069 - Attachment is obsolete: true
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)
Attachment #477070 - Attachment is obsolete: true
Attachment #477073 - Attachment is obsolete: true
Attachment #477073 - Flags: review?(dao)
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.
Attachment #477086 - Flags: review?(dao)
Attachment #476905 - Flags: feedback? → feedback?(shorlander)
Attachment #476933 - Flags: feedback? → feedback?(shorlander)
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)
Attachment #476420 - Attachment is obsolete: true
Paul, why do your images have a gray background?
Whiteboard: [strings]
(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?
So the background isn't supposed to be there on any platform.
(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.
Okey doke. Is there a PSD for the above images, or should I just apply to the above PNGs?
FWIW, I think that pre-landing strings for bugs that aren't release blockers is a bad idea.
Attached image Without the background gradient (obsolete) —
Attachment #476905 - Attachment is obsolete: true
Attachment #477140 - Flags: feedback?(shorlander)
Attachment #476905 - Flags: feedback?(shorlander)
Attachment #476933 - Attachment is obsolete: true
Attachment #477142 - Flags: feedback?(shorlander)
Attachment #476933 - Flags: feedback?(shorlander)
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+
Attached image Windows RSS Icon (obsolete) —
Attachment #477140 - Attachment is obsolete: true
Attachment #477142 - Attachment is obsolete: true
Attachment #477140 - Flags: feedback?(shorlander)
Attachment #477142 - Flags: feedback?(shorlander)
Attached image Mac RSS Icon (obsolete) —
The Windows one looks a little bit jagged to me.
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.
Attached image Mac RSS Icon
The previous didn't respect the margins sufficiently.
Attachment #477438 - Attachment is obsolete: true
Attached image Mac RSS Icon (Active) (obsolete) —
Previously forgotten graphic
(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.
Attached image Windows RSS Icon
Tweak in an attempt to fix jaggedness issue.
Attachment #477437 - Attachment is obsolete: true
Now it's okay for me!
Attached image Mac RSS Icon (Active) (obsolete) —
Removes background gradient.
Attachment #477473 - Attachment is obsolete: true
(In reply to comment #61)
> Now it's okay for me!

Great news. These are now ready to integrate into the toolbar.PNGs
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 :)
Is it OK to them up and add them as a single attachment?
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.
Attachment #477480 - Attachment is obsolete: true
(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.
(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.
Thanks, Stephen! I'll make a patch now.
Assignee: nobody → margaret.leibovic
Attached patch patchSplinter Review
I tested this on Windows 7, Ubuntu, and OSX.
Attachment #482350 - Flags: review?(dolske)
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+
Whiteboard: [needs review dolske] → [can land]
http://hg.mozilla.org/mozilla-central/rev/fc828f784051
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Depends on: 605072
No longer depends on: 605072
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?
(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
Blocks: 621081
(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.
No longer blocks: 621081
Depends on: 621081
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.
(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
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.