870452 - When the Subscribe toolbaritem is in the panel, it should show a subview if there are more than 1 available feeds

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Open CNN.com
Put the Subscribe button in the panel
Click on the Subscribe button

Expected:
Since there are more than one feeds available, there should be a subview shown

Actual:
Need to click and hold to get a popup to show the feeds, and accessing the popup is awkward.

Comment 1

[:Gijs (out until Jan 5th; he/him)]

I'll try taking a stab at this.

Comment 2

[:Gijs (out until Jan 5th; he/him)]

Attachment: Patch

So this fixes the issue by migrating to the widget API. Changes to the API:

- widget.onCreated to be notified once a widget is created. Our existing infrastructure takes care of updating the button's disabled state on navigation, however, I believe we can get rid of calling FeedHandler.updateFeeds after customization. I've not added that change in here because I'd like someone to sanity check that assumption of mine.

- widget.onCommand's handler should be called handleWidgetCommand, and we should support handleWidgetClick for cases where toolbar buttons want to handle eg. middle click. This is now widget.onClick (I know, surprising).

- widget.onViewShowing can call preventDefault() on its event and have the view not shown.


Change for the feed button: this now means we show a panel rather than a popup menu in the case of multiple items. As we're doing that for the downloads manager anyway, I think this isn't a big deal, but maybe people disagree! :-)

(NB: note for the Europeans about comment #0: you want http://us.cnn.com/. http://edition.cnn.com (?!) which is the international version only has 1 feed. Surprise!)

Comment 3

[:Gijs (out until Jan 5th; he/him)]

Attachment: Patch v1.1 (unbitrotted)

Unbitrotting after l10n updates; also including l10n strings rather than hardcoding them...

Comment 4

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

Comment on attachment 758569 [details] [diff] [review]
Patch v1.1 (unbitrotted)

This appears to be the wrong patch.

Comment 5

[:Gijs (out until Jan 5th; he/him)]

Attachment: Correct patch

Why are all these bugs I'm working on having similar bug numbers (they both started with 8704!)

Comment 6

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

Comment on attachment 758730 [details] [diff] [review]
Correct patch

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

This was surprisingly straight-forward. Great job, Gijs!

Just two suggestions.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +767,5 @@
>          node.setAttribute("acceltext", shortcut);
>        }
>        node.setAttribute("class", "toolbarbutton-1 chromeclass-toolbar-additional");
>  
> +      let handler = this.handleWidgetCommand.bind(this, aWidget, node);

Let's call this commandHandler.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +482,5 @@
> +        aEvent.stopPropagation();
> +        return;
> +      }
> +    },
> +    onCreated: function() {

I think I want onCreated to get passed the node that's created, as opposed to having it be this. I think that's more in tune with the other functions we can declare here.

Comment 7

[:Gijs (out until Jan 5th; he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/788d4dc9aaca

Comment 8

[:Gijs (out until Jan 5th; he/him)]

https://hg.mozilla.org/mozilla-central/rev/788d4dc9aaca