765874 - Implement v1 of recommend/like share button

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

This bug is to implement the recommend/share button/component for the Social API.

See https://wiki.mozilla.org/Labs/SocialAPI#The_.22Recommend.2FShare.22_command for more information.

Comment 1

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

Attachment: WIP of patch

This WIP patch adds a button to the location bar but it does not do anything yet.

Still to do:
+ Connect to the provider/frameworker patches
+ Add ability to hide/show the feature

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Recommend/share button

The mockup attached shows how a recommend/share button would appear in the Firefox URL bar.  Its behavior is consistent with the similar functionality of Firefox’s star button.  Like the star button, the recommend/share button is activated by a single click, minimizing the time a user needs to invest in sharing.  If the user wishes to unshare a page or to comment on the page they’ve shared, they can click on the button a second time.

Comment 3

[u428464]

I don't really get the reason behind the button in the location. AFAIK one of Australis goal was to clean the location bar and avoid visual noise in this area (bookmark star merge outside the location bar, suppression of the dropdown arrow, etc.), so why put the recommend/like button in the location bar ?

Comment 4

[bogas04]

(In reply to Guillaume C. [:ge3k0s] from comment #3)
> I don't really get the reason behind the button in the location. AFAIK one
> of Australis goal was to clean the location bar and avoid visual noise in
> this area (bookmark star merge outside the location bar, suppression of the
> dropdown arrow, etc.), so why put the recommend/like button in the location
> bar ?

Success of +1 / like / recommend / tweet / reddit all over articles on web , and use of social sites for just about everything. With this feature , you may not , but most of the Young internet audience would be benefited by it and "like" it. Further it makes Firefox on desktop more like Firefox on Android and on other devices. Safari and IE are including this feature in the next operating systems they are coming to. It sure has some importance that Apple and MS would like to include it :)

Comment 5

[u428464]

I know and I totally agree, but if you have read correctly what I've written my comment was about the button located in the location bar. Why here and not in a toolbar ?

Comment 6

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

The bookmark star is not a great comparison because there are other factors in play there. We currently have a bookmark star in the location bar and a bookmark sidebar button outside of the location bar.

Moving the bookmark star outside of the location bar will allow us to combine both of those elements. The same cannot be said for this feature.

Comment 7

[u428464]

Thanks for the answer, but the bookmark star was just an example amongst other. The location bar should stay the more minimalistic possible in term of UI.

Comment 8

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

Attachment: WIP v.2 patch for bug

This patch allows the button to be clicked and remembers its state if the tab is switched and reverts on page navigation.

This is still using a generic icon and generic tooltip since the social APIs haven't landed yet.

IRC Conversation:
> 4:28 PM <jaws> Boriss: if someone shares a page, then clicks again on the button to add a comment, they will most likely end up with two shares
> 4:28 PM <jaws> i don't think that's what the user would want
> 4:29 PM <Boriss> jaws: the intention of the design is not that it would end up with two shares
> 4:29 PM <jaws> is it possible for us to retroactively update a share?
> 4:30 PM <Boriss> to un-share and to comment?   how technically hard that is i don't know, but that's what the design in the mockup conveys
> 4:30 PM <michaelhanson> jaws | Boriss: I don't think we've come up with messages to convey that idea, but it is a good one.  wouldn't be hard to add to the messaging API
> 4:31 PM <jaws> michaelhanson, Boriss: for now i'll just concentrate on sharing without a message, since i don't know how we would pull it off today without generating two shares
> 4:31 PM <michaelhanson> ok.

Based on the conversation, I'm only planning on implementing the first part of Boriss' mockup in this bug. Let's push the recommend/share annotations to a followup bug when we have a deeper API.

Comment 9

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

Comment on attachment 635586 [details] [diff] [review]
WIP v.2 patch for bug

Unsolicited review :)

> <command id="cmd_socialRecommend" oncommand=""/>

It looks like it might be better named as "Social:Recommend" to better match the existing style?

+    // todo: Mark the page as recommended if it was previously
+    //       recommended (switching tabs, loaded).

It looks like that comment is now stale?

\ No newline at end of file

Add a newline?

+      // todo: Perform the share and pass the callback function
+      //       to the SocialService to be executed upon success.
+      recommendCallback();

recommendCallback doesn't seem to be defined - should that line just be removed and let the todo comment stand in its place?

+        aBrowser._locationRecommended = false;

I wonder if the state would be better managed as a global in the module - eg, if the user selects "Back" to a page they have already recommended, it seems the state would be lost?  Or maybe even managed via session history?

+<!ENTITY recommendButton.tooltip      "Recommend this page">

I realize this (and the image) are just place-holders, but might it be better to just hard-code the placeholder string in the XUL so browser.dtd doesn't need to change once now and then again when we remove the string?

+/* social recommend button */
+
+#social-recommend-button {
+  list-style-image: url("chrome://browser/skin/recommend.png");
+}
+
+#social-recommend-button[recommended] {
+  -moz-transform: rotate(45deg);
+}
+

Similarly here - given the image will be set by the provider and therefore managed directly in the code, might it be better to just have the code explicitly modify the style so the .css doesn't need to be reverted once the placeholders are removed?

Comment 10

[Johnathan Nightingale [:johnath]]

(In reply to Jared Wein [:jaws] from comment #8)

> IRC Conversation:
> > 4:28 PM <jaws> Boriss: if someone shares a page, then clicks again on the button to add a comment, they will most likely end up with two shares
> > 4:28 PM <jaws> i don't think that's what the user would want
> > 4:29 PM <Boriss> jaws: the intention of the design is not that it would end up with two shares
> > 4:29 PM <jaws> is it possible for us to retroactively update a share?
> > 4:30 PM <Boriss> to un-share and to comment?   how technically hard that is i don't know, but that's what the design in the mockup conveys
> > 4:30 PM <michaelhanson> jaws | Boriss: I don't think we've come up with messages to convey that idea, but it is a good one.  wouldn't be hard to add to the messaging API
> > 4:31 PM <jaws> michaelhanson, Boriss: for now i'll just concentrate on sharing without a message, since i don't know how we would pull it off today without generating two shares
> > 4:31 PM <michaelhanson> ok.
> 
> Based on the conversation, I'm only planning on implementing the first part
> of Boriss' mockup in this bug. Let's push the recommend/share annotations to
> a followup bug when we have a deeper API.


<peanut-gallery>
Could the first button click share on, say, a 5s delay? I suspect most users who discover the click-again-to-comment action will do so relatively quickly, at which point we can cancel the original.
</peanut-gallery>

Maybe that strategy is too error-prone and we're better to leave it off the table; just trying to get creative.

Comment 11

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

(In reply to Johnathan Nightingale [:johnath] from comment #10)
> (In reply to Jared Wein [:jaws] from comment #8) 
> > Based on the conversation, I'm only planning on implementing the first part
> > of Boriss' mockup in this bug. Let's push the recommend/share annotations to
> > a followup bug when we have a deeper API.
> 
> 
> <peanut-gallery>
> Could the first button click share on, say, a 5s delay? I suspect most users
> who discover the click-again-to-comment action will do so relatively
> quickly, at which point we can cancel the original.
> </peanut-gallery>
> 
> Maybe that strategy is too error-prone and we're better to leave it off the
> table; just trying to get creative.

Another option would be to show the doorhanger by default and not to send the message until the doorhanger is dismissed (via adding an annotation then hitting submit, hitting submit, or just clicking outside of it).

This could also show a cancel button which would stop the submission from occurring.

Comment 12

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

The other side of this equation is the API used by providers.  The current API allows the provider to change the tooltip text and the toolbar icon after a share, so, in theory, the fact a page has been shared should be fairly obvious from the UI.  IF the user clicks on it again, the provider again can change the text and icon, so can do an "unshare" and reflect that new state in the UI.

The other consideration is that the API does not offer a way to get content to display in a doorhanger.  So with the current API, a doorhanger could really have *just* a submit button and nothing else.

Obviously this could change in V2, or we could even work with our partner to change it for V1, but we are somewhat constrained by that.

Comment 13

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

In case it wasn't clear to others (as it wasn't clear to me), the first version of this button will just be a binary share/unshare feature.

> <jaws> markh: based on your comment in bug 765874, should i just make the doorhanger have a "Remove/undo" button and nothing else? this is different than Boriss's mockup but obviously much easier to implement for a v1
> <Boriss> jaws: in the first iteration of this feature, it's only binary - one click on (share), one click off (unshare)
> <Boriss> what's in that mockup is for v2

Comment 14

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #11)
> Another option would be to show the doorhanger by default and not to send
> the message until the doorhanger is dismissed (via adding an annotation then
> hitting submit, hitting submit, or just clicking outside of it).

doorhanger is useful for notifying non-modal info.
However, doorhanger has a role to ask to user about permissions in a page. I seem that doorhanger is not good for share button.

Comment 15

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

(In reply to Jared Wein [:jaws] from comment #11)
> Another option would be to show the doorhanger by default

I meant to say "arrow panel". Sorry for the confusion.

Comment 16

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #15)
> (In reply to Jared Wein [:jaws] from comment #11)
> > Another option would be to show the doorhanger by default
> 
> I meant to say "arrow panel". Sorry for the confusion.

All right.

I think it is good that show the arrow panel by default.
But I don't agree that not to send the message until the arrow panel is dismissed. If a message is sent on dismissed the arrow panel, user may mistake accidentally sending a message. So it should send a message only if user clicks the "send" button.

Comment 17

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

Attachment: WIP v.3 patch for bug

This is an updated WIP patch for the bug. This does about as much as is possible until the backend parts are finished.

TODOs are located in browser-social.js. We might remove the ability to revoke a share/recommend for v1 based on API limitations (not sure here though).

Comment 18

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

Attachment: WIP v.4 patch for bug

Comment 19

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

Comment on attachment 636912 [details] [diff] [review]
WIP v.4 patch for bug

* The 'social-recommend-button' styling in browser.css and the recommendButton.tooltip string are both still in the patch but seem unused.  I guess that's just an oversight?

* The '#share-button' part of the CSS should probably be removed given the images will be provided by the provider.  Given we allow them to provide a completely different icon after it is shared, the 'outline' style probably isn't appropriate either.

* Similarly for '#socialUserAvatar' - that probably shouldn't be in the CSS as the image will be supplied by the provider.

* The 'unshare' function:
** gBrowser.selectedBrowser._locationShared = true; - should that be setting to false?
** // TODO: notify social service that share was revoked. - there is no such API at the moment, so I doubt this could be implemented this way for V1.  Obviously we could talk with our partners about this, but that conversation would need to happen before we can commit to this UI for v1.

* I don't understand the logic in 'onClick: function SRB_onClick'.  I read it as:
** if we haven't shared this location, just set _locationShared and add a 'shared' attribute to the button - but don't tell the provider about it.
** If we have shared this location, then display the popup, telling the provider once one of the buttons in the popup has been hit.
** There doesn't seem to be any logic for hiding either of the buttons in the panel - thus, whenever the panel is shown both the editSharePopupOkButton and editSharePopupUndoButton are shown.

This implies to me that (a) the user needs to click on the share button twice to see the panel, where the first one doesn't actually take any action - and that the panel doesn't reflect whether the user has or has-not already shared.

* Keeping the shared state in _locationShared concerns me as, IIUC, if the user shares a page, navigates somewhere else, then hits "back" (or otherwise ends up back at the first page) we have lost the fact they shared that page.

* The TODO section might be misleading:
** The stack might be tricky to do given the image comes from the provider.
** There is currently no API to notify the service that the share was revoked.  All we have at the moment is the ability to tell them the button was hit - the provider then gets to choose what to do (eg, the provider can determine if the page was already shared, unshare it, then provide an icon and tooltip text which reflect the action it took.

Comment 21

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Mark Hammond (:markh) from comment #19)
> Comment on attachment 636912 [details] [diff] [review]
> * The 'unshare' function:
> ** gBrowser.selectedBrowser._locationShared = true; - should that be setting
> to false?
> ** // TODO: notify social service that share was revoked. - there is no such
> API at the moment, so I doubt this could be implemented this way for V1. 
> Obviously we could talk with our partners about this, but that conversation
> would need to happen before we can commit to this UI for v1.

We can't ship a recommend/share button without the ability to undo that action accessible on the button itself.  This would create a very poor user experience - one misclick on any website and a user would be stuck with that action with that provider, likely viewable by others.  As you said, we should be having this conversation as soon as possible.

Comment 22

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

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #21)

> We can't ship a recommend/share button without the ability to undo that
> action accessible on the button itself.  This would create a very poor user
> experience - one misclick on any website and a user would be stuck with that
> action with that provider, likely viewable by others.  As you said, we
> should be having this conversation as soon as possible.

To clarify, the current scheme does allow for the provider to "unlike" on the second click and to update the icon and tooltip text accordingly.

I agree the UI for this is poor and it might not be obvious to the user that a second click will undo, but I'm just clarifying the situation.  The flow with the current API looks (roughly) like:

* We ask provider for tooltip text and icon - they send, eg, a "thumbs-up" icon and "Like on Amigo"
* User clicks - we tell provider a click was done.  They do the like.
* We ask provider for text and icon again - they send an icon reflecting it has been liked and "You liked this on Amigo".
* User clicks - we tell provider a click was done (same notification as above).
* They notice a like was already done, so they "undo" the action.
* We ask provider for text and icon again - they send the same icon and tooltip text as step 1, indicating the next click will again perform a "like".

The key points are:
* There is currently exactly one notification about a user click on the recommend button.
* After every click, we get a *new* icon and tooltip text which should reflect the current state.
* The provider themselves are responsible for tracking the current "like" state and deciding what action to take on the click.

Comment 23

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

(In reply to Mark Hammond (:markh) from comment #22)
> * We ask provider for tooltip text and icon - they send, eg, a "thumbs-up"
> icon and "Like on Amigo"
> * User clicks - we tell provider a click was done.  They do the like.
> * We ask provider for text and icon again - they send an icon reflecting it
> has been liked and "You liked this on Amigo".

This is going to look really janky from the user's perspective since it will take too long to get the response back and update the state of the icon. We should get these icons and strings in advance and just do a quick swap on the front-end side of things.

> The key points are:
> * There is currently exactly one notification about a user click on the
> recommend button.
> * After every click, we get a *new* icon and tooltip text which should
> reflect the current state.

Again, this will be unnecessary network traffic and slowness that can be solved by caching these assets upfront (when the provider is set) and only sending a very short request (location, title) with very short response (such as HTTP status code).

Firefox should be in control here and make sure that we as a product are able to provide users an undo option. This undo option should be a two-click step since that is in line with our current undo for bookmarks.

I don't think we can trust that all providers will treat a second 'share/recommend' as an undo.

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #23)

> Firefox should be in control here and make sure that we as a product are
> able to provide users an undo option. This undo option should be a two-click
> step since that is in line with our current undo for bookmarks.
> 
> I don't think we can trust that all providers will treat a second
> 'share/recommend' as an undo.

not arguing one way or another, I just want to point out that undo will entirely rely on the provider cooperating with that.  The only way we can control it is to delay-send the initial click and have an undo capability prior to the provider ever getting an initial recommend signal.

Personally I prefer the ui in the last share/web activities module I did, drop panel with provider content for sharing.  It does leak the url to the provider soon as the button is clicked, but the user then has to "officially" share it in the providers UI.

Comment 25

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

Attachment: WIP v.5 patch for bug

Thanks for the feedback Mark.

I went over the UX here with Boriss and she was happy with the interactions here.

This is still using a property off of a browser to store the current shared/not-shared bit. Unless we get this hooked in to some persistent store (bookmarks is likely the best), then we will need to do something like this. Back-forward navigation will not reapply the state, but at the same time users should have the ability to like pages multiple times over the course of using the browser (e.g. once in October, once again in July, etc).

This patch is not hooked up to a social service yet as that has yet to be implemented. To fit the current interaction model, we will need these APIs:
1) Share/recommend a page
2) Undo share/recommend
3) Address of a user's profile/account page
4) User & provider information of:
--> Share tooltip text ("Bacon this")
--> Shared tooltip text ("You bacon this")
--> Share button graphic (require it to be 16x16)
--> User avatar (require it to be 48x48)
--> Logged in text (e.g., "Jared 'JAWS' Wein")
--> Undo text ("Un-bacon")
--> Undo accesskey ("U")
--> OK label ("OK")
--> OK accesskey ("O")

I am also planning on writing a couple unit tests here to test the observation of the preference and the workings of the urlbar buttons with the associated panel.

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #25)
> Created attachment 637319 [details] [diff] [review]
> WIP v.5 patch for bug

> To fit the current interaction model, we will need these APIs:
> 1) Share/recommend a page
> 2) Undo share/recommend
> 3) Address of a user's profile/account page

This is available (in xulmenu branch) and is tracked in bug 757747

> 4) User & provider information of:
> --> User avatar (require it to be 48x48)

We have 32x32 for the toolbarbutton.  We can ask for multiple sizes to be sent.  also bug 757747

> --> Logged in text (e.g., "Jared 'JAWS' Wein")

Do you want DisplayName or Username?  e.g. I have "Shane Caraveo" for multiple email accounts that are used for google accounts.

Comment 27

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

Thanks. We should be using DisplayName here.

Comment 28

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

Attachment: Automated tests for recommend/share button

This tests the pref and panel functions.

Comment 29

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

Comment on attachment 637319 [details] [diff] [review]
WIP v.5 patch for bug

lookin' good!

Comment 30

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

Comment on attachment 637319 [details] [diff] [review]
WIP v.5 patch for bug

With the exception of the glue parts between the front-end and the social API services, can you take a look at this patch and let me know if you see any issues?

Comment 31

[Marco Zehe (:MarcoZ)]

What will the keyboard shortcut be?

Comment 32

[Shane Caraveo (:mixedpuppy)]

This may have some affect on your code, making profile data available.

profile data documentation added at for bug 757747 in change https://github.com/mozilla/socialapi-dev/commit/3863547243feaacded7d31532c2bf1eca216ae0c

This is in a git branch, still needs to be merged via bug 763837

Comment 33

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

(In reply to Marco Zehe (:MarcoZ) from comment #31)
> What will the keyboard shortcut be?

I still need to determine which keyboard shortcut is available and makes sense.

Comment 34

[Drew Willcoxon :adw]

Comment on attachment 637319 [details] [diff] [review]
WIP v.5 patch for bug

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

I haven't written any XUL or CSS in a good long while.  With that in mind...  The structure here looks OK to me.

::: browser/base/content/browser-social.js
@@ +1,3 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Unrelated nit: Preprocessor comments mess up line numbers in stack traces for no upside (infinitesimally smaller post-processed source?), so I'd prefer JS // comments.

Also, couldn't this file simply be named social.js instead of browser-social.js?  It's already in browser/.

@@ +66,5 @@
> +    this._provider = aProvider;
> +    // The fallback strings need to be removed, they are only for demo purposes.
> +    this._shareButton.setAttribute("tooltiptext", aProvider["tooltiptext"] || "Bacon this");
> +    this._shareButtonShared.setAttribute("tooltiptext", aProvider["tooltiptext"] || "You bacon this");
> +    this._shareButton.style.listStyleImage = aProvider["icon"] || "url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAAadEVYdFNvZnR3YXJlAFBhaW50Lk5FVCB2My41LjEwMPRyoQAAAiBJREFUOE+Fkk1rGlEUhgWzqJB9foY7peCi6xARcdGFKGRRlC6MkFhEEYxNrUSxkVI1iAou/IQKutKNRVASqhi/UIyYqCQqSLSrQgi8nTOQEJORDjxc7j3vfWbOvcPjvXicTucuw+F/2H2572l+fHz8eblc4u7ujhOqUWatwOFwfJnP55hOp5xQjTJrBXa73TGbzXB7e8sJ1SizVmCz2Zy0eTQacUI1yqwVWK3Wb+PxGIPB4BXD4RCXl5egjEajecMpsVgs36+urtDr9Vbo9/vodrvIZrMIh8M/mdwnToHZbPbRW9rt9gq0lk6nUa/XcX9/D8pxCoxGY6DT6aDRaKxAa8lkEjQuFgtQjhVsbGxUCblcriEODg5awWAQgUAAiUQC5XIZ1WoVzWYT0WiUHW9ubmAwGMKsYH9/HwqFAg8PD8hkMri+vmahnovFIvx+PyKRCM7Ozqh3XFxcsKK9vb0oK9jZ2YFEImF/Giq0Wi1UKpUnqJV8Pg+32w2TyYRUKoWjoyNotdrfOp3uI08sFkMoFLJXdnp6ilqthlgsBp/PB4/Hg3g8jlKpxIqZq0MoFML5+fnjPMDb3t6GSCTCZDLB1tbWoUqlajJnMM/lcuynu1wu6hd6vR5erxfM+t+Tk5M2c1a/mOx7Hp/P/0MIBIKvm5ubHxjhO4aoUqlsMv3PC4UCHqG5Wq2uSaXSHzKZ7O3av5EKjyJmrDwjSuvPN/4DWmMg0C/xjlwAAAAASUVORK5CYII=)";

I presume these data URIs will be replaced with image files at some point.

@@ +94,5 @@
> +
> +  unshare: function SRB_unshare() {
> +    this._log("unshare");
> +    // Commented out until API is completed.
> +    //this._provider.unshare(url);

I don't know how the provider will work here, but is it possible that the provider could veto the unshare, or that the unshare could fail for some other reason (like a network error)?  What happens then?  I guess the same question applies to share too.

@@ +103,5 @@
> +  },
> +
> +  updateState: function SRB_updateState() {
> +    this._log("updateState");
> +    let currentPageShared = gBrowser.selectedBrowser._locationShared;

It doesn't look like _locationShared is persisted anywhere.  If you close a shared page and then reopen it, wouldn't it be incorrectly marked as unshared?  Shouldn't shared pages behave pretty much like bookmarks, with a database?  Or do you connect with the provider every time a page loads to ask if the page was shared?

@@ +123,5 @@
> +  get _shareButton() {
> +    delete this._shareButton;
> +    return this._shareButton =
> +      document.getElementById("share-button");
> +  },

This is relatively unimportant, but instead of defining all these DOM getters by hand, it would be simpler to either define them programmatically or define a single getter that takes an ID and populates a mapping of IDs to DOM elements.

::: browser/base/content/browser.js
@@ +4420,5 @@
>        if (aRequest) {
>          // Initialize the click-to-play state.
>          aBrowser._clickToPlayDoorhangerShown = false;
>          aBrowser._clickToPlayPluginsActivated = false;
> +        aBrowser._locationShared = false;

It would be better to keep share-related data isolated to the ShareButton (or whatever larger share UI we end up with).  Could you keep track of per-browser data within ShareButton instead of hanging it off each browser, e.g., by mapping browser outer window IDs to data?  I know it'll probably be more work.

Comment 35

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

Attachment: WIP v.6 patch

This patch combines the two previous patches and incorporates some of the changes that Drew requested. I'm using ctrl+shift+l as the keyboard shortcut to recommend/share a page.

I've updated the mochitest to exercise the keyboard shortcuts and keyboard navigation.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=520034488d91

Comment 36

[Marco Zehe (:MarcoZ)]

Comment on attachment 638475 [details] [diff] [review]
WIP v.6 patch

>+    <panel id="editSharePopup"
>+           type="arrow"
>+           orient="vertical"
>+           ignorekeys="true"
>+           hidden="true"
>+           onpopupshown="ShareButton.panelShown(event);"
>+           consumeoutsideclicks="true"
>+           aria-labelledby=""

Is this meant to actually point to an ID that labels this panel, or did you mean to just wrie this down here so you won't forget about it?

There are two problems I still see with the user interaction.

1. Ctrl+Shift+L pressed. I don't get any response from my screen reader. The share converts to "shared", but this is a purely visual indication that screen readers will not be able to pick up.

Solution: Create a hidden element somewhere that contains the aria-live attribute, and where you can push messages a screen reader should speak. That way, once you push the share, you can send in text that says "Page shared" or so. Screen readers will pick this up and speak it.
We might need to experiment with this, since this is in XUL, and we don't have many instances of ARIA live region usages in XUL yet.

Second: If I press Ctrl+Shift+L again, I'm placed inside the panel on the OK button. The "Undo button" has a label of 'un-baken', the other button has a label "Jared 'jaws' Wein" label, and I'm not sure what this one is supposed to do, and then there's another button preceeding that which has no label.

I assume that this is the user avatar that allows one to go to the profile or so.

So, in parts, this is OK, but needs more work. And since this is neither a denial nor an unconditional plus, I'm simply removing the feedback request.

Comment 37

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

Attachment: WIP v.7 patch

Thanks for the great feedback Marco. Can you test out this version of the patch and let me know if it is better?

Comment 38

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

Attachment: WIP v.8 patch

This patch now uses the SocialService.getProvider API, but shouldn't change much else.

Marco, please provide feedback on the previous patch since this one will now require some prefs to be set to work correctly.

Comment 39

[Marco Zehe (:MarcoZ)]

Comment on attachment 638892 [details] [diff] [review]
WIP v.7 patch

Clever, f=me.

Comment 40

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

Attachment: WIP v.9 patch

This is pretty broken now, as I've moved the setup of the images to click on to the now get set from the worker response.

Comment 41

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

Attachment: Patch

This patch implements the recommend/share button. It should be applied before the patch for bug 764872.

Comment 42

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

Comment on attachment 639943 [details] [diff] [review]
Patch

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

>+Components.utils.import("resource://gre/modules/SocialService.jsm");

I think you should put this in browser.js, just so that it's easy to tell which modules are imported in that scope (the fact that these files are all merged together isn't intuitive).

>+let ShareButton = {

Maybe the global variable name should be "Social" instead of "ShareButton", since we'll certainly want to extend its functionality, and much of the state it holds (provider, enabled state, worker port) will apply to the other UI bits as well.

>+  init: function SRB_init() {

>+    // hack for now while social service is being worked on.
>+    SocialService.getProvider("https://motown-dev.mozillalabs.com", this.setProvider.bind(this));

SocialService.getProviderList(function (providers) {
  this.setProvider(providers[0]);
}.bind(this));

would probably be a more future-proof hack.

>+  dismissPopup: function SRB_dismissPopup() {

dismissSharePopup

>+  handleWorkerResponse: function SRB_handleWorkerResponse(aEvent) {

>+      case "social.user-recommend-response":
>+      case "social.user-unrecommend-response":

>+        // For perceived performance, the UI was already updated for
>+        // the expected case. Revert the UI if there was an error.

Is there an agreed-upon error message?

>+      case "social.ambient-notification-area":

What triggers the firing of this message from the social provider? Can we use it as a way to verify that the frameworker is set up correctly, and only enable the button once we've gotten this response? I guess this name is odd for that purpose, but I think we need some kind of such message.

>+  observe: function SRB_observe(aSubject, aTopic, aData) {

>+    if (aTopic == "nsPref:changed")

You shouldn't need to check the topic if you're only registering it as a pref observer.

>+  panelShown: function SRB_panelShown(aEvent) {

>+    this._sharePopupOkButton.focus();

Hmm, do we really want to be stealing focus like this? Is it even necessary given that the button has autofocus set?

>+  share: function SRB_share() {

>+      let browser = gBrowser.selectedBrowser;
>+      let webNav = browser.webNavigation;
>+      let url = webNav.currentURI;

gBrowser.currentURI. And the URL needs to be sent as text, so you want .spec. The addon code uses specIgnoringRef, but I'm not sure that's really desired. We might also want to clone and clear out userPass? Not sure. (these same comments apply to unshare).

>+  updateState: function SRB_updateState() {

>+    let buttonShowingShared = this._shareButton.hasAttribute("shared");
>+    this._log("currentPageShared: " + currentPageShared + ", buttonShowingShared: " + buttonShowingShared);

You'll want to remove this before landing I assume? Probably want to remove _log entirely actually.

>+  get _shareButton() {
>+  get _shareButtonShared() {
>+  get _shareButtonStack() {

These all look like they need to be refreshed after after BrowserToolboxCustomizeDone, since they refer to elements that are part of the location bar (which can be removed/readded, etc.).

There are a lot of other getters here. You should probably reduce the boilerplate and do something like ["button", "popup"].forEach(defineGetter). For elements that aren't actually frequently accessed, just inline getElementById and avoid getters entirely.

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+#ifndef XP_UNIX

nit: if you have an #else, always use an #ifdef instead of an #ifndef, just easier to read.

>+            <stack id="share-button-stack" hidden="true">

>+              <image id="share-button"
>+              <image id="share-button-shared"

Couldn't we use a single dynamically-updated image for these, rather than two images with a stack?

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

>+let prefObserver = {
>+  observe: function(aSubject, aTopic, aData) {
>+    is(aData, prefName, "only should get notifications for expected prefname");

Don't need to check this, it will never happen.

>+    // Let the other pref observers run before we start with the tests.
>+    let prefEnabled = Services.prefs.getBoolPref(prefName);
>+    if (prefEnabled)
>+      setTimeout(test1, 500);
>+    else
>+      setTimeout(test12, 500);

executeSoon? Same applies to all the other uses of setTimeout in this test. If it doesn't work as executeSoon you've got a problem with the test that needs fixing.

>+function test0() {

>+  is(Services.prefs.getBoolPref(prefName), true, "test0, sanity check for setting a boolean pref");

this is also a bit silly of a check

>+function test8() {
>+  editSharePopup.removeEventListener("popupshown", test8, false);
>+  let focusedElement = document.querySelector(":focus");

document.activeElement? (also applies to test9)

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY social.shareButtonShared.tooltip "You bacon this">

Let's use "Share" rather than "Bacon" :)

>+<!ENTITY social.popupAvatar.arialabel     "Picture of user">

"User profile picture"?

r- to address these comments, but should be good after another pass.

Comment 43

[Willy_ Foo_Foo]

BTW For they people who hate Social or Value Privacy
will it be optional, and fully under the user's control.
(Better Fully Disabled?)

Comment 44

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

It will be disabled by default. The activation will be via the provider's web site (see bug 764869) or opt-in via Firefox prefs, so if you aren't a user of the supported providers you won't see any of this functionality.

Comment 45

[Willy_ Foo_Foo]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #44)
>It will be disabled by default or opt-in via Firefox prefs

That's why Love & support Mozilla 
because users are the ones who decide & can configure
Thanks

Comment 46

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

Attachment: Patch v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #42)
> >+      case "social.user-unrecommend-response":
> 
> >+        // For perceived performance, the UI was already updated for
> >+        // the expected case. Revert the UI if there was an error.
> 
> Is there an agreed-upon error message?

It's not in the docs yet, so I could remove it for now. I've left it in this version of the patch though.

> >+      case "social.ambient-notification-area":
> 
> What triggers the firing of this message from the social provider? Can we
> use it as a way to verify that the frameworker is set up correctly, and only
> enable the button once we've gotten this response? I guess this name is odd
> for that purpose, but I think we need some kind of such message.

Yeah, it's a pretty poorly named message. I am now keeping the button disabled until we get this message though. We may want to update the tooltips and icon when the button is disabled (not part of this patch though).

> >+  panelShown: function SRB_panelShown(aEvent) {
> 
> >+    this._sharePopupOkButton.focus();
> 
> Hmm, do we really want to be stealing focus like this? Is it even necessary
> given that the button has autofocus set?

I found that the focus wasn't being placed on the button, such that I could hit the space bar and trigger the default action without this.
 
> Couldn't we use a single dynamically-updated image for these, rather than
> two images with a stack?

Yeah, I switched this now to just use CSS to swap which image is displayed.

Comment 47

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

Comment on attachment 640017 [details] [diff] [review]
Patch v2

updateState seems to be called in a couple of places where it's unnecessary - the only thing that influences its behavior is currentBrowser._locationShared, and the calls to updateState in browser.js and in share()/unshare() take care of all the relevant cases where either currentBrowser or _locationShared can change, don't they? It can't change in response to the provider changing, or the global disabled/enabled state changing.

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

>+  dismissSharePopup: function SUI_dismissPopup() {
>+    let sharePopup = document.getElementById("editSharePopup");
>+    if (sharePopup)
>+      sharePopup.hidePopup();

This is called by a button in the popup, so the null check shouldn't be necessary.

>+  share: function SUI_share() {

>+      let currentURI = gBrowser.currentURI.clone();
>+      try {
>+        // Setting userPass on about:config throws.
>+        currentURI.userPass = "";
>+      } catch (e) {}
>+      let url = currentURI.spec;

Could add a helper for this (since it's duplicated in unshare()).

>+      // Provide a11y-friendly notification of share.
>+      let status = document.getElementById("share-button-status");
>+      if (status)
>+        status.setAttribute("value", status.getAttribute("sharedText"));

Shouldn't this be in updateState, so it can be reverted when the share is undone (or the tab is switched, new page loaded, etc.)?

>+    } else {
>+      let sharePopup = document.getElementById("editSharePopup");
>+      let shareButton = document.getElementById("share-button");
>+      if (sharePopup && shareButton)
>+        sharePopup.openPopup(shareButton, "bottomcenter topright");

Isn't it impossible for this method to be called when the share button is hidden?

>+  _providerSet: function SUI_providerSet(aData, aShareButton) {
>+    let provider = JSON.parse(aData);
>+    if (aShareButton)
>+      aShareButton.hidden = false;

If the button isn't present here (user removed URL bar), but it later gets re-added, it looks like nothing will unhide it. Seems like you need a way to keep track of the provider state globally, and check it every time the customize toolbox done event fires. Also, you need to check it for windows opened after the provider set event is fired, so that their buttons are unhidden too.

Same goes for _userAccountSet - all of this global state needs to be held by Social.jsm, and checked by consumers (windows) when needed.

>diff --git a/browser/modules/NewTabUtils.jsm b/browser/modules/NewTabUtils.jsm

>-const Cc = Components.classes;
>+const Cc = Components.classes;z

oops :)

>diff --git a/browser/modules/Social.jsm b/browser/modules/Social.jsm

>+  observe: function Social_observe(aSubject, aTopic, aData) {
>+    if (aTopic == "nsPref:changed")

Still don't need to check aTopic if you're only ever passing this as a pref observer :)

>+  share: function Social_share(url) {

>+    Services.obs.notifyObservers(null, "social:share", null);

This doesn't appear to be used?

Comment 48

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

Previous patch also doesn't seem to properly hide the button initially when the enabled pref is false.

I also just realized that the _userAccountSet stuff isn't going to work, because we're not sending social.initialize yet (and so we won't receive social.ambient-notification-area). So let's remove that function and all associated code for the moment, and just have the button be active as long as there's an active provider whose getWorkerPort is non-null. We can also remove the user account name/avatar from the panel for the moment, which should simplify things.

I will file a followup to get the workerAPI bits added.

Comment 49

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

(Filed bug 771877 for that.)

Comment 50

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #48)
> 
> just have the button be active as long
> as there's an active provider whose getWorkerPort is non-null. We can also
> remove the user account name/avatar from the panel for the moment, which
> should simplify things.

The way this worked in the past, we didn't enable the button unless we got a response from the worker that it supported the button.  In the case of motown, I'm not sure what it means to share a url, so until motown grows that functionality, it wouldn't have to have the button enabled.

other things:

I believe that setProvider and the loading of the providers should be a part of SocialService, as well SocialService should have a getter for currentProvider which returns a SocialProvider instance.  Share/Unshare should remain in the UI and use currentProvider.getWorkerPort().postMessage.  SocialProvider could have postMessage as a method to shorten.

The reason worker communication should not be a part of a singleton service is that it will complicate use cases in a multi provider system.  e.g. I have motown and xyz, with xyz as my current provider, I could still get chat request notifications from motown. 


+let Social = {
+  init: function Social_init() {
+    Services.prefs.addObserver(PREF_SOCIAL_ENABLED, this, false);
+
+    SocialService.getProviderList(function (providers) {
+      if (providers.length)
+        this.setProvider(providers[0]);
+    }.bind(this));
+  },

We have had async setup of the provider and found we had to do a notification here to startup the rest of the social ui functionality, since much of it needed to get at the currentProvider.  We had a social-service-ready notification for that (you can see it here https://github.com/mozilla/socialapi-dev/blob/develop/content/main.js#L203)  Perhaps we can instantiate this part of the service during the profile-after-change notification, that should get the reading of the providers early enough to be prepared for use by the ui.

Comment 51

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

Attachment: Patch v3

This patch is based on top of the patches for bug 771877 and bug 771980.

Comment 52

[Shane Caraveo (:mixedpuppy)]

I still feel my comments in comment 50 are valid, especially given that the SocialService has enabled in bug 771980.  I think Social.jsm should be split out, enabled/provider into SocialService and share/unshare into SocialUI.

Comment 53

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #25)
> This is still using a property off of a browser to store the current
> shared/not-shared bit. Unless we get this hooked in to some persistent store
> (bookmarks is likely the best), then we will need to do something like this.
> Back-forward navigation will not reapply the state, but at the same time
> users should have the ability to like pages multiple times over the course
> of using the browser (e.g. once in October, once again in July, etc).

Then the state should just be kept in memory and expire with the current session. The flag on the browser element surely looks like a design flaw.

Comment 54

[Dão Gottwald [:dao]]

Comment on attachment 640670 [details] [diff] [review]
Patch v3

>+    <command id="Social:Share" oncommand="SocialUI.share();"/>
>+    <command id="Social:Unshare" oncommand="SocialUI.unshare();"/>

Social:SharePage, Social:UnsharePage, SocialUI.sharePage, SocialUI.unsharePage

>+#share-button-status {
>+  /* Make the element invisible but still readable by screen readers. */
>+  font-size: 0;
>+}

What's the point of this when the element is already hidden via hidden="true"?

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -129,30 +129,33 @@ XPCOMUtils.defineLazyGetter(this, "Debug
> });
> 
> XPCOMUtils.defineLazyGetter(this, "Tilt", function() {
>   let tmp = {};
>   Cu.import("resource:///modules/devtools/Tilt.jsm", tmp);
>   return new tmp.Tilt(window);
> });
> 
>+Components.utils.import("resource:///modules/Social.jsm");

Why isn't this a lazy getter?

>+            <label id="share-button-status" role="status"
>+                   sharedText="&social.pageShared.label;"

>+            <image id="share-button"
>+                   class="urlbar-icon"
>+                   hidden="true"
>+                   disabled="true"
>+                   tooltiptext="&social.shareButton.tooltip;"
>+                   sharedTooltipText="&social.shareButtonShared.tooltip;"
>+                   unsharedTooltipText="&social.shareButton.tooltip;"

Attribute names are all-lowercase by convention. However, it probably makes more sense to put these strings in properties files anyway.

Comment 55

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

Attachment: Patch v4

Comment 56

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

Attachment: patch v5

I tweaked some things, and changed around how Social.jsm works a little, removed some parts (theme stuff leftover from the more involved popup). I think things mostly work, but I'm still having issues with the test. I think we should re-write it to use events or notifications instead of relying on executeSoons.

This is still on top of the latest patch in bug 771980.

Comment 57

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

Comment on attachment 640911 [details] [diff] [review]
patch v5

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

I think the test is hanging because the MoTown provider was removed. I believe that if we set a provider at the beginning then the test will work correctly, but we should still replace the remaining executeSoon's with events where possible. For the synthesizeKey one, I guess we could add an event listener for onkeyup, but everything in the patch looks good to me.

I think we can remove the margin-top on the buttons in the popup though. I added that when I was trying to get the styling to look right but at this point it's not really necessary and we could choose a different implementation for the styling when we get that far.

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 640911 [details] [diff] [review]
patch v5

>+#share-button-status {
>+  /* Make the element invisible but still readable by screen readers. */
>+  font-size: 0;
>+}

Can you use collapsed="true" instead?

Comment 59

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

Attachment: patch with working test

We'll need to improve this UX (the popup looks kind of weird as-is, strings might need to change), but this is disabled by default, so let's get this landed and iterate.

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

Comment 60

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

Comment on attachment 641567 [details] [diff] [review]
patch with working test

Sorry, wrong patch.

Comment 61

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

Attachment: patch with working test

Here's the right patch. "SocialUI" is more generic to fit with the work pending in bug 771826 / bug 755136. SocialShareButton is specific to the share button.

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

Comment 62

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

Attachment: patch with working test

Grr, I *again* attached the wrong patch. Sorry for the spam :(

Comment 63

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

Made a change to the test to fix the mac orange:
 function checkNextInTabOrder(element, next) {
+  // This particular test doesn't really apply on Mac, since buttons aren't
+  // focusable by default.
+  if (navigator.platform.indexOf("Mac") != -1) {
+    executeSoon(next);
+    return;
+  }

Comment 64

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

Comment on attachment 641575 [details] [diff] [review]
patch with working test

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

Look good, thanks for taking this patch to completion Gavin.

::: browser/base/content/browser-social.js
@@ +2,5 @@
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +let SocialUI = {
> +  // Called on delayed startup to initialized UI

s/initialized/initialize

@@ +14,5 @@
> +    Services.obs.removeObserver(this, "social:pref-changed");
> +  },
> +
> +  // Called when the social.enabled pref is changed
> +  observe: function SSB_observe(aSubject, aTopic, aData) {

Please rename the function to SocialUI_observe()

@@ +15,5 @@
> +  },
> +
> +  // Called when the social.enabled pref is changed
> +  observe: function SSB_observe(aSubject, aTopic, aData) {
> +    // Update the share button's state

nit: this comment seems redundant.

@@ +20,5 @@
> +    SocialShareButton.updateButtonEnabledState();
> +  },
> +
> +  // Called once Social.jsm's provider has been set
> +  _providerReady: function SocialUI_realInit() {

Please rename the function to SocialUI_providerReady()

::: browser/base/content/test/browser_shareButton.js
@@ +81,5 @@
> +    sharePopup.removeEventListener("popuphidden", listener);
> +    is(shareButton.hasAttribute("shared"), true, "Share button should still have 'shared' attribute after OK button is clicked");
> +    executeSoon(testSecondClick.bind(window, testPopupUndoButton));
> +  });
> +  let okButton = document.getElementById("editSharePopupOkButton");

This line here shouldn't be needed, it's redeclaring a local instance of |okButton| when there is one already declared in the outer scope.

@@ +109,5 @@
> +
> +  // Test a second invocation of the shortcut
> +  sharePopup.addEventListener("popupshown", function listener() {
> +    sharePopup.removeEventListener("popupshown", listener);
> +    ok(true, "popup was shown after second click");

The message here should be "popup was shown after second use of keyboard shortcut"

Comment 65

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/0e5b85c4fdde

Comment 66

[Matt Brubeck (:mbrubeck)]

It looks like this patch has regressed the Trace Malloc Allocs and Trace Malloc MaxHeap benchmarks on Windows and Linux, e.g. http://mzl.la/MqM4Gc

Would you prefer to back it out or file a follow-up bug to fix the regression?

Comment 67

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

(In reply to Matt Brubeck (:mbrubeck) from comment #66)
> It looks like this patch has regressed the Trace Malloc Allocs and Trace
> Malloc MaxHeap benchmarks on Windows and Linux, e.g. http://mzl.la/MqM4Gc
> 
> Would you prefer to back it out or file a follow-up bug to fix the
> regression?

Can you leave this patch here and file and follow-up bug? We need these patches to stick for Firefox 16.

Comment 68

[Matt Brubeck (:mbrubeck)]

(In reply to Jared Wein [:jaws] from comment #67)
> Can you leave this patch here and file and follow-up bug? We need these
> patches to stick for Firefox 16.

Filed bug 773845.

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e5b85c4fdde

Comment 70

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

Comment on attachment 641575 [details] [diff] [review]
patch with working test

Dao: would still appreciate you looking this code over at some point, but I'll cancel the request since this already landed.