Closed Bug 985659 Opened 6 years ago Closed 4 years ago

Improve usability and styling of feed button's menu, remove bookmarks menu button's "subscribe to" menuitem/submenu

Categories

(Firefox :: Theme, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ge3k0s, Assigned: lewis.cowper, Mentored)

References

Details

(Whiteboard: [qx:link:spec] [diamond] [lang=css] [lang=js])

Attachments

(2 files, 4 obsolete files)

The "Subscribe to this page" item in the bookmarks widget can open a submenu. This menu is the only one that doesn't use the new styling from bug 969592. It should.
Blocks: 969592
Summary: RSS feed menu doesn't have the new bookmark menu style → RSS feed menu doesn't use the new bookmark menu style
Whiteboard: [Australis:P?]
We may actually evaluate to remove it, since the RSS widget is duping this functionality (also in a much better way, since it also signals the existence of feeds) and can be very easily kept in the panel ui now.
Whiteboard: [Australis:P?] → [Australis:P4-]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 989770
Attached image 989770-screenshot.png
Screenshot posted to bug 989770
(In reply to Marco Bonardo [:mak] from comment #1)
> We may actually evaluate to remove it, since the RSS widget is duping this
> functionality (also in a much better way, since it also signals the
> existence of feeds) and can be very easily kept in the panel ui now.

I would be in favour. What would be needed to make this happen?
Flags: needinfo?(mak77)
Flags: firefox-backlog+
(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Marco Bonardo [:mak] from comment #1)
> > We may actually evaluate to remove it, since the RSS widget is duping this
> > functionality (also in a much better way, since it also signals the
> > existence of feeds) and can be very easily kept in the panel ui now.
> 
> I would be in favour. What would be needed to make this happen?

I wouldn't have problems with the change, provided we keep providing the RSS button. With Australis customization it's even easier to add it to a toolbar or to the menu.

There is some usability issue with the widget contents though, when the page provides multiple feeds, the "Subscribe To" text takes most of the space, so it's basically impossible to read the feeds titles. This makes the bookmarks menu entry much more usable (for example try the difference on http://hwupgrade.it), so before doing the removal would be nice to find a way to reduce impact of this downside (maybe even just removing "Subscribe to" prefix, since the RSS widget connection is pretty clear, and it's still going to open the handler before actually subscribing). UX feedback may be useful here.
Flags: needinfo?(mak77)
Whiteboard: [Australis:P4-] → [Australis:P4-] p=2
Depends on: 995335
Points: --- → 2
Whiteboard: [Australis:P4-] p=2 → [Australis:P4-]
Feature removals might be good next bugs. Can I get a mentor and DXR link for this?
Whiteboard: [Australis:P4-] → [Australis:P4-][diamond]
(In reply to Mike Hoye [:mhoye] from comment #6)
> Feature removals might be good next bugs. Can I get a mentor and DXR link
> for this?

Let's doublecheck with UX first. Philipp?
Flags: needinfo?(philipp)
Can Comment 5 be adressed first please?
Just casking since "Firefox UI" + "removal without non-addon alternative" = "outcry" in these days.
(In reply to XtC4UaLL [:xtc4uall] from comment #8)
> Can Comment 5 be adressed first please?
> Just casking since "Firefox UI" + "removal without non-addon alternative" =
> "outcry" in these days.

I'm really confused what this has to do with comment 5. I took it as read that this bug, should we go ahead with removing the bookmarks menu button entries, would involve fixing the "Subscribe To..." strings in the feed button menu. And I don't see what *that* has to do with the "no add-on alternative" - there is an alternative *in Firefox itself*; in fact, I think we should keep the "regular" bookmarks menu subscribe submenu, if only because we need a keyboard-accessible way to do that (and that one is not misstyled, so...).
... I took comment 6 as the RSS panel's feed items descriptions *not* being adjusted/cared about - just read "removal"; no consensus has been stated.
Removing the item from the bookmarks menu is a good idea since that menu is already very complex.

I agree that before doing so, we should address the issues with the feeds button.
How about adding a disabled entry at the top of the menu that says »Subscribe to a feed« and then just listing the feeds?
Flags: needinfo?(philipp)
Blocks: 1021101
Whiteboard: [Australis:P4-][diamond] → [Australis:P4-][diamond][qx]
So, it looks like there are two parts to this bug.
1) At https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#940 we need to pass an extra argument of 'true', to tell the feed list builder to build nicer menuitems.
2) At https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#871 we need to add space at the bottom because this submenu doesn't have a footer.  The selector for that is '#BMB_subscribeToPageSubmenuMenupopup > .popup-internal-box > .arrowscrollbox-scrollbox > .scrollbox-innerbox'.

As mentioned above, you can test the submenu at http://hwupgrade.it/
Mentor: bwinton
Whiteboard: [Australis:P4-][diamond][qx] → [Australis:P4-][good-first-bug][qx]
(Looks like a good bug to start people contributing with…)
Flags: needinfo?(mhoye)
comment #5 also needs to be addressed, and the actual submenu in the bookmarks menu needs to be removed. I think it works as a mentored bug, but not really as 'good first'.
Whiteboard: [Australis:P4-][good-first-bug][qx] → [Australis:P4-][qx]
Those are issues with the Feed button, and shouldn't block the fixing of the styles of the bookmark menu item, which is what this bug is about.

(They totally block bug 1021101, but we should file a follow-up bug to track and fix them.)
(In reply to Blake Winton (:bwinton) from comment #15)
> Those are issues with the Feed button, and shouldn't block the fixing of the
> styles of the bookmark menu item, which is what this bug is about.
> 
> (They totally block bug 1021101, but we should file a follow-up bug to track
> and fix them.)

Unless I'm misreading, comment #1 says we should consider removing the bookmark menu (button) item for the feed bits, and comment #4, comment #7 and comment #11 concur with that. I don't think it makes sense to spend time fixing the styling of the submenu if we're going to remove it.

We shouldn't do that removal before fixing the feed button styles. So this bug effectively needs to:

(1) fix the feed button's menu's styling
(2) remove the item from the bookmarks menu button menu (but not the toplevel bookmarks menu, which also doesn't need styling changes so that's OK)

If we want to do (2) in bug 1021101, that's fine, but then that means this bug should address (1).

Personally, I see no reason not to just address both (1) and (2) here and dupe bug 1021101 here.
We should definitely consider removing it, after fixing the feed button's styling, but given how small the patch to fix the bookmarks menu button submenu, why not do that, and make Firefox a little better while we wait?

Then we could address (1) and (2) in bug 1021101…  ;)
(In reply to Blake Winton (:bwinton) from comment #17)
> We should definitely consider removing it, after fixing the feed button's
> styling, but given how small the patch to fix the bookmarks menu button
> submenu, why not do that, and make Firefox a little better while we wait?
> 
> Then we could address (1) and (2) in bug 1021101…  ;)

Because I think we can address (1) and (2) in a relatively small patch, too. :-)
And yet no-one has for a year, and no-one updated this bug's summary to indicate the new direction.
If we don't want to do this, that's great, let's mark it WONTFIX, and get on with our lives…
(Or update the description to indicate what this bug is currently about.)
Flags: needinfo?(mhoye)
(In reply to Blake Winton (:bwinton) from comment #19)
> And yet no-one has for a year, and no-one updated this bug's summary to
> indicate the new direction.

Fixed! :-)
OS: Windows 7 → All
Summary: RSS feed menu doesn't use the new bookmark menu style → Improve usability and styling of feed button's menu, remove bookmarks menu button's "subscribe to" menuitem/submenu
Whiteboard: [Australis:P4-][qx] → [qx]
Alright!  So, it looks like we can add a new label here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#152
(Perhaps using the existing subscribeToPageMenupopup.label entity!)

and just use the baseTitle (instead of the labelStr) here:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-feeds.js#53

and finally remove the menus and associated styles from:
https://dxr.mozilla.org/mozilla-central/search?q=BMB_subscribe

and I think we'll be done!  (Since this probably isn't a [good-first-bug], I'll mark it [diamond] instead.)
Flags: needinfo?(mhoye)
Whiteboard: [qx] → [qx] [diamond] [lang=css] [lang=js]
I'd like to work with Blake on this, if that's okay. Will begin work once I'm home tonight, so that'll be about 8 hours from now.
(In reply to Lewis Cowper from comment #22)
> I'd like to work with Blake on this, if that's okay. Will begin work once
> I'm home tonight, so that'll be about 8 hours from now.

Sounds perfect!
Assignee: nobody → lewis.cowper
Status: NEW → ASSIGNED
I see my info is no longer needed.

Very well, then. Proceed.
Flags: needinfo?(mhoye)
Attached patch Changes as suggested (obsolete) — Splinter Review
I think I'm pretty much there, I want to test it properly, but that'll have to wait until tomorrow, I figure I can put the patch up and anything obvious I've missed or done that is obvious can be sorted out before that.
Flags: needinfo?(bwinton)
Comment on attachment 8563093 [details] [diff] [review]
Changes as suggested

Review of attachment 8563093 [details] [diff] [review]:
-----------------------------------------------------------------

So, this doesn't count as a review, but I noticed a couple of things you can fix while you're waiting for the tests…  :)

(But other than that, it looks pretty good to me!)

::: browser/base/content/browser-feeds.js
@@ +50,3 @@
>        var item = document.createElement(itemNodeType);
>        var baseTitle = feedInfo.title || feedInfo.href;
>        var labelStr = gNavigatorBundle.getFormattedString("feedShowFeedNew", [baseTitle]);

Since we're not using labelStr anymore, we can remove this line.
And since that's the only use of "feedShowFeedNew", we can also remove the line:
https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#209
Flags: needinfo?(bwinton)
Attached patch New changes as suggested (obsolete) — Splinter Review
Tacked that on bwinton, not really sure about mercurial workflow, whether there's a rebase-ish kind of thing that I could do that would squash two commits down to one.
Flags: needinfo?(bwinton)
Yes, there is!  "hg histedit -r -2" should open an editor for the last two commits.  There are instructions on what the various options are in the edit screen.

When you get some free time, http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html is a fairly interesting read, and seems like it might be useful in the future.  ;)
Flags: needinfo?(bwinton)
Attached patch feed_button-patch.diff (obsolete) — Splinter Review
Sorry it took so long to get back to this, putting it up for review as a single commit now.
Attachment #8563093 - Attachment is obsolete: true
Attachment #8563307 - Attachment is obsolete: true
Attachment #8567308 - Flags: review+
Comment on attachment 8567308 [details] [diff] [review]
feed_button-patch.diff

Awesome, but we should probably ask someone for a review, instead of just setting it as + ourselves.  ;)

I believe Gijs has some opinions on what we should be doing here, so let's ask him…  :)

Thanks!
Attachment #8567308 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8567308 [details] [diff] [review]
feed_button-patch.diff

Review of attachment 8567308 [details] [diff] [review]:
-----------------------------------------------------------------

This looks near enough perfect to me, thanks!

Can you take the one nit I gave below and request the next review from :mak (Marco Bonardo) to make sure I've not missed anything?

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +149,4 @@
>      <panelview id="PanelUI-loopapi" flex="1"/>
>  
>      <panelview id="PanelUI-feeds" flex="1" oncommand="FeedHandler.subscribeToFeed(null, event);">
> +      <label value="&subscribeToPageMenupopup.label;" class="panel-subview-header"/>

Looks like you can get rid of the entity feedsMenu.label; here:

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#161

in this case... but I'd actually suggest creating a different entity because we don't know that it'll be possible for the same text to fit here as fits in the "regular" menu (the regular menu can be much wider than the Firefox/hamburger menu) in localizations other than English.

So it might make more sense to just s/feedsMenu.label/feedsMenu2.label/, and then update the contents to match the entity you used here.
Attachment #8567308 - Flags: review?(gijskruitbosch+bugs) → feedback+
Hi :bwinton, my bad on review+, I must have misclicked.

:gijs, I'm not sure about what you mean when you said "update the contents to match the entity you used here".

Is it a case of creating the feedsMenu2.label entity here:

mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#161

and then changing it's contents to "Subscribe to This Page", in line with the subscribeToPageMenupopup.label entity?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Lewis Cowper from comment #32)
> Hi :bwinton, my bad on review+, I must have misclicked.
> 
> :gijs, I'm not sure about what you mean when you said "update the contents
> to match the entity you used here".
> 
> Is it a case of creating the feedsMenu2.label entity here:
> 
> mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/
> browser.dtd#161
> 
> and then changing it's contents to "Subscribe to This Page", in line with
> the subscribeToPageMenupopup.label entity?

That's exactly right. Basically, you can change feedsMenu to feedsMenu2, and then the contents to "Subscribe to This Page". :-)
Flags: needinfo?(gijskruitbosch+bugs)
Hey Lewis, how are things going?  Are you running into any problems I can help with?
Hi Blake, I've been somewhat snowed under with work and busy outside of that for the last few weeks. I've not run into any problems, I've just not gone further with the patch. I'm at a conference in Austria on Saturday, so I hope to catch up with the two bugs I have assigned to me over the course of the weekend. :)
Whiteboard: [qx] [diamond] [lang=css] [lang=js] → [qx:link:spec] [diamond] [lang=css] [lang=js]
Tapping on the glass here, to see how we're doing. Lewis, what do you think?
Flags: needinfo?(lewis.cowper)
Argh, I totally forgot about this. I've not even got a local build on my computer. I'll get that sorted and try and get back to where I was before and fix the things that people have talked about.

My bad. D:
Flags: needinfo?(lewis.cowper)
I hope this works, I've not used mercurial in about a year.
Attachment #8567308 - Attachment is obsolete: true
Flags: needinfo?(bwinton)
Attachment #8714747 - Flags: review?(bwinton)
Comment on attachment 8714747 [details] [diff] [review]
Updated patch with new entity added

Over to Gijs (since he reviewed the last version of this patch, and since I don't think I'm a valid reviewer for this bit of the tree
Flags: needinfo?(bwinton)
Attachment #8714747 - Flags: review?(bwinton) → review?(gijskruitbosch+bugs)
This is the same patch, but with r?mak added to the summary and without the extra trailing whitespace removals in the dtd file.
Attachment #8714747 - Attachment is obsolete: true
Attachment #8714747 - Flags: review?(gijskruitbosch+bugs)
Attachment #8715248 - Flags: review?(mak77)
Comment on attachment 8715248 [details] [diff] [review]
Improve usability and styling of feed button's menu, remove bookmarks menu button's "subscribe to" menuitem/submenu,

Actually, looking over this again, I think we're good and I'll just land this. I know Marco is really busy at the moment, and I don't think re-reviewing this is a good use of his time. r=me!
Attachment #8715248 - Flags: review?(mak77) → review+
(In reply to Lewis Cowper from comment #37)
> Argh, I totally forgot about this. I've not even got a local build on my
> computer. I'll get that sorted and try and get back to where I was before
> and fix the things that people have talked about.
> 
> My bad. D:

No worries, and thanks very much for picking this up again so quickly. Landed:

https://hg.mozilla.org/integration/fx-team/rev/806034f2553f
Regardless at first glance it looks good, thank you for fixing this.
https://hg.mozilla.org/mozilla-central/rev/806034f2553f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1219810
No longer blocks: fx-qx
No longer depends on: 995335
You need to log in before you can comment on or make changes to this bug.