Closed
Bug 879982
Opened 11 years ago
Closed 10 years ago
Implement "Email Link" widget
Categories
(Firefox :: Toolbars and Customization, defect)
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)
7.87 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
(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)
Updated•11 years ago
|
Assignee: nobody → jaws
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Share landed in Fx23
Comment 4•11 years ago
|
||
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)
Reporter | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
(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.
Reporter | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
We clarified this yesterday, Jarad's direction is good for me.
Reporter | ||
Updated•11 years ago
|
Summary: Implement "Email Link" (or "Share"?) widget → Implement "Email Link" widget
Reporter | ||
Comment 10•11 years ago
|
||
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]
Reporter | ||
Comment 11•11 years ago
|
||
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Updated•11 years ago
|
Flags: needinfo?(mixedpuppy)
Comment 12•11 years ago
|
||
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]
Comment 13•11 years ago
|
||
(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?]
Comment 14•11 years ago
|
||
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]
Comment 15•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
If the user doesn't have Social enabled, then should we still have an email link widget?
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 21•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #822847 -
Flags: checkin+
Comment 23•10 years ago
|
||
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-
Assignee | ||
Comment 24•10 years ago
|
||
(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 :)
Assignee | ||
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
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+
Updated•10 years ago
|
Assignee: nobody → fhd
Status: NEW → ASSIGNED
Assignee | ||
Comment 27•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 28•10 years ago
|
||
Your patch needs review before being checked in. :-)
Keywords: checkin-needed
Assignee | ||
Comment 29•10 years ago
|
||
Oh sorry, I thought checkin-needed just means that it'll need checkin eventually, as opposed to right now :P
Comment 30•10 years ago
|
||
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+
Assignee | ||
Comment 31•10 years ago
|
||
(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)
Comment 32•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(jaws)
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
Comment 33•10 years ago
|
||
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.
Assignee | ||
Comment 34•10 years ago
|
||
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)
Comment 35•10 years ago
|
||
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)
Comment 36•10 years ago
|
||
We can land without the new icon. Felix, can you upload an updated patch?
Flags: needinfo?(fhd)
Assignee | ||
Comment 37•10 years ago
|
||
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)
Assignee | ||
Comment 38•10 years ago
|
||
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 39•10 years ago
|
||
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-
Assignee | ||
Comment 40•10 years ago
|
||
Here's a new patch addressing the issues.
Attachment #8335350 -
Attachment is obsolete: true
Attachment #8337130 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #8337130 -
Flags: review?(jaws) → review+
Comment 41•10 years ago
|
||
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]
Comment 42•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8ffcc8dc682b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•