Closed Bug 960991 Opened 9 years ago Closed 8 years ago

fixups to support standard web share/like endpoints

Categories

(Firefox Graveyard :: SocialAPI, defect)

26 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox32 verified)

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch panel sizes via manifest (obsolete) — Splinter Review
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]
    }
  }
Assignee: nobody → mixedpuppy
Attachment #8361635 - Flags: feedback?(mhammond)
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.
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?
Blocks: Talkilla
(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)
(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 on attachment 8361635 [details] [diff] [review]
panel sizes via manifest

Cancelling review request for now - let's revisit this when necessary.
Attachment #8361635 - Flags: feedback?(mhammond)
(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?
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.
Summary: allow panel size definition in manifest → fixups to support standard web share/like endpoints
Attached patch share endpoints patch (obsolete) — Splinter Review
Attachment #8361635 - Attachment is obsolete: true
Attachment #8411498 - Flags: feedback?(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.
Attachment #8411498 - Flags: feedback?(mhammond) → feedback-
(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.
@markh what do you feel needs to happen to move this forward?
Flags: needinfo?(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?
Flags: needinfo?(mhammond)
Attached patch share endpoints patch (obsolete) — Splinter Review
rather than a wrapper, it seemed easier to handle this where necessary.  existing resize tests pass locally.
Attachment #8417744 - Flags: feedback?(mhammond)
Attachment #8411498 - Attachment is obsolete: true
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?
Attachment #8417744 - Flags: feedback?(mhammond) → feedback+
(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.
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.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(bugs)
@markh aside from nits I need to fix, any further issues with my response in comment #16?
Flags: needinfo?(mhammond)
yeah, sounds fine
Flags: needinfo?(mhammond)
Attached patch share endpoints patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e228010f6ba8
Attachment #8417744 - Attachment is obsolete: true
Attachment #8419466 - Flags: review?(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.
Attachment #8419466 - Flags: review?(mhammond) → review+
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?).
> will the opener setter accept a non-window object?

No.
(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.
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
Attachment #8419466 - Attachment is obsolete: true
Attachment #8421877 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/361165c8f203
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Blocks: 1014332
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.
Keywords: verifyme
Florin, can you please make sure this gets extensive testing as part of Firefox 32's sign-off?
Flags: needinfo?(florin.mezei)
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?
Flags: needinfo?(anthony.s.hughes)
(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.
Flags: needinfo?(anthony.s.hughes)
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.
(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.
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 ?
Flags: needinfo?(florin.mezei)
(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?
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
Attached image 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.
(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.
Marking the original bug as verified on beta FF 32.0b5.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.