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

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: 989770-screenshot.png

Screenshot posted to bug 989770

Comment 4

[:Gijs (out for now; he/him)]

(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?

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Mike Hoye [:mhoye]]

Feature removals might be good next bugs. Can I get a mentor and DXR link for this?

Comment 7

[:Gijs (out for now; he/him)]

(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?

Comment 8

[(mostly gone) XtC4UaLL [:xtc4uall]]

Can Comment 5 be adressed first please?
Just casking since "Firefox UI" + "removal without non-addon alternative" = "outcry" in these days.

Comment 9

[:Gijs (out for now; he/him)]

(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...).

Comment 10

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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?

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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/

Comment 13

[Blake Winton (:bwinton) (:☕️)]

(Looks like a good bug to start people contributing with…)

Comment 14

[:Gijs (out for now; he/him)]

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'.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

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.)

Comment 16

[:Gijs (out for now; he/him)]

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

Comment 17

[Blake Winton (:bwinton) (:☕️)]

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…  ;)

Comment 18

[:Gijs (out for now; he/him)]

(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. :-)

Comment 19

[Blake Winton (:bwinton) (:☕️)]

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.)

Comment 20

[:Gijs (out for now; he/him)]

(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! :-)

Comment 21

[Blake Winton (:bwinton) (:☕️)]

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.)

Comment 22

[Lewis Cowper]

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.

Comment 23

[:Gijs (out for now; he/him)]

(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!

Comment 24

[Mike Hoye [:mhoye]]

I see my info is no longer needed.

Very well, then. Proceed.

Comment 25

[Lewis Cowper]

Attachment: Changes as suggested

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.

Comment 26

[Blake Winton (: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

Comment 27

[Lewis Cowper]

Attachment: New changes as suggested

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.

Comment 28

[Blake Winton (: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.  ;)

Comment 29

[Lewis Cowper]

Attachment: feed_button-patch.diff

Sorry it took so long to get back to this, putting it up for review as a single commit now.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

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!

Comment 31

[:Gijs (out for now; he/him)]

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.

Comment 32

[Lewis Cowper]

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?

Comment 33

[:Gijs (out for now; he/him)]

(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". :-)

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Hey Lewis, how are things going?  Are you running into any problems I can help with?

Comment 35

[Lewis Cowper]

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. :)

Comment 36

[Mike Hoye [:mhoye]]

Tapping on the glass here, to see how we're doing. Lewis, what do you think?

Comment 37

[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:

Comment 38

[Lewis Cowper]

Attachment: Updated patch with new entity added

I hope this works, I've not used mercurial in about a year.

Comment 39

[Blake Winton (: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

Comment 40

[:Gijs (out for now; he/him)]

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

This is the same patch, but with r?mak added to the summary and without the extra trailing whitespace removals in the dtd file.

Comment 41

[:Gijs (out for now; he/him)]

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!

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/806034f2553f

Comment 43

[:Gijs (out for now; he/him)]

(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

Comment 44

[Marco Bonardo [:mak]]

Regardless at first glance it looks good, thank you for fixing this.

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/806034f2553f