896314 - Social permissions UI doesn't reset on provider change.

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect, P1)

Description

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

STR:
* Activate a provider that requests some permission that displays a UI - eg, the demo Twitter provider request geo-location.
* Note the permissions button in the toolbar and the panel - do NOT click on the panel.
* Change to a different provider.

Actual:
* Permissions button still exists in the UI.  Click on the panel and it still says "This website (www.provider1.com) ..." - but provider2.com is now active - no provider1 UI remains visible.

Expected:
* The permissions UI should not imply the incorrect provider is requesting those permissions.  In the short-term, this means the button should just be removed from the toolbar until the original provider is re-selected.

Marking as a high priority as it may mislead the user into granting a permission to a site they did not intend to (eg, they see the facebook icons, then click on that permissions icon without reading the panel message carefully, and decide they do want to grant the permission to Facebook - but they actually granted it to Twitter.)

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions fix when changing provider sidebar

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

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 781187 [details] [diff] [review]
permissions fix when changing provider sidebar

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

::: toolkit/modules/PopupNotifications.jsm
@@ +346,5 @@
>  
> +    if (aBrowser.docShell.isActive) {
> +      // get the anchor element if the browser has defined one so it will
> +      // properly update in the call to _update.
> +      let anchorElement = notifications.length > 0 ? notifications[0].anchorElement : null;

So let's skip the extra lookup below if this is not null

@@ +357,5 @@
> +          anchorElement = this.iconBox.ownerDocument.getElementById(anchor);
> +        }
> +      }
> +      this._update(notifications, anchorElement);
> +    }

if you wanna go the extra mile and refactor this code and the getter, i'm imagining it'll be better in case the way this anchor works changes in the future. up to you. I'll re-review quickly if you do that

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions fix when changing provider sidebar

updated per irc discussion w/felipe

Comment 4

[Shane Caraveo (:mixedpuppy)]

try on updated patch:
https://tbpl.mozilla.org/?tree=Try&rev=d27c6e67b78d

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 782300 [details] [diff] [review]
permissions fix when changing provider sidebar

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

::: toolkit/modules/PopupNotifications.jsm
@@ +29,5 @@
> +      return anchor;
> +    } else {
> +      return aBrowser.ownerDocument.getElementById(anchor);
> +    }
> +  }

just add an explicit return null here at the end

Comment 6

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

Comment on attachment 782300 [details] [diff] [review]
permissions fix when changing provider sidebar

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

>       // Make sure the right sidebar URL is loaded
>       if (sbrowser.getAttribute("src") != Social.provider.sidebarURL) {
>         sbrowser.setAttribute("src", Social.provider.sidebarURL);
>+        PopupNotifications.locationChange(sbrowser);
>         sbrowser.addEventListener("load", SocialSidebar._loadListener, true);

Hrm, wouldn't it be a bit closer to the existing locationChange contract if you called this from _loadListener? I guess that might be "too late" in some cases.

>diff --git a/toolkit/modules/PopupNotifications.jsm b/toolkit/modules/PopupNotifications.jsm

>+function getAnchorFromBrowser(aBrowser) {
>+  let anchor = aBrowser.getAttribute("popupnotificationanchor") ||
>+                aBrowser.popupnotificationanchor;
>+  if (anchor) {
>+    if (anchor instanceof Ci.nsIDOMXULElement) {
>+      return anchor;
>+    } else {

nit: get rid of unneeded else-after-return

>-    if (aBrowser.docShell.isActive)
>-      this._update(notifications);
>+    if (aBrowser.docShell.isActive) {
>+      // get the anchor element if the browser has defined one so it will
>+      // properly update in the call to _update.
>+      let anchorElement = notifications.length > 0 ? notifications[0].anchorElement : null;
>+      if (!anchorElement)
>+        anchorElement = getAnchorFromBrowser(aBrowser);
>+      this._update(notifications, anchorElement);
>+    }

Why is this change needed? "properly update" could use some elaboration. _update already uses the first notification's anchor if no anchor is passed in, so is this meant to impact its "useIconBox" check?

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> Comment on attachment 782300 [details] [diff] [review]
> permissions fix when changing provider sidebar
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

> >-    if (aBrowser.docShell.isActive)
> >-      this._update(notifications);
> >+    if (aBrowser.docShell.isActive) {
> >+      // get the anchor element if the browser has defined one so it will
> >+      // properly update in the call to _update.
> >+      let anchorElement = notifications.length > 0 ? notifications[0].anchorElement : null;
> >+      if (!anchorElement)
> >+        anchorElement = getAnchorFromBrowser(aBrowser);
> >+      this._update(notifications, anchorElement);
> >+    }
> 
> Why is this change needed? "properly update" could use some elaboration.
> _update already uses the first notification's anchor if no anchor is passed
> in, so is this meant to impact its "useIconBox" check?

yes.  I can update the comment.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: permissions fix when changing provider sidebar

updated from comments

Comment 9

[Shane Caraveo (:mixedpuppy)]

how's about a try that runs tests...
https://tbpl.mozilla.org/?tree=Try&rev=8857e3daeba9

Comment 10

[Shane Caraveo (:mixedpuppy)]

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 782355 [details] [diff] [review]
permissions fix when changing provider sidebar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 809085
User impact if declined: permission icon attached to social toolbar does not change when user changes social provider
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f24192cb2b2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ef5c3efbcab

Not entirely sure about the un-bitrotting of this. Please take a look to make sure it's right.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/7ef5c3efbcab
> 
> Not entirely sure about the un-bitrotting of this. Please take a look to
> make sure it's right.

That looks correct.  The m-c version was done on top of bug 886227, which if we wanted to get rid of intermittent oranges on aurora, could also be uplifted.

Comment 15

[Ioana (away)]

Verified as fixed on Firefox 25 beta 4 - 20131001024718 - on Windows 7 64bit, Mac OS X 10.7.5 and Ubuntu 13.04 32bit.