879982 - Implement "Email Link" widget

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

According to the UX team, we want the "Email Link" function to be made into a button that can go into the menu panel.

In the mock-ups, this button has (I believe) been called "Share"...is that at all in conflict with any of the Social stuff?

Comment 1

[Zhenshuo Fang (:fang) - Firefox UX Team]

(In reply to Mike Conley (:mconley) from comment #0)

I talked to Boriss a while ago and seems like it's not in conflict with the current social work. The original reason we want to change it to "Share" was that we will add more function to the button other than just email link. But, since the social work will not be ready before we ship Australis, the only thing that button does is still emailing link... I'm fine with either way, changing it now or leaving it to "Email Link" till we have a more complete "Share" feature.

Boriss, do you have a preference?

Comment 2

[Florian Bender]

I'm a bit confused, does this mean "E-Mail this site" will end up in the "Share" paperplane button? I hope so :) 

It might be better though to adopt the model Safari uses: Show a simple list of (available) providers (incl. Mail) and upon a click, switch (or morph in)to the current Share doorhanger. This will mean that you need a second click to share with your "favorite" provider though it does not mean any more work for the other providers (furthermore, it delays loading of "unnecessary" content if you want to share with a different provider than the default one).

Comment 3

[Shane Caraveo (:mixedpuppy)]

Share landed in Fx23

Comment 4

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

This bug should probably be just to allow the Share button to move to the panel or the palette.

Shane, do you see any issues with doing this?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Whoa whoa whoa - I think we're getting confused here.

Unless I'm mistaken, I think the idea here is to create a widget that simply allows a user to fire the "Email Link" functionality. I think the confusion with "Share" was that the functionality was originally called "Share" in one of the mock-ups.

So I don't think this bug is about the same thing as the current "Share" button. I think it's about creating an "Email Link" button.

Comment 6

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

(In reply to Mike Conley (:mconley) from comment #5)
> Whoa whoa whoa - I think we're getting confused here.
> 
> Unless I'm mistaken, I think the idea here is to create a widget that simply
> allows a user to fire the "Email Link" functionality. I think the confusion
> with "Share" was that the functionality was originally called "Share" in one
> of the mock-ups.
> 
> So I don't think this bug is about the same thing as the current "Share"
> button. I think it's about creating an "Email Link" button.

Yes, but there will be overlap between both the icons for Social API "Share" and this button, as well as behavioral overlap. It doesn't make much sense for the user to have two share/email buttons in their toolbar.

Mike, see comment #1 and comment #3 as well. Since Social API's Share has already landed, I think it would be better to get their Share to a state where it can run independent of an installed provider and also be customizable.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hm, ok. Perhaps I misunderstood what Social API's Share was supposed to do / be responsible for.

Since you're someone who's worked on it, I trust you to know. :)

Comment 8

[Shane Caraveo (:mixedpuppy)]

We clarified this yesterday, Jarad's direction is good for me.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Yoink

Comment 10

[Mike Conley (:mconley) (:⚙️)]

These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 12

[Justin Dolske [:Dolske]]

This is new functionality -- P5, if we do it at all.

I don't understand why we'd be adding this button, though. Isn't the SocialAPI taking care of "Share Page" type stuff?

Comment 13

[Dão Gottwald [::dao]]

(In reply to Justin Dolske [:Dolske] from comment #12)
> This is new functionality -- P5, if we do it at all.

Nope, "Email Link" exists in the File menu and in the Firefox button menu.

Comment 14

[Justin Dolske [:Dolske]]

Hmm. Stil P5, I think, as being infrequently used and to-be-deprecated in favor of the Social Share stuff anyway.

Comment 15

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

Felix, would you like to work on this? We can just do the simple approach of implementing a new widget that will run the "Email Link" functionality. We can use the paper plane icon for this. You can probably follow the patch for the Find button widget, https://hg.mozilla.org/projects/ux/rev/826250367627 (bug 866838).

Comment 16

[Shane Caraveo (:mixedpuppy)]

quick off the cuff thoughts...

Wont the paper airplane icon be confusing when it does two different things?  As well, we would like to see email providers in the share panel.  I wonder if the share panel should grow a "local email apps" pane.

Comment 17

[Stephen Horlander [:shorlander]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> quick off the cuff thoughts...
> 
> Wont the paper airplane icon be confusing when it does two different things?
> As well, we would like to see email providers in the share panel.  I wonder
> if the share panel should grow a "local email apps" pane.

I think it definitely makes more sense to combine all "Share" related items into the same UI. Also agree we shouldn't use the Shareplane for more than one action.

Comment 18

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

If the user doesn't have Social enabled, then should we still have an email link widget?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #18)
> If the user doesn't have Social enabled, then should we still have an email
> link widget?

yes.  we do have plans in the works to have the share button always visible, and show options for share providers.  if we did something like a local app panel, then that could also be useful.  I think in the short term the regular email link menu is easier to deal with, it just shouldn't use the same icon.

Comment 20

[Felix H. Dahlke]

Sure, I'll look into it.

Comment 21

[Felix H. Dahlke]

Attachment: Add Email Link widget

Here's a patch, works fine on OSX and Windows. I can't manage to build on Linux right now, so that still needs testing. Can someone push this to the try server for me? Would give me a Linux build to test.

Comment 22

[Felix H. Dahlke]

Attachment: Add Email Link widget (2)

Oops, broke the test. Here's a patch that doesn't do that.

Also, I've requested level 1 commit access in bug 931419, that'd allow me to push this to the try server myself. Vouchers welcome :)

Comment 23

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

Comment on attachment 822847 [details] [diff] [review]
Add Email Link widget (2)

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

As I understand the previous comments in this bug (especially comment #19), this widget should only show up if Social isn't enabled.

We also need a different icon than the Shareplane, but you won't have to create that icon.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +144,5 @@
>        "fullscreen-button",
>        "find-button",
>        "preferences-button",
>        "add-ons-button",
> +      "email-link-button",

This widget shouldn't be a default widget in the menu panel. It should live in the palette by default.

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +373,5 @@
>           "The palette child count should have returned to its prior value.");
>        ok(CustomizableUI.inDefaultState, "Should still be in default state.");
>      },
>    },
> +/*

Why is this test commented out?

Comment 24

[Felix H. Dahlke]

(In reply to Jared Wein [:jaws] from comment #23) 
> We also need a different icon than the Shareplane, but you won't have to
> create that icon.

Then I guess I better duplicate the sharing rulesets instead of adding selectors to it? Will be easier to change the icon that way.

> :::
> browser/components/customizableui/test/
> browser_880382_drag_wide_widgets_in_panel.js
> @@ +373,5 @@
> >           "The palette child count should have returned to its prior value.");
> >        ok(CustomizableUI.inDefaultState, "Should still be in default state.");
> >      },
> >    },
> > +/*
> 
> Why is this test commented out?

Oops, my bad :)

Comment 25

[Felix H. Dahlke]

Attachment: Add Email Link widget (3)

Here's a new patch that should address everything. I haven't duplicated the share button rulesets here, let me know if you'd prefer that.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 823706 [details] [diff] [review]
Add Email Link widget (3)

I think that the email link needs to be entirely separate from the share button right now, since share being enabled does not mean the user has an email share provider.  We don't want to remove email functionality in that situation.

I've made a followup bug (bug 932236) to add a new panel to the share button that allows a user to use "system apps" which, IIUC, is what sendLinkForWindow does.  At that time, we could remove the email button entirely (might need a little more thought on this).

So:

- undo the change in browser-social.js
- remove the empty change in browser_880382_drag_wide_widgets_in_panel.js
- change the css so that the email icon is separated from the share css.  You can reuse the share image until someone creates a new image (bug 932235 for shorlander or someone)

Comment 27

[Felix H. Dahlke]

Attachment: Add Email Link widget (4)

Here's a new patch, this one should address everything. Also removed one CSS ruleset that didn't seem necessary.

Comment 28

[:Gijs (he/him)]

Your patch needs review before being checked in. :-)

Comment 29

[Felix H. Dahlke]

Oh sorry, I thought checkin-needed just means that it'll need checkin eventually, as opposed to right now :P

Comment 30

[:Gijs (he/him)]

Comment on attachment 824326 [details] [diff] [review]
Add Email Link widget (4)

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

Generally, this looks good, but I have a bunch of nits, so I'd like someone to look at the next revision of the patch. Hence feedback+ rather than r+, but it's a narrow miss. :-)

As you're no longer touching social, I don't think you need Shane's review otherwise, so no worries about that. :-)

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +778,5 @@
>      }
> +  }, {
> +    id: "email-link-button",
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_PANEL,

This line ^ should have been removed as well, I believe (in addition to not including it in the panel by default).

@@ +780,5 @@
> +    id: "email-link-button",
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_PANEL,
> +    onCommand: function(aEvent) {
> +      let win = aEvent.target

nit: let win = aEvent.view;

@@ +783,5 @@
> +    onCommand: function(aEvent) {
> +      let win = aEvent.target
> +                && aEvent.target.ownerDocument
> +                && aEvent.target.ownerDocument.defaultView;
> +      if (win && win.MailIntegration) {

I don't think there's a non-pathological case where we'll have a window-less event, so don't bother nullchecking that. The command (used by the menu etc.) doesn't nullcheck MailIntegration, so you shouldn't either. :-)

::: browser/themes/osx/browser.css
@@ +587,5 @@
>      -moz-image-region: rect(36px, 612px, 54px, 594px);
>    }
>  
> +  #email-link-button@toolbarButtonPressed@ {
> +    -moz-image-region: rect(18px, 306px, 36px, 288px);

Nit: these are ordered from left to right in Toolbar.png, i.e. this should be after the item(s) between horizontal coordinates 288,270. :-)

The same applies further down in this file, and in toolbarbuttons.inc.css and menupanel.inc.css

Comment 31

[Felix H. Dahlke]

(In reply to :Gijs Kruitbosch from comment #30)
> @@ +783,5 @@
> > +    onCommand: function(aEvent) {
> > +      let win = aEvent.target
> > +                && aEvent.target.ownerDocument
> > +                && aEvent.target.ownerDocument.defaultView;
> > +      if (win && win.MailIntegration) {
> 
> I don't think there's a non-pathological case where we'll have a window-less
> event, so don't bother nullchecking that. The command (used by the menu
> etc.) doesn't nullcheck MailIntegration, so you shouldn't either. :-)

I did that because pretty much all the other widget commands are very nervous about null values. I couldn't really figure out why, and email link does work fine without all that checking, so I'll remove it.

> ::: browser/themes/osx/browser.css
> @@ +587,5 @@
> >      -moz-image-region: rect(36px, 612px, 54px, 594px);
> >    }
> >  
> > +  #email-link-button@toolbarButtonPressed@ {
> > +    -moz-image-region: rect(18px, 306px, 36px, 288px);
> 
> Nit: these are ordered from left to right in Toolbar.png, i.e. this should
> be after the item(s) between horizontal coordinates 288,270. :-)
> 
> The same applies further down in this file, and in toolbarbuttons.inc.css
> and menupanel.inc.css

This won't be checked in before we get the new icon I guess, so wouldn't it make more sense to keep the rulesets at the end? That's where the new icon will go, won't it?

Comment 32

[:Gijs (he/him)]

(In reply to Felix H. Dahlke from comment #31)
> This won't be checked in before we get the new icon I guess

This I'm not sure about. I assumed it would land on UX just fine. I'd like to defer to Jared.

Comment 33

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

Hm, before landing it on UX I think we should make some change to the icon to make it obvious that it is temporary. Even doing a transform:scale(-1, -1); on the icon so it is obvious that something is wrong here should suffice.

Bug 932235 was already filed to get this icon, so we can undo the transform there.

Comment 34

[Felix H. Dahlke]

IMO, it'd make sense to wait for the final icon. This doesn't seem critical and the bug's already 4 months old, why not wait a bit longer and have a single clean check in for the feature?

But it's your call Jared, would you rather have this checked in now?

Comment 35

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

We could wait, but patches are also easy to forget about and turn-around-time on icons isn't the fastest since they typically aren't the highest priority either.

Comment 36

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

We can land without the new icon. Felix, can you upload an updated patch?

Comment 37

[Felix H. Dahlke]

Sure, I'll do that in a bit. I wanted to wait so I wouldn't have to move the rule sets around twice, but I don't really mind.

Comment 38

[Felix H. Dahlke]

Attachment: bug-879982.patch

Here's a new patch. I rebased, moved the rule sets I added to their proper place and also applied that transformation Jared suggested to the icon.

Comment 39

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

Comment on attachment 8335350 [details] [diff] [review]
bug-879982.patch

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

::: browser/themes/osx/browser.css
@@ +599,5 @@
>    }
>  
> +  #email-link-button@toolbarButtonPressed@ {
> +    -moz-image-region: rect(18px, 306px, 36px, 288px);
> +    transform: scale(-1, -1);

You can move out the transform to its own rule that is always applied to #email-link-button and has a comment explaining that it is only temporary until we have an icon for this.

/* This rule is temporary until we have
   an email-link icon (bug XXX). */
#email-link-button {
  transform: scale(-1, -1);
}

::: browser/themes/shared/menupanel.inc.css
@@ +61,5 @@
>  }
>  
> +#email-link-button[cui-areatype="menu-panel"],
> +toolbarpaletteitem[place="palette"] > #email-link-button {
> +  -moz-image-region: rect(0px, 448px, 32px, 416px);

nit, use 0 instead of 0px here and in toolbarbuttons.inc.css.

@@ +64,5 @@
> +toolbarpaletteitem[place="palette"] > #email-link-button {
> +  -moz-image-region: rect(0px, 448px, 32px, 416px);
> +}
> +
> +#email-link-button[cui-areatype="menu-panel"] > image,

Add a comment here explaining that it is temporary and reference the bug for the new icon.

Comment 40

[Felix H. Dahlke]

Attachment: Add Email Link widget (5)

Here's a new patch addressing the issues.

Comment 41

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

Thanks for helping to fix this bug Felix! I've checked it in to the fx-team repository. It should get merged to mozilla-central today as long as all tests pass (I ran the customizableui test suite locally and it was all green).

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

Comment 42

[Carsten Book [:Tomcat]]

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