960991 - fixups to support standard web share/like endpoints

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The social panels try hard to figure out what size to display for the given content, however that is not always possible.  Allow the manifest to define the display size so we can bypass that discovery and get appropriately sized panels with no effort.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: panel sizes via manifest

Here's an example manifest using the size for the share panel.

{
    "name": "GMail",
    "iconURL": "https://mail.google.com/favicon.ico",
    "origin": "https://mail.google.com",
    "shareURL": "https://mail.google.com/mail/?view=cm&ui=2&tf=0&fs=1&body=%{body}&su=%{title}",
    "pageSize": {
      "social-share-panel": [530, 500]
    }
  }

Comment 2

[Shane Caraveo (:mixedpuppy)]

Spoke with Chad about this issue.  The decision at this point is to make it technically possible, and we will decide if we actually produce our own manifests later.

Comment 3

[Mark Banner (:standard8)]

Would this bug fix the fact that we get scrollbars on panel content? The height is calculated a few pixes too short typically.

Also the current panels are limited to be within the height of the browser (although it is a once per session value), how would this interact with the manifest values?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Banner (:standard8) from comment #3)
> Would this bug fix the fact that we get scrollbars on panel content? The
> height is calculated a few pixes too short typically.

you should be able to solve that by having zero margin on your page, but otherwise, yes, you could define a size for your panel, and that would 

> Also the current panels are limited to be within the height of the browser
> (although it is a once per session value), how would this interact with the
> manifest values?

we don't limit to browser size, there is a hard coded minimum size.  if you used the manifest to define the size, your panel would always be that size (unless moved into the menu panel)

Comment 5

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> but
> otherwise, yes, you could define a size for your panel, and that would 

IIUC, this bug is purely about allowing us to enable providers with manifests/code that we supply, and thus enable non-social-aware pages to have a story for setting a panel size.  To put it another way, if there were no plans for us to handle such "external" providers, this would be WONTFIX.

So I'd be inclined to suggest that social-aware content should not need to use this, and if they do, it implies something else is wrong with our panel resize code.

Comment 6

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

Comment on attachment 8361635 [details] [diff] [review]
panel sizes via manifest

Cancelling review request for now - let's revisit this when necessary.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #6)
> Comment on attachment 8361635 [details] [diff] [review]
> panel sizes via manifest
> 
> Cancelling review request for now - let's revisit this when necessary.

Were you waiting on something to review?

Comment 8

[Shane Caraveo (:mixedpuppy)]

updating this bug to cover:

- defining panel size in manifest
- handle overflow in list of share icons in share panel
- set window.opener for share pages that depend on that to determine if they are opened as a dialog rather than full website

These updates provide support for a wide variety of share endpoints that can be supported without vendor interaction.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: share endpoints patch

Comment 10

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

Comment on attachment 8411498 [details] [diff] [review]
share endpoints patch

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

See comment 5 - I'm not that comfortable with this additional complexity without a clear rationale - and for this iteration of the patch I don't understand the need for the changes in the panel's min-size, nor the browser.xul change.  Finally, this change will add a new dependency on social providers meaning the code can't be reused for loop, which is something I'd really like to avoid.

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #10)
> Comment on attachment 8411498 [details] [diff] [review]
> share endpoints patch
> 
> Review of attachment 8411498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comment 5 - I'm not that comfortable with this additional complexity
> without a clear rationale - 

It's to support standard share endpoints without requiring changes on their part.  I've already passed this by Chad/Gavin/Madhava.

> and for this iteration of the patch I don't
> understand the need for the changes in the panel's min-size, nor the
> browser.xul change.  

There are no panels that are that small, so I was increasing it a bit, it's not important though.  The browser.xul change makes the provider list scrollable if there are more than fits in the panel.  With the number of potential share providers now, this is a minimum to deal with a long list.

> Finally, this change will add a new dependency on
> social providers meaning the code can't be reused for loop, which is
> something I'd really like to avoid.

I'm assuming you're talking about fetching the size from the manifest.  That is not a requirement, it wouldn't affect Loop.

Comment 12

[Shane Caraveo (:mixedpuppy)]

@markh what do you feel needs to happen to move this forward?

Comment 13

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

There are really 2 main things I'm not keen on:

* Can we add a new social-specific helper, as a kind of wrapper around sizeSocialPanelToContent - something that handles the fixed sizes, and if they don't exist, calls sizeSocialPanelToContent.  That way we (a) don't add a dependency on a social provider and (b) keep the code a little saner so we don't have one monster method handling both the dynamic and static cases?

* pageSizes seems to use the XUL panel IDs as keys - that doesn't make much sense to me - can we just dictate some simple strings which are independent of the implemention?

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: share endpoints patch

rather than a wrapper, it seemed easier to handle this where necessary.  existing resize tests pass locally.

Comment 15

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

Comment on attachment 8417744 [details] [diff] [review]
share endpoints patch

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

Looks much better, thanks.  Only f+ as I'm not clear on the Social.jsm changes.  The window.opener change is also a red-flag that I don't feel qualified to make a determination on.

::: browser/base/content/browser-social.js
@@ +691,5 @@
>          iframe.docShell.isAppTab = true;
> +        // to support standard share endpoints mimick window.open by setting
> +        // window.opener, some share endpoints rely on w.opener to know they
> +        // should close the window when done.
> +        iframe.contentWindow.opener = gBrowser.selectedBrowser.contentWindow;

I'm a little uncomfortable with this change, mainly as I don't understand the security implications.  Eg, if it means the social frame could determine the URL of the window currently open in the browser I could forsee a problem.  Do things work OK if we totally fake window.opener to be an object instead of null?

I think this needs some kind of sec-review - not necessarily a formal one, but an r+ from someone who does understand any implications here.

::: browser/modules/Social.jsm
@@ +413,5 @@
>      let computedWidth = parseInt(cs.marginLeft) + body.offsetWidth + parseInt(cs.marginRight);
>      width = Math.max(computedWidth, width);
>    }
> +  // add extra space the panel needs if any
> +  width += panel.boxObject.width - iframe.boxObject.width;

I don't understand why these changes are necessary for existing providers that don't use a fixed size.  I'm also not clear why boxObject is used instead of computed style?

@@ +472,5 @@
>          if (!p) {
>            // preserve non-template query vars
>            query[name] = value;
>          } else if (pageData[p[1]]) {
> +          if (p[1] == "previews")

is this supposed to be in this patch?  It doesn't seem related to the sizing code.

::: toolkit/components/social/SocialService.jsm
@@ +787,5 @@
>    },
>  
> +  getPageSize: function(name) {
> +    let manifest = this.manifest;
> +    if (manifest && manifest.pageSize != undefined)

you can probably remove the '!= undefined' part of this?

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #15)
> Comment on attachment 8417744 [details] [diff] [review]
> share endpoints patch
> 
> Review of attachment 8417744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks much better, thanks.  Only f+ as I'm not clear on the Social.jsm
> changes.  The window.opener change is also a red-flag that I don't feel
> qualified to make a determination on.
> 
> ::: browser/base/content/browser-social.js
> @@ +691,5 @@
> >          iframe.docShell.isAppTab = true;
> > +        // to support standard share endpoints mimick window.open by setting
> > +        // window.opener, some share endpoints rely on w.opener to know they
> > +        // should close the window when done.
> > +        iframe.contentWindow.opener = gBrowser.selectedBrowser.contentWindow;
> 
> I'm a little uncomfortable with this change, mainly as I don't understand
> the security implications.

Yeah, I've been meaning to ping someone in sec about this part (one reason for requesting only f?)

> ::: browser/modules/Social.jsm
> @@ +413,5 @@
> >      let computedWidth = parseInt(cs.marginLeft) + body.offsetWidth + parseInt(cs.marginRight);
> >      width = Math.max(computedWidth, width);
> >    }
> > +  // add extra space the panel needs if any
> > +  width += panel.boxObject.width - iframe.boxObject.width;
> 
> I don't understand why these changes are necessary for existing providers
> that don't use a fixed size.  I'm also not clear why boxObject is used
> instead of computed style?

sizing the iframe will only size the panel to a minimum that fits all elements.  The result is that the arrow change in browser.xul to handle larger lists of share providers will keep the panel size large.  Using panel.sizeTo actually makes the panel size properly, but I need to account for any other children in the panel (thus the boxobject calculations).

> @@ +472,5 @@
> >          if (!p) {
> >            // preserve non-template query vars
> >            query[name] = value;
> >          } else if (pageData[p[1]]) {
> > +          if (p[1] == "previews")
> 
> is this supposed to be in this patch?  It doesn't seem related to the sizing
> code.

It's a fix to the data sent to marks and share.  I changed this bug slightly to address issues necessary to make standard endpoints work, not just the size.

> ::: toolkit/components/social/SocialService.jsm
> @@ +787,5 @@
> >    },
> >  
> > +  getPageSize: function(name) {
> > +    let manifest = this.manifest;
> > +    if (manifest && manifest.pageSize != undefined)
> 
> you can probably remove the '!= undefined' part of this?

yeah.

Comment 17

[Shane Caraveo (:mixedpuppy)]

I'm trying to figure out whether setting window.opener is ok, and could use expert input here.  From digging through nsGlobalWindow.cpp::SetOpener and my test below, it appears to be ok.

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4437

In the patch I'm using (essentially) "panel.iframe.contentWindow.opener = gBrowser.contentWindow;".  From what I can see, this is getting exactly the same security as a dialog opened from any webpage.  I tested using the following code in a test share panel, and in a share dialog opened from youtube (via the webconsole):

  try{dump("window.opener? "); dump(window.opener+"\n");}catch(e){dump(e+"\n");}
  var winObj = [];
  for (var name in window) {
    try{dump("window.opener."+name+"? "); dump(window.opener[name]+"\n");}catch(e){dump(e+"\n");}
  }

The only elements that did not throw security exception in both cases are: close, closed, focus, blur, postMessage, length and opener (ie. w.opener.opener).  If opener is same origin, then you have access to everything.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Setting .opener is generally OK, esp. if you only set it to unprivileged windows.

That said, window.close() on that window might not work right, because setting .opener doesn't set mHadOriginalOpener, right?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Setting .opener is generally OK, esp. if you only set it to unprivileged
> windows.

That's great to know, thanks!

> That said, window.close() on that window might not work right, because
> setting .opener doesn't set mHadOriginalOpener, right?

In this particular use case, I actually wouldn't want it to work, if it did I'd consider the need to figure out how to prevent that.

Comment 20

[Shane Caraveo (:mixedpuppy)]

@markh aside from nits I need to fix, any further issues with my response in comment #16?

Comment 21

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

yeah, sounds fine

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: share endpoints patch

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

Comment 23

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

Comment on attachment 8419466 [details] [diff] [review]
share endpoints patch

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

::: browser/base/content/browser-social.js
@@ +657,5 @@
>      let shareEndpoint = OpenGraphBuilder.generateEndpointURL(provider.shareURL, pageData);
>  
> +    let size = provider.getPageSize("share");
> +    if (size) {
> +      if (this._dynamicResizer) {

is it possible to ever hit this?  ie, it seems getPageSize is effectively static, so if size is true we probably never previously created a dynamicResizer.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8419466 [details] [diff] [review]
share endpoints patch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>+        // to support standard share endpoints mimick window.open by setting
>+        // window.opener, some share endpoints rely on w.opener to know they
>+        // should close the window when done.
>+        iframe.contentWindow.opener = gBrowser.selectedBrowser.contentWindow;

Why are you using selectedBrowser.contentWindow here? If the only purpose is to make this non-null, why not iframe.contentWindow.opener = iframe.contentWindow or some other hack-around (will the opener setter accept a non-window object?).

Comment 25

[Boris Zbarsky [:bzbarsky]]

> will the opener setter accept a non-window object?

No.

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #23)
> Comment on attachment 8419466 [details] [diff] [review]
> share endpoints patch
> 
> Review of attachment 8419466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +657,5 @@
> >      let shareEndpoint = OpenGraphBuilder.generateEndpointURL(provider.shareURL, pageData);
> >  
> > +    let size = provider.getPageSize("share");
> > +    if (size) {
> > +      if (this._dynamicResizer) {
> 
> is it possible to ever hit this?  ie, it seems getPageSize is effectively
> static, so if size is true we probably never previously created a
> dynamicResizer.

It would be hit if you switch from one provider to another in the panel, and the first provider doesn't define size in the manifest.

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: share endpoints patch

Gavins suggestion of using iframe.contentWindow.opener = iframe.contentWindow works fine for this, that change has been made.

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

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/361165c8f203

Comment 29

[Shane Caraveo (:mixedpuppy)]

The demo directory site at http://mixedpuppy.github.io/socialapi-directory/en-US/ has a number of share providers that can be used to test this.  Look for the share panel to have the appropriate size for the content being displayed.  Good test providers from the directory include Facebook, Delicous, Pinterest (if loggedin already), Twitter, Gmail, VKontakte, Tumbler and the Weibo entry towards the bottom of the list (there are two Weibo entries).  Providers should be closing the share panel themselves after sharing, or at least provide a close button (e.g. that is what Twitter does).  Be sure to set the directory pref per the instructions on the directory.

Comment 30

[u279076]

Florin, can you please make sure this gets extensive testing as part of Firefox 32's sign-off?

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Since this is for 32, I'd like to push it until after the work for 31 is done (which should be next week on Tuesday), since we're getting really near to the release and we still have quite some work to do. Is that ok with you Anthony?

Comment 32

[u279076]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #31)
> Since this is for 32, I'd like to push it until after the work for 31 is
> done (which should be next week on Tuesday), since we're getting really near
> to the release and we still have quite some work to do. Is that ok with you
> Anthony?

Yes, that's perfectly reasonable to me.

Comment 33

[Catalin Varga [QA][:VarCat]]

We verified this fix using the following environment:

FF 32 Aurora
Build Id:20140721004001
OS: Mac OS X 10.9.4, Mac Os X 10.7.5

While you are logged in to the share provider the share panel has the appropriate size, but if you are not logged in there are some issues. Those issues and other issues encountered while testing are noted in the following etherpad https://etherpad.mozilla.org/Social-Service-Web-Share

We also tried testing under Windows 7 but we encountered issues while installing more than 1 social service, like some of the social services get removed during the installation of another social service(they are removed completely from about:addons not just disabled), the share panel is not displaying all of the installed social services, while activating one social service you get the activation web page of another social service, due to this issues the testing under windows was not manageable.

Comment 34

[Shane Caraveo (:mixedpuppy)]

(In reply to Catalin Varga [QA][:VarCat] from comment #33)
> We verified this fix using the following environment:
> 
> FF 32 Aurora
> Build Id:20140721004001
> OS: Mac OS X 10.9.4, Mac Os X 10.7.5
> 
> While you are logged in to the share provider the share panel has the
> appropriate size, but if you are not logged in there are some issues. Those
> issues and other issues encountered while testing are noted in the following
> etherpad https://etherpad.mozilla.org/Social-Service-Web-Share

I'll look over these, but note that we are not QA'ing the implementation as much as we are the Firefox side of things.  e.g. Lack of a signout button is not a Firefox bug, the panel being too small to display the content is a manifest bug (we can fix it outside of firefox), and problems switching between providers would be an issue in Firefox.

> We also tried testing under Windows 7 but we encountered issues while
> installing more than 1 social service, like some of the social services get
> removed during the installation of another social service(they are removed
> completely from about:addons not just disabled), the share panel is not
> displaying all of the installed social services, while activating one social
> service you get the activation web page of another social service, due to
> this issues the testing under windows was not manageable.

Did you change the social.directories pref to include the directory site you were testing from?  It sounds like you did not.  The old directory site on github now redirects to https://activations.paas.allizom.org/en-US/ which has a modal dialog explaining that you must change the pref.

Comment 35

[Catalin Varga [QA][:VarCat]]

Thanks for the quick feedback we retested on Windows and Linux, you can find the conclusions on https://etherpad.mozilla.org/Social-Service-Web-Share . 

Also the issue encountered on Windows with the activation of 1 social service that removes another one was indeed due to social.directories pref. If you add a social service using the default value for social.directories pref than the profile will get somewhat corrupted, as the issue will be reproducible even if you change the pref to the correct value (http://mixedpuppy.github.io) afterwards and perform a restart. Do you consider this an issue that deserves a fix ?

Comment 36

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Catalin Varga [QA][:VarCat] from comment #35)
> Thanks for the quick feedback we retested on Windows and Linux, you can find
> the conclusions on https://etherpad.mozilla.org/Social-Service-Web-Share . 
> 
> Also the issue encountered on Windows with the activation of 1 social
> service that removes another one was indeed due to social.directories pref.
> If you add a social service using the default value for social.directories
> pref than the profile will get somewhat corrupted, as the issue will be
> reproducible even if you change the pref to the correct value
> (http://mixedpuppy.github.io) afterwards and perform a restart. Do you
> consider this an issue that deserves a fix ?

Any updates on this Shane?

Comment 37

[Shane Caraveo (:mixedpuppy)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #36)
> (In reply to Catalin Varga [QA][:VarCat] from comment #35)
> > Thanks for the quick feedback we retested on Windows and Linux, you can find
> > the conclusions on https://etherpad.mozilla.org/Social-Service-Web-Share . 
> > 
> > Also the issue encountered on Windows with the activation of 1 social
> > service that removes another one was indeed due to social.directories pref.
> > If you add a social service using the default value for social.directories
> > pref than the profile will get somewhat corrupted, as the issue will be
> > reproducible even if you change the pref to the correct value
> > (http://mixedpuppy.github.io) afterwards and perform a restart. Do you
> > consider this an issue that deserves a fix ?
> 
> Any updates on this Shane?

It's only a problem for people who use alternate directories (ie. those of us who test things).  I improved the test directories with a dialog (automatically shown) on the main page warning about the pref.  IMO that is enough to address this issue.

Comment 38

[Catalin Varga [QA][:VarCat]]

Attachment: printscreen

I've retested this bug on 
FF 32.0b5 
Build Id:20140807212602
and I've noticed on Win 7 and Mac Os X 10.7.5 that the bottom part of  the share panel is cut when Delicious is not logged in. Attached you can see the screenshot.

Comment 39

[Shane Caraveo (:mixedpuppy)]

(In reply to Catalin Varga [QA][:VarCat] from comment #38)
> and I've noticed on Win 7 and Mac Os X 10.7.5 that the bottom part of  the
> share panel is cut when Delicious is not logged in. Attached you can see the
> screenshot.

I've created bug 1051890.

Comment 40

[Catalin Varga [QA][:VarCat]]

Marking the original bug as verified on beta FF 32.0b5.