791901 - providers should specify the "You shared this page" and "undo share" button labels.

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

In the share popup (poorly named - it is really the "unshare popup", as no popup is shown when initially sharing), there are 2 labels that should be specified by the provider:

socialUserRecommendedText - currently "You shared this page".
editSharePopupUndoButton - currently "Unshare".

Comment 1

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

For l10n purposes, we might as well allow the provider to specify the "OK" text as well. This way if the user has a separate locales for their social network then the popup won't show strings in two different locales.

Comment 2

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

Attachment: Accept more strings from the provider for the share process.

It turns out we also need them to specify the "accesskey" for those elements, and the aria-label for the profile image.

Attaching a patch which fixes this.  In particular, I'd like feedback on the string names I've added - please see the change to social_worker.js - all of these strings will form part of our documented API, so they should be as descriptive as possible.  If you are happy with the names, please feel free to upgrade the feedback request to a review request ;)

Comment 3

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

Comment on attachment 662035 [details] [diff] [review]
Accept more strings from the provider for the share process.

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

::: browser/base/content/browser-social.js
@@ +372,5 @@
>        }
>        promptMessages[sub] = data.messages[sub];
>      }
> +    // The following attributes have default values to avoid breaking a
> +    // specific provider, but should be removed.

Please file a follow-up bug to track the removal of these strings.

@@ +427,5 @@
>      let sharePopupOkButton = document.getElementById("editSharePopupOkButton");
> +    sharePopupOkButton.focus();
> +    // The labels here seem to be back-to-front, but this is by design:
> +    // * The OK button is really just cancelling the unshare process.
> +    // * The Undo button is actually taking the unshare action.

Let's rename these buttons to avoid this confusion.

What do you think about unsharePopupContineSharingButton and unsharePopupStopSharingButton? A little wordy, but hopefully clears up any confusion in the code.

We can also rename the popup to be the unsharePopup.

Comment 4

[Axel Hecht [:Pike]]

Comment on attachment 662035 [details] [diff] [review]
Accept more strings from the provider for the share process.

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

::: browser/base/content/browser-social.js
@@ +379,5 @@
> +        ["unshareLabel", "You shared this page."],
> +        ["unshareConfirmLabel", "OK"],
> +        ["unshareConfirmAccessKey", "O"],
> +        ["unshareCancelLabel", "Unshare"],
> +        ["unshareCancelAccessKey", "U"]]) {

Why wouldn't we keep the defaults localized?

Comment 5

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

(In reply to Axel Hecht [:Pike] from comment #4)
> Comment on attachment 662035 [details] [diff] [review]
> Accept more strings from the provider for the share process.
> 
> Review of attachment 662035 [details] [diff] [review]:
> -----------------------------------------------------------------
> Why wouldn't we keep the defaults localized?

The defaults are only there for a short transitional period while the providers adjust - after that, they MUST be supplied for the feature to work correctly.   We hope that all providers will be able to adjust within a week or so and well before general rollout of this feature.

Comment 6

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

I'm not sure we should make providing these strings a hard requirement - it makes implementing the feature for providers more difficult, and it's not that hard for us to provide localized defaults.

Comment 7

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> I'm not sure we should make providing these strings a hard requirement - it
> makes implementing the feature for providers more difficult, and it's not
> that hard for us to provide localized defaults.

I see 2 problems here:

* The defaults will be in the locale Firefox is using, which may be a different locale than the provider is using.  This is why Jaws suggested the "OK" label also be provided.

* Apart from the "OK" string, all other strings have the word "share" in it.  We already insist they provide the "share tooltip" string, which will have whatever replacement they choose for the word "share" - it doesn't make sense they would decline to provide those other strings and just leave "share" in place there.  Or to put it another way - if they want consistency, they need to override all.

The problem I have is that the defaults are too generic to be useful

Comment 8

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

(In reply to Mark Hammond (:markh) from comment #7)
> * The defaults will be in the locale Firefox is using, which may be a
> different locale than the provider is using.  This is why Jaws suggested the
> "OK" label also be provided.

I don't think this is a problem, some providers will be OK with that.

> We already insist they provide the "share tooltip" string

I didn't realize we had done that, but it seems like something we could easily revert.

Comment 9

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> (In reply to Mark Hammond (:markh) from comment #7)
> > * The defaults will be in the locale Firefox is using, which may be a
> > different locale than the provider is using.  This is why Jaws suggested the
> > "OK" label also be provided.
> 
> I don't think this is a problem, some providers will be OK with that.

They won't.  Certainly the providers we are currently targeting will not.  Why not just nuke them all and consider re-adding if we get feedback that the burden of providing all/any such strings is high (which I wager we will never get)?

> > We already insist they provide the "share tooltip" string
> 
> I didn't realize we had done that, but it seems like something we could
> easily revert.

Sure.  But the cost of carrying these strings around and forcing our localizers to maintain them when they will *not ever* be used in practice for the providers we are currently targeting seems wasteful and pointless.

Comment 10

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

OK, you've convinced me. But in that case I don't see why we should bother with interim defaults at all - why not just rip them out now?

Comment 11

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> I don't see why we should bother
> with interim defaults at all - why not just rip them out now?

IIUC, one of our providers intends landing support for the share button in the next day or 2, but we have not yet communicated these new strings to them - so if we ripped them out, it would cause that new support to not appear until they again revisit that code.  The plan with the defaults was so we could start experimenting with their share support as soon as it lands based on our existing spec (ie, without those strings)

IOW, I expect the defaults to exist for literally 2 weeks max.

Comment 12

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

Comment on attachment 662035 [details] [diff] [review]
Accept more strings from the provider for the share process.

Are we good to land this?

Comment 13

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

Attachment: remove string defaults, rename panel and buttons

This updated patch:

* Removes the defaults for the new strings - providers must supply all strings.

* As per Jaws's suggestion, renames the panel and buttons to "unshare..." and "...stopSharing/...continueSharing", and renames other methods or property-getters accordingly.

As the rename part of the patch touches many more files, a try run is at https://tbpl.mozilla.org/?tree=Try&rev=fddfd6f76969

Comment 14

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

Comment on attachment 663890 [details] [diff] [review]
remove string defaults, rename panel and buttons

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

::: browser/base/content/browser-social.js
@@ +377,5 @@
>        }
>        promptImages[sub] = url;
>      }
> +    for (let sub of ["shareTooltip", "unshareTooltip", "sharedLabel",
> +                     "unsharedLabel", "portraitLabel", "unshareLabel",

nit: please place the "unsharedLabel" and "unshareLabel" adjacent to each other, so it is easier to see the slight difference in name.

@@ +429,5 @@
> +    stopSharingButton.setAttribute("accesskey", this.promptMessages.unshareConfirmAccessKey);
> +    let profileImage = document.getElementById("socialUserPortrait");
> +    profileImage.setAttribute("aria-label", this.promptMessages.portraitLabel);
> +    let recommendedText = document.getElementById("socialUserRecommendedText");
> +    recommendedText.setAttribute("value", this.promptMessages.unshareLabel);

function updateElement(id, attrs) {
  let el = document.getElementById(id);
  attrs && Object.keys(attrs).forEach(function(attr) {
    el.setAttribute(attr, attrs[attr]);
  });
}
updateElement("unsharePopupStopSharingButton", {label: this.promptMessages.unshareConfirmLabel, accesskey: this.promptMessages.unshareConfirmAccessKey});

... and so on. For "aria-label" the object literal notation won't work, but using obj["aria-label"] should.

::: browser/base/content/test/browser_social_shareButton.js
@@ +66,1 @@
>    

while making all of these changes, can you remove this extra whitespace on this line here? :)

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a802a8baf3f8

Comment 16

[Ed Morley [:emorley]]

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

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e80b671b68b