Closed Bug 582615 Opened 9 years ago Closed 9 years ago

Sharing front-end

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

()

Details

Attachments

(2 files, 9 obsolete files)

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

See specs at https://wiki.mozilla.org/Mobile/Projects/Sharing#Designs
Blocks: 582616
Blocks: 582621
Attached patch WIP 1 (obsolete) — Splinter Review
Fully functional prototype with three hard-coded sharing options (Twitter, Google Reader, and Facebook).  Open the site menu and choose "Share Page."
Attached image screenshot
Attached patch WIP 2 (obsolete) — Splinter Review
Add sharing of links and images via the context menu.
Attachment #461055 - Attachment is obsolete: true
Attached file WIP 3 (obsolete) —
Code cleanup; fix handling of escape key and interaction with popups.
Attachment #461064 - Attachment is obsolete: true
Attachment #461090 - Flags: review?(mark.finkle)
Attached patch WIP 3 (obsolete) — Splinter Review
oops, set the wrong content type
Attachment #461090 - Attachment is obsolete: true
Attachment #461091 - Flags: review?(mark.finkle)
Attachment #461090 - Flags: review?(mark.finkle)
Attached patch WIP 4 (obsolete) — Splinter Review
Update to latest m-b, add email sharing, i18n, some dialog style tweaks.
Attachment #461091 - Attachment is obsolete: true
Attachment #461258 - Flags: review?(mark.finkle)
Attachment #461091 - Flags: review?(mark.finkle)
Attached patch WIP 5 (obsolete) — Splinter Review
Use nsIExternalProtocolService to open mailto: link.
Attachment #461258 - Attachment is obsolete: true
Attachment #461306 - Flags: review?(mark.finkle)
Attachment #461258 - Flags: review?(mark.finkle)
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.
Attachment #461306 - Flags: review?(mark.finkle) → review-
Attached patch WIP 6 (obsolete) — Splinter Review
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.
Attachment #461306 - Attachment is obsolete: true
Attachment #461350 - Flags: review?(mark.finkle)
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
>
Attached patch part 1 v7 (obsolete) — Splinter Review
(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.
Attachment #461350 - Attachment is obsolete: true
Attachment #461363 - Flags: review?(mark.finkle)
Attachment #461350 - Flags: review?(mark.finkle)
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
Attached patch patch v8 (obsolete) — Splinter Review
(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.
Attachment #461363 - Attachment is obsolete: true
Attachment #461448 - Flags: review?(mark.finkle)
Attachment #461363 - Flags: review?(mark.finkle)
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 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.
Attachment #461448 - Flags: review?(mark.finkle) → review-
Attached patch patch v9Splinter Review
(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.
Attachment #461448 - Attachment is obsolete: true
Attachment #461458 - Flags: review?(mark.finkle)
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.
Attachment #461458 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/b1c75dcf313f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fix on Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:2.0b4pre) Gecko/20100812 Namoroka/4.04pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
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?
tracking-fennec: ? → ---
Flags: in-testsuite?
Flags: in-litmus+
You need to log in before you can comment on or make changes to this bug.