940820 - fix buttons to work with australis customization

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The status and mark buttons managed their positions in currentset attributes prior to australis.  This no longer works, so the first step forward is to migrate the buttons to use the customization api enough that we're back in a working state.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: use widgets for status and mark buttons

This patch moves to using CustomizableUI.jsm to manage the positioning of social buttons.  Works great, removes the management of currentset that we had to do before (which no longer is possible)

@Gijs, I ended up having to use widget.areaType to figure out if I had called createWidget or not.  Not very clean but I don't know any other way to handle that.

Comment 2

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=533fdc1eeec4

Comment 3

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> Created attachment 8335046 [details] [diff] [review]
> use widgets for status and mark buttons
> 
> This patch moves to using CustomizableUI.jsm to manage the positioning of
> social buttons.  Works great, removes the management of currentset that we
> had to do before (which no longer is possible)
> 
> @Gijs, I ended up having to use widget.areaType to figure out if I had
> called createWidget or not.  Not very clean but I don't know any other way
> to handle that.

Did you try the widget.provider == PROVIDER_API suggestion?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8335046 [details] [diff] [review]
use widgets for status and mark buttons

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

I've not looked at the tests in detail. Generally this looks OK, but you probably need to look into the areaType thing, the styling, and the conflict of the ids.

::: browser/base/content/browser-social.js
@@ +1321,5 @@
> +      // We have no good way to know if the widget was already created since
> +      // getWidget returns a wrapper, however areaType remains null until we've
> +      // done it. widget is only null if we've created then destroyed the
> +      // widget.
> +      if (!widget || !widget.areaType)

areaType will be null if the user has moved the widget to the customization palette, too... recreating the button in that case would be a bug. Try the .provider suggestion I made earlier.

@@ +1365,5 @@
>  
>    _createButton: function(provider) {
>      if (!provider.statusURL)
> +      return;
> +    let aId =this._toolbarHelper.idFromOrgin(provider.origin);

Nit: space after equals

This button and the following button use the same ID, as far as I can tell. That's going to cause conflicts.

@@ +1371,5 @@
> +      id: aId,
> +      type: 'custom',
> +      removable: true,
> +      defaultArea: CustomizableUI.AREA_NAVBAR,
> +      allowedAreas: [  CustomizableUI.AREA_NAVBAR ], /*CustomizableUI.AREA_PANEL,*/

You can leave out the allowedAreas bit

@@ +1379,5 @@
> +        let node = document.createElement('toolbarbutton');
> +
> +        node.setAttribute('id', this.id);
> +        node.setAttribute('class', 'toolbarbutton-1 social-status-button');
> +        node.setAttribute('type', "badged");

Have you checked this is styled correctly when moved to the menupanel?

@@ +1650,5 @@
> +      id: aId,
> +      type: 'custom',
> +      removable: true,
> +      defaultArea: CustomizableUI.AREA_NAVBAR,
> +      allowedAreas: [  CustomizableUI.AREA_NAVBAR ], /*CustomizableUI.AREA_PANEL,*/

you can leave out the allowedAreas bit

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> > Created attachment 8335046 [details] [diff] [review]
> > use widgets for status and mark buttons
> > 
> > This patch moves to using CustomizableUI.jsm to manage the positioning of
> > social buttons.  Works great, removes the management of currentset that we
> > had to do before (which no longer is possible)
> > 
> > @Gijs, I ended up having to use widget.areaType to figure out if I had
> > called createWidget or not.  Not very clean but I don't know any other way
> > to handle that.
> 
> Did you try the widget.provider == PROVIDER_API suggestion?

Yes, it was always PROVIDER_XUL

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #1)

> > Did you try the widget.provider == PROVIDER_API suggestion?
> 
> Yes, it was always PROVIDER_XUL

ie. even before createWidget, and it is never PROVIDER_API

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8335046 [details] [diff] [review]
> use widgets for status and mark buttons
> 
> Review of attachment 8335046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've not looked at the tests in detail. Generally this looks OK, but you
> probably need to look into the areaType thing, the styling, and the conflict
> of the ids.

There isn't a conflict of ids.  The status button is prefixed with social-status- and the mark button with social-mark-.  The origin of the provider is used for the tail of the id.  Providers can only have one button of each type.

> ::: browser/base/content/browser-social.js
> @@ +1321,5 @@
> > +      // We have no good way to know if the widget was already created since
> > +      // getWidget returns a wrapper, however areaType remains null until we've
> > +      // done it. widget is only null if we've created then destroyed the
> > +      // widget.
> > +      if (!widget || !widget.areaType)
> 
> areaType will be null if the user has moved the widget to the customization
> palette, too... recreating the button in that case would be a bug. Try the
> .provider suggestion I made earlier.

The .provider trick doesn't work.

> @@ +1365,5 @@
> >  
> >    _createButton: function(provider) {
> >      if (!provider.statusURL)
> > +      return;
> > +    let aId =this._toolbarHelper.idFromOrgin(provider.origin);
> 
> Nit: space after equals
> 
> This button and the following button use the same ID, as far as I can tell.
> That's going to cause conflicts.

per above, no conflicts

> @@ +1371,5 @@
> > +      id: aId,
> > +      type: 'custom',
> > +      removable: true,
> > +      defaultArea: CustomizableUI.AREA_NAVBAR,
> > +      allowedAreas: [  CustomizableUI.AREA_NAVBAR ], /*CustomizableUI.AREA_PANEL,*/
> 
> You can leave out the allowedAreas bit

Hmm, I thought this would prevent the button from being dragged into the menu panel (preferable until bug 914921 and it's twin for the mark button are done)

> @@ +1379,5 @@
> > +        let node = document.createElement('toolbarbutton');
> > +
> > +        node.setAttribute('id', this.id);
> > +        node.setAttribute('class', 'toolbarbutton-1 social-status-button');
> > +        node.setAttribute('type', "badged");
> 
> Have you checked this is styled correctly when moved to the menupanel?

per above, I was expecting to prevent dropping into the menu panel until those bugs are fixed separately.  In any case, the menu panel styling doesn't matter for this patch.  This patch only addresses making pre-australis functionality work again.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: use widgets for status and mark buttons

review ? for either Mark or Gijs

This patch does not address usability in the menu panel or css within that panel, that will be handled in follow bug 914921 and bug 940155.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8335542 [details] [diff] [review]
use widgets for status and mark buttons

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

This is effectively r+ for the customizableui bits, but I don't know enough about socialapi to feel comfortable giving you an r+ without Mark having looked at the code in more detail, sorry.

::: browser/base/content/browser-social.js
@@ -59,5 @@
>  
>      gBrowser.addEventListener("ActivateSocialFeature", this._activationEventHandler.bind(this), true, true);
> -    window.addEventListener("aftercustomization", function() {
> -      if (SocialUI.enabled)
> -        SocialMarks.populateContextMenu(SocialMarks);

Out of curiosity, why is this no longer necessary?

@@ +1290,5 @@
>    this._type = type;
>  }
>  
>  ToolbarHelper.prototype = {
>    idFromOrgin: function(origin) {

Can we add the missing i to this function name or would that break outside stuff?

@@ +1318,5 @@
>      for (let provider of Social.providers) {
>        let id = this.idFromOrgin(provider.origin);
> +      let widget = CustomizableUI.getWidget(id);
> +      // We have no good way to know if the widget was already created since
> +      // getWidget returns a wrapper, however areaType remains null until we've

This comment is now out of date.

@@ +1376,5 @@
> +        let window = document.defaultView;
> +
> +        let node = document.createElement('toolbarbutton');
> +
> +        node.setAttribute('id', this.id);

Nit: node.id = this.id;

@@ +1379,5 @@
> +
> +        node.setAttribute('id', this.id);
> +        node.setAttribute('class', 'toolbarbutton-1 social-status-button');
> +        node.setAttribute('type', "badged");
> +        node.style.listStyleImage = "url(" + provider.iconURL + ")";

OOI, why the switch to listStyleImage from the image attribute?

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #9)

> ::: browser/base/content/browser-social.js
> @@ -59,5 @@
> >  
> >      gBrowser.addEventListener("ActivateSocialFeature", this._activationEventHandler.bind(this), true, true);
> > -    window.addEventListener("aftercustomization", function() {
> > -      if (SocialUI.enabled)
> > -        SocialMarks.populateContextMenu(SocialMarks);
> 
> Out of curiosity, why is this no longer necessary?

It's working perfectly fine via CustomizationUI without that, I cannot remember the specific reason that was necessary before.

> @@ +1290,5 @@
> >    this._type = type;
> >  }
> >  
> >  ToolbarHelper.prototype = {
> >    idFromOrgin: function(origin) {
> 
> Can we add the missing i to this function name or would that break outside
> stuff?

sigh.  yes.

> @@ +1379,5 @@
> > +
> > +        node.setAttribute('id', this.id);
> > +        node.setAttribute('class', 'toolbarbutton-1 social-status-button');
> > +        node.setAttribute('type', "badged");
> > +        node.style.listStyleImage = "url(" + provider.iconURL + ")";
> 
> OOI, why the switch to listStyleImage from the image attribute?

I had switched away from it before, but the consensus seems to be that we should only use listStyleImage.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: use widgets for status and mark buttons

https://tbpl.mozilla.org/?tree=Try&rev=f333b04a8b0b

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8335659 [details] [diff] [review]
use widgets for status and mark buttons

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

Thanks Gijs,
  I glossed over the customizable UI stuff, but the social specific stuff looks fine to me.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I'm reasonably certain this bug encompasses bug 804235, so I marked it as a dupe. Please re-open that one if I'm wrong.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: use widgets for status and mark buttons

small modification to patch on top of bug 941648 that just landed.

https://hg.mozilla.org/integration/fx-team/rev/9ba2a9dc8abb

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for having broken tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=30977484&tree=Fx-Team 

 https://hg.mozilla.org/integration/fx-team/rev/927c19bd04ab

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: use widgets for status and mark buttons

relanded with test from bug 936712 fixed

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

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/dd40ffa484b9

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

The status and mark buttons are working with Australis on latest Aurora. They can now be repositioned/customized.

Marking this issue as verified fixed.

Build ID: 20140310004003
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0	
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:29.0) Gecko/20100101 Firefox/29.0