582615 - Sharing front-end

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Implement the UI for the new Sharing feature in Fennec 2.0.

See specs at https://wiki.mozilla.org/Mobile/Projects/Sharing#Designs

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 1

Fully functional prototype with three hard-coded sharing options (Twitter, Google Reader, and Facebook).  Open the site menu and choose "Share Page."

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Add sharing of links and images via the context menu.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

Code cleanup; fix handling of escape key and interaction with popups.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

oops, set the wrong content type

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 4

Update to latest m-b, add email sharing, i18n, some dialog style tweaks.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 5

Use nsIExternalProtocolService to open mailto: link.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461306 [details] [diff] [review]
WIP 5

>diff -r ec0920b0e0ad chrome/content/browser-ui.js

>+  _share: function _share(aURL, aTitle) {

Looks like _share is called from external code, so remove the "_" to make it "public"

>+    let dlg = importDialog(window, "chrome://browser/content/share.xul", {});
>+    document.getElementById("share-title").value = aTitle || aURL;
>+
>+    BrowserUI._hidePopup();

Why this call?

>+  _shareHandlers: [

Spaces between the "+" in this code would be nice for readability and line length is not hard and fast. Feel free to unwrap these lines.

>+      name: "Twitter",

>+        Browser.addTab(url, true, Browser.selectedTab);

>+      name: "Google Reader",

>+        Browser.addTab(url, true, Browser.selectedTab);

>+      name: "Facebook",

>+        Browser.addTab(url, true, Browser.selectedTab);

I'm not sure this was Madhava's initial intent. I'm OK with this as a first step



>diff -r ec0920b0e0ad chrome/content/browser.xul

>-          <richlistitem id="context-editbookmark" type="edit-bookmark" onclick="ContextCommands.editBookmark(event);">
>+          <richlistitem id="context-editbookmark" type="edit-bookmark" onclick="ContextCommands.editBookmark();">
>             <label value="&contextEditBookmark.label;"/>
>           </richlistitem>
>-          <richlistitem id="context-removebookmark" type="edit-bookmark" onclick="ContextCommands.removeBookmark(event);">
>+          <richlistitem id="context-share-link" type="link" onclick="ContextCommands.shareLink();">
>+            <label value="&contextShareLink.label;"/>
>+          </richlistitem>
>+          <richlistitem id="context-share-image" type="image" onclick="ContextCommands.shareMedia();">
>+            <label value="&contextShareImage.label;"/>
>+          </richlistitem>
>+          <richlistitem id="context-removebookmark" type="edit-bookmark" onclick="ContextCommands.removeBookmark();">
>             <label value="&contextRemoveBookmark.label;"/>
>           </richlistitem>

move the "context-editbookmark" item down to just above the "context-removebookmark", keeping them together as a group

>diff -r ec0920b0e0ad chrome/content/content.js

>     let state = {
>       types: [],
>       label: "",
>       linkURL: "",
>+      linkTitle: "",
>       linkProtocol: null,
>-      mediaURL: ""
>+      mediaURL: "",
>+      mediaTitle: ""

I got a little sad to see us pushing more state in here after just removing a bunch. I'm trying to see if there is a different way to do this.


>diff -r ec0920b0e0ad chrome/content/share.xul

>+  <commandset>
>+    <command id="cmd_cancel" oncommand="document.getElementById('share-dialog').close()"/> <!-- XXX -->
>+  </commandset>

What's the XXX about?

r-, just for discussing this a bit more. Looks very good though.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 6

Better code organization; whitespace changes.

If this gets much larger we can move it into a separate .js or .jsm file.

> I'm not sure this was Madhava's initial intent. I'm OK with this as a first
> step

Yeah, the Email/Twitter/Reader/Facebook "backend" is just a temporary stub, it should all be replaced (or used only as a fallback) when the real back-end code is written.

> I got a little sad to see us pushing more state in here after just removing a
> bunch. I'm trying to see if there is a different way to do this.

Well, it turned out the mediaTitle attribute was basically never useful for sharing so I removed it.  The only new attribute now is linkTitle.

> What's the XXX about?

Leftover from an earlier version; removed now.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Some pieces you missed answering:

> >+    let dlg = importDialog(window, "chrome://browser/content/share.xul", {});
> >+    document.getElementById("share-title").value = aTitle || aURL;
> >+
> >+    BrowserUI._hidePopup();
> 
> Why this call?

> >-          <richlistitem id="context-editbookmark" type="edit-bookmark" onclick="ContextCommands.editBookmark(event);">
> >+          <richlistitem id="context-editbookmark" type="edit-bookmark" onclick="ContextCommands.editBookmark();">
> >             <label value="&contextEditBookmark.label;"/>
> >           </richlistitem>
> >-          <richlistitem id="context-removebookmark" type="edit-bookmark" onclick="ContextCommands.removeBookmark(event);">
> >+          <richlistitem id="context-share-link" type="link" onclick="ContextCommands.shareLink();">
> >+            <label value="&contextShareLink.label;"/>
> >+          </richlistitem>
> >+          <richlistitem id="context-share-image" type="image" onclick="ContextCommands.shareMedia();">
> >+            <label value="&contextShareImage.label;"/>
> >+          </richlistitem>
> >+          <richlistitem id="context-removebookmark" type="edit-bookmark" onclick="ContextCommands.removeBookmark();">
> >             <label value="&contextRemoveBookmark.label;"/>
> >           </richlistitem>
> 
> move the "context-editbookmark" item down to just above the
> "context-removebookmark", keeping them together as a group
>

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: part 1 v7

(In reply to comment #8)
> >+    BrowserUI._hidePopup();
> Why this call?

To hide the context menu or site menu that spawned the sharing dialog.

> move the "context-editbookmark" item down to just above the
> "context-removebookmark", keeping them together as a group

Done.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461363 [details] [diff] [review]
part 1 v7

Two issues with this after running with it:
* The prompt-header text color is black and should be white
* The sharing dialog does not dismiss if I tap outside of it. It looks like a context-panel, not a dialog, so I expected it to work the same way.

I think this rule was trying to make the text default to white, but it's not making it happen anymore:
http://mxr.mozilla.org/mobile-browser/source/themes/core/platform.css#122

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: patch v8

(In reply to comment #12)
> * The prompt-header text color is black and should be white

I can't reproduce this.  It looks white to me (with the current patch applied to mozilla-central tip and mobile-browser tip on desktop Linux and Android).

> * The sharing dialog does not dismiss if I tap outside of it. It looks like a
> context-panel, not a dialog, so I expected it to work the same way.

Fixed - the sharing dialog is now a proper popup.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Looking very good. I see a small error we can fixup later:

JavaScript strict warning: chrome://browser/content/bindings/dialog.xml, line 0: reference to undefined property this.getElementsByTagName("scrollbox")[0]

See:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/dialog.xml#15
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/dialog.xml#28

I think we can just make the getter a bit more robust to fix this.

I also see we need to add a cropt="center" into the prompt-header, like we do for the prompt dialogs

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461448 [details] [diff] [review]
patch v8

>diff -r 172857e8c6f6 chrome/content/browser-ui.js
>+  updateShare: function updateShare() {

>+  },
>+
>+

remove extra line

>+  removeBookmark: function cc_removeBookmark() {
>     let target = ContextHelper.popupState.target;
>     target.remove();
>   }
> }
> 
>+var SharingUI = {

add an extra line between them

>+}
>+
> function removeBookmarksForURI(aURI) {

and here

>diff -r 172857e8c6f6 chrome/content/share.xul

>+  <hbox class="prompt-header">
>+    <label id="share-title" class="prompt-title" crop="center"/>
>+  </hbox>

Turns out you need a flex="1" on the <label> for the crop to work. Add them flex="1" back in the other prompt dialogs too.

r- cause I know you have a new patch coming to fix the flex.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: patch v9

(In reply to comment #15)
> >+  updateShare: function updateShare() {
> 
> >+  },
> >+
> >+
> 
> remove extra line

Done.

> >+var SharingUI = {
> 
> add an extra line between them

Done.

> >+}
> >+
> > function removeBookmarksForURI(aURI) {
> 
> and here

Done, and fixed the missing semicolon after }.

> >+  <hbox class="prompt-header">
> >+    <label id="share-title" class="prompt-title" crop="center"/>
> >+  </hbox>
> 
> Turns out you need a flex="1" on the <label> for the crop to work. Add them
> flex="1" back in the other prompt dialogs too.

Done.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 461458 [details] [diff] [review]
patch v9

The title of the sharing popup is still black for me, but if it's a real problem, it's a simple fix.

This patch does a very good job setting up the front-end of the sharing system.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/b1c75dcf313f

Comment 19

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:2.0b4pre) Gecko/20100812 Namoroka/4.04pre Fennec/2.0a1pre

Comment 20

[Tony Chung [:tchung]]

Litmus test: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12771.  this will be updated when native app support land. 

can we get automated tests also?

Comment 21

[Tony Chung [:tchung]]

litmus test: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=12770