Closed Bug 879982 Opened 8 years ago Closed 7 years ago

Implement "Email Link" widget

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: fhd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 5 obsolete files)

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?
(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?
Flags: needinfo?(jboriss)
Assignee: nobody → jaws
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).
Share landed in Fx23
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?
Status: NEW → ASSIGNED
Flags: needinfo?(jboriss) → needinfo?(mixedpuppy)
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.
(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.
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. :)
We clarified this yesterday, Jarad's direction is good for me.
Yoink
Assignee: jaws → mconley
Summary: Implement "Email Link" (or "Share"?) widget → Implement "Email Link" widget
These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7]
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Flags: needinfo?(mixedpuppy)
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?
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
(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.
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?]
Hmm. Stil P5, I think, as being infrequently used and to-be-deprecated in favor of the Social Share stuff anyway.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
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).
Assignee: mconley → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(fhd)
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.
(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.
If the user doesn't have Social enabled, then should we still have an email link widget?
(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.
Sure, I'll look into it.
Flags: needinfo?(fhd)
Attached patch Add Email Link widget (obsolete) — Splinter Review
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.
Attachment #822828 - Flags: review?(jaws)
Attachment #822828 - Flags: checkin+
Attached patch Add Email Link widget (2) (obsolete) — Splinter Review
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 :)
Attachment #822828 - Attachment is obsolete: true
Attachment #822828 - Flags: review?(jaws)
Attachment #822847 - Flags: review?(jaws)
Attachment #822847 - Flags: checkin+
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?
Attachment #822847 - Flags: review?(jaws) → review-
(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 :)
Attached patch Add Email Link widget (3) (obsolete) — Splinter Review
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.
Attachment #822847 - Attachment is obsolete: true
Attachment #823706 - Flags: review?(jaws)
Depends on: 932235
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)
Attachment #823706 - Flags: review?(jaws)
Attachment #823706 - Flags: review-
Attachment #823706 - Flags: feedback+
Assignee: nobody → fhd
Status: NEW → ASSIGNED
Attached patch Add Email Link widget (4) (obsolete) — Splinter Review
Here's a new patch, this one should address everything. Also removed one CSS ruleset that didn't seem necessary.
Attachment #823706 - Attachment is obsolete: true
Attachment #824326 - Flags: review?(mixedpuppy)
Attachment #824326 - Flags: review?(jaws)
Keywords: checkin-needed
Your patch needs review before being checked in. :-)
Keywords: checkin-needed
Oh sorry, I thought checkin-needed just means that it'll need checkin eventually, as opposed to right now :P
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
Attachment #824326 - Flags: review?(mixedpuppy)
Attachment #824326 - Flags: review?(jaws)
Attachment #824326 - Flags: feedback+
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jaws)
Flags: needinfo?(jaws)
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
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.
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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
We can land without the new icon. Felix, can you upload an updated patch?
Flags: needinfo?(fhd)
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.
Flags: needinfo?(fhd)
Attached patch bug-879982.patch (obsolete) — Splinter Review
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.
Attachment #824326 - Attachment is obsolete: true
Attachment #8335350 - Flags: review?(jaws)
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.
Attachment #8335350 - Flags: review?(jaws) → review-
Here's a new patch addressing the issues.
Attachment #8335350 - Attachment is obsolete: true
Attachment #8337130 - Flags: review?(jaws)
Attachment #8337130 - Flags: review?(jaws) → review+
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
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P3]
https://hg.mozilla.org/mozilla-central/rev/8ffcc8dc682b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.