Closed Bug 1171929 Opened 5 years ago Closed 4 years ago

When sharing selected text, also include the current page URL

Categories

(Firefox for Android :: Text Selection, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 + verified
relnote-firefox --- 43+

People

(Reporter: Margaret, Assigned: qiubit, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(2 files, 4 obsolete files)

For easy sharing of excerpts from articles.

Fixing this bug would involve changing the logic here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#1107
hi! i would like to work on this bug. 
could you just guide me how to approach it.
I'm working on this bug and I'm not sure as for how should I approach it. Should I append the URL to selected text (separated with newlines) or add page title as well? Also, what if someone opens a textfile in Fennec, should I append url as well in that case (e.g. "file:///storage/emulated/0/test") or should I check for scheme and only append url for webpages (so only if scheme is "http://" or "https://")?
Flags: needinfo?(margaret.leibovic)
Attached patch bug1171929 (obsolete) — Splinter Review
Anyway, I'm attatching simple patch with adding URL no matter what the situation is (I'm just copying current URL two newlines after shared text)
Attachment #8641745 - Flags: review?(margaret.leibovic)
I think it's fine to add the URL in all cases.

I haven't looked into the Intent documentation closely, but I wonder if we can do a better job formatting the share intent, such that it has a title and a body (e.g. sharing to gmail will properly set the title and message body).

mcomella, are you familiar with this?
Flags: needinfo?(margaret.leibovic) → needinfo?(michael.l.comella)
Comment on attachment 8641745 [details] [diff] [review]
bug1171929

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

Redirecting review here to capella, since he's the expert on this code :)
Attachment #8641745 - Flags: review?(margaret.leibovic) → review?(markcapella)
Comment on attachment 8641745 [details] [diff] [review]
bug1171929

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

I'm happy to f+ here, as the code is pretty straight-forward, but I'd like to hold off on r+ until feedback from mcomella re:intents, and maybe some further discussion of handling email, ie: how might we populate subject, etc.

Also, don't forget we'll need to duplicate the code change from SelectionHandler.js to ActionBarHandler.js, which is destined as it's replacement.

See starting around https://bugzilla.mozilla.org/show_bug.cgi?id=988143#c74 for details how to test that.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +1109,2 @@
>      let selectedText = this._getSelectedText();
> +    let selectedTextWithUrlString = selectedText + "\n\n" + selectionUrl.asciiSpec;

I think we'd prefer the more usual foo.spec here vs foo.asciiSpec, unless you can explain why you favor this?

iirc, asciiSpec has special uses for transmitting to server backends which doesn't seem the case here. Also arguing against, would be for users of Arabic sites such as:

http://موقع.وزارة-الأتصالات.مصر/

Which when pasted into mail for example will appear as:

http://xn--4gbrim.xn----rmckbbajlc6dj7bxne2c.xn--wgbh1c/
Attachment #8641745 - Flags: review?(markcapella) → feedback+
It's probably also in the scope of the request to do a quick conversion of the page url to remove the |about:reader| portion added when the user is sharing text from a page displayed in ReaderView mode.

see:
[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0ebb7da63ced&mark=4744-4744#4742
(In reply to :Margaret Leibovic from comment #5)
> I haven't looked into the Intent documentation closely, but I wonder if we
> can do a better job formatting the share intent, such that it has a title
> and a body (e.g. sharing to gmail will properly set the title and message
> body).

It looks like there is:
* Intent.EXTRA_SUBJECT [1]
* Intent.EXTRA_TITLE [2]
* Intent.EXTRA_TEXT [3]

in addition to the standard data field. We use them for the share menu at [4].

[1]: http://developer.android.com/reference/android/content/Intent.html#EXTRA_SUBJECT
[2]: http://developer.android.com/reference/android/content/Intent.html#EXTRA_TITLE
[3]: http://developer.android.com/reference/android/content/Intent.html#EXTRA_TEXT

[4]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=f327907afbb1#3329
Flags: needinfo?(michael.l.comella)
Assignee: nobody → qiubit.bugzilla
Attached patch bug1171929.diff (obsolete) — Splinter Review
Here's another version of the patch, with all features added in experimental version as well. I also added escaping the URL special characters in GeckoApp.java, because otherwise we would attach something like "http%3A%2F%2Fwww.jwir3.com%2Fblog%2F2012%2F07%2F30%2Ffont-inflation-fennec-and-you%2F" when sharing a text through Reader Mode which is quite unuseful (e.g. you can't click on such a link in Android, and copying it to all browsers I tested caused unexpected results)
Attachment #8641745 - Attachment is obsolete: true
Attachment #8646388 - Flags: review?(markcapella)
Comment on attachment 8646388 [details] [diff] [review]
bug1171929.diff

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

Better! Looking here again, I think we can simplify this even more (see the below).

Also, fyi, add ", r=margaret" to the end of your commit message
Bug 1171929 - When sharing selected text, also include the current page URL, r=margaret

We'll have you submit back to her when we finalize.

::: mobile/android/base/GeckoApp.java
@@ +629,5 @@
>          } else if ("Session:StatePurged".equals(event)) {
>              onStatePurged();
>  
>          } else if ("Share:Text".equals(event)) {
> +            String title = message.getString("title");

in general, use /final/ vars where possible ... ie:
   final String title ...

@@ +634,2 @@
>              String text = message.getString("text");
> +            String urlString = message.getString("url");

in general, just url vs urlString is ok

@@ +647,1 @@
>  

You can get the url, title, and "un-readermode'ed url" using these examples, and avoid a lot of work, by first grabbing the currently selected Tab instance, then deriving from there.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=7f0cc9a2bf79&mark=1323-1325,1341-1341#1321

This seems to let you remove all changes from SelectionHandler, and ActionBarHandler, and just do all the work here.
Attachment #8646388 - Flags: review?(markcapella) → feedback+
Attached patch bug1171929.diff (obsolete) — Splinter Review
Moved all code to GeckoApp.java. I've also noticed that often when using "Tabs.getInstance().getSelectedTab()" it is checked for null before getting information from there. Is this necessary here? If yes, what are the example scenarios that lead to tab being null, and what to do when that happens?
Attachment #8646388 - Attachment is obsolete: true
Attachment #8647092 - Flags: review?(markcapella)
Comment on attachment 8647092 [details] [diff] [review]
bug1171929.diff

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

As the method can return null, we should check for it, to guard against the page being destroyed between the time the user triggers the share message and it's receipt by Java. In that case, we'd still trigger a |text| share intent as we do today, (without the new / additional title/url information.)

Do you have access to the TRY server? We'll need to push it there for volume tests, to be safe as a next step, before we get final r+.

::: mobile/android/base/GeckoApp.java
@@ +628,5 @@
>          } else if ("Session:StatePurged".equals(event)) {
>              onStatePurged();
>  
>          } else if ("Share:Text".equals(event)) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();

use final here
Attachment #8647092 - Flags: review?(markcapella) → feedback+
I'm attatching another version of patch, everything should be fine now. As for the try servers, I don't think I have the permissions to push there (I've never done that). Could you give me some directions on how to obtain them, and which tests should I choose in TryChooser?
Attachment #8647092 - Attachment is obsolete: true
Attachment #8647256 - Flags: review?(markcapella)
Comment on attachment 8647256 [details] [diff] [review]
Bug 1171929 - When sharing selected text, also include the current page URL (version 4)

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

You can read about getting starter / L1 committer access here: https://www.mozilla.org/en-US/about/governance/policies/commit/ ... generally they want you to have a couple patches completed first, so I'll plan on pushing it for you this time.

fyi, there's an easy tool for setting the try-commit options here: http://trychooser.pub.build.mozilla.org/ ... when I push tests that affect Android primarily, I commonly use |try: -b do -p android-api-9,android-api-11 -u all -t none|

::: mobile/android/base/GeckoApp.java
@@ +637,5 @@
> +                text += "\n\n" + url;
> +                GeckoAppShell.openUriExternal(text, "text/plain", "", "", Intent.ACTION_SEND, title);
> +            } else {
> +                GeckoAppShell.openUriExternal(text, "text/plain", "", "", Intent.ACTION_SEND, "");
> +            }

Can you restructure this a bit, so that there's only one |GeckoAppShell.openUriExternal(text, ...| ?
Attachment #8647256 - Flags: review?(markcapella) → feedback+
Here's a patch with code refactored to avoid unnecessary "else" statement and openUriExternal() call
Attachment #8647256 - Attachment is obsolete: true
Attachment #8647268 - Flags: review?(markcapella)
Comment on attachment 8647268 [details] [diff] [review]
Bug 1171929 - When sharing selected text, also include the current page URL (version 5)

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

Good! Let's push it to TRY ... you can watch here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a6c5fe264d

If that goes well (it should), I'll get it to margaret for final r?, then push it to fx-team -> m-c for you. Basically, you should be done now, just watching final progress :-)

Start another bug?

::: mobile/android/base/GeckoApp.java
@@ +632,5 @@
>              String text = message.getString("text");
> +            final Tab tab = Tabs.getInstance().getSelectedTab();
> +            String title = "";
> +            if (tab != null) {
> +                title += tab.getDisplayTitle();

small nit here: s/title = tab.getDisplayTitle();
I fixed it up in your patch rather than make you revise again :D
Attachment #8647268 - Flags: review?(markcapella) → review+
Comment on attachment 8647268 [details] [diff] [review]
Bug 1171929 - When sharing selected text, also include the current page URL (version 5)

Looks good on TRY, final r? from margaret
Attachment #8647268 - Flags: review?(margaret.leibovic)
Comment on attachment 8647268 [details] [diff] [review]
Bug 1171929 - When sharing selected text, also include the current page URL (version 5)

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

Looks good to me. Thank you Pawel, and thank you Capella for mentoring!
Attachment #8647268 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/f61d28c21d80
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Verified as fixed using:
Device: LG Nexus 4 (Android 5.0)
Build: Firefox for Android 43.0a1 (2015-08-17)
Release Note Request (optional, but appreciated)
[Why is this notable]: Sounds like a nice user facing feature
[Suggested wording]:  Include current page URL when sharing selected text
[Links (documentation, blog post, etc)]:

Margaret, what do you think? Let me know if you want to improve the wording for the note.
relnote-firefox: --- → ?
Flags: needinfo?(margaret.leibovic)
I defer to Barbara, but this sounds good to me.
Flags: needinfo?(margaret.leibovic) → needinfo?(bbermes)
Is this something users can turn off? 

Not sure about having the URL in all cases, e.g. somebody copying a poem and sending it via text to their loved one :), they might not want to have the URL shown and would manually remove it.

If this can be turned off, I'm fine with it.

Proposed text by Liz OK'd by me -- not sure if we need a blog post.
One suggestion though:
Instead of "Include current page URL when sharing selected text"
say "Include URL when sharing selected text from website"
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(lhenry)
Flags: needinfo?(bbermes)
(In reply to Barbara Bermes [:barbara] from comment #26)
> Is this something users can turn off? 
> 
> Not sure about having the URL in all cases, e.g. somebody copying a poem and
> sending it via text to their loved one :), they might not want to have the
> URL shown and would manually remove it.
> 
> If this can be turned off, I'm fine with it.

Currently, no, it can't be turned off. Should we disable this feature for 43?

I personally find this very useful, since most of the time I want to be sharing a link, but I want to quote form the article as well (I believe Medium has some sort of functionality like this).
Flags: needinfo?(margaret.leibovic) → needinfo?(bbermes)
Yes, some websites use either services or their own implementation, and personally I never found this useful for the cases I copied stuff. I ended up always removing the URL, mostly though because it had some additional text (from the vendor) in there. 

Tell me again, why did we decide to do this? feature request from users?

How hard would it be to put this behind a pref? can we still get this in for 43?

I'm not saying we shouldn't be doing it, but rather putting the user in control.
Flags: needinfo?(bbermes) → needinfo?(margaret.leibovic)
(In reply to Barbara Bermes [:barbara] from comment #28)
> Yes, some websites use either services or their own implementation, and
> personally I never found this useful for the cases I copied stuff. I ended
> up always removing the URL, mostly though because it had some additional
> text (from the vendor) in there. 
> 
> Tell me again, why did we decide to do this? feature request from users?

Feature request from a user who happens to be me.

> How hard would it be to put this behind a pref? can we still get this in for
> 43?
> 
> I'm not saying we shouldn't be doing it, but rather putting the user in
> control.

I don't know that it's worth the complexity of a pref here. If you don't think this is useful for the majority of users, we should just back out this patch.
Flags: needinfo?(margaret.leibovic)
That being said, this patch also adds a Intent.EXTRA_SUBJECT parameter for these share intents to share the page title, similarly to what we do when you're sharing a page URL. So we may want to still keep that logic.
I have the same instinct about making sure it's user controllable, because users may have expectations about how copy and paste should work, copying what you see and pasting the same thing. adding a url seems super useful but like it may break a basic UI expectation. I could also imagine scenarios where you are copying and pasting info, like an address, but the URL isn't super relevant (or is even embarrassing)

It seems ok to decide whether to ship this or not during the 43 aurora cycle, it doesn't have to be today.
I'll go ahead and track this for 43 in case you all reopen it.
Flags: needinfo?(lhenry)
Sounds good Liz, I'll create an Aha card as well for product to keep an eye on
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(lhenry)
(In reply to Barbara Bermes [:barbara] from comment #33)
> Sounds good Liz, I'll create an Aha card as well for product to keep an eye
> on

That sounds fine to me. I missed the product meetings this week, but I'm fine with backing this out if we think this would be a worse experience for most users, rather than an improvement.
Flags: needinfo?(margaret.leibovic)
From what I can tell here, we are shipping this in 43. Barbara and Margaret I think it's your call here what you would like to do.
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(lhenry)
Flags: needinfo?(bbermes)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> From what I can tell here, we are shipping this in 43. Barbara and Margaret
> I think it's your call here what you would like to do.

Yes, we decided to ship this, so I don't think there's anything else to do here.
Flags: needinfo?(margaret.leibovic)
Yes, let' ship it. thanks for checking int again.
Flags: needinfo?(bbermes)
Is there any way to disable this bug? Yes, bug, because it is not a feature, it's a bug for me.

Anyway: how can I disable this?

Thanks,
Tamas
(In reply to tamas.barath from comment #38)
> Is there any way to disable this bug? Yes, bug, because it is not a feature,
> it's a bug for me.
> 
> Anyway: how can I disable this?
> 
> Thanks,
> Tamas

Thanks for reporting your issue. No, I'm sorry, we didn't implement a way to disable it.

Barbara, are we monitoring feedback about this feature? This is the first complaint I've heard, but this just hit release now, so we should pay attention to see if we want to change this.
Flags: needinfo?(bbermes)
Dear Margaret,

I don't think mozilla is going to get many feedbacks, or if they get just they do not care. Otherwise the dozens of annoying "features" of recent years has been revoked. Period.

This behaviour _can_ be useful, but in my case mostly not.
I don't speak english very well (sorry for my mistakes btw), but I read english articles on my phone. And when I find an unknown word or phrase I select it and share to Google Translate. Then I add the new word to my phrasebook, and later I make anki cards for them. This new "feature" makes it literally impossible, because I don't waste my time with deleting long urls on a touchscreen.

And you did not implement a way to disable it, this is great. Good to see how mozilla "develop" this ever great (or not, but at least useful) software.

And by the way, this is offtopic but important too. Earlier when I selected text in ff, there was share icon in the top line, I just touched it and the app list appeared. But now there is a "menu button" (three vertical dots) in this position, and when I touch it a floating "menu" appears with only one item: Share. Congratulations, really.
I thought this is the most annoying "feature" (in close competition) what the mozilla idiots can do. But now this is just the second, far behind after this new bug.

I think I will switch to chrome, I don't want spend any time with deleting urls in the next 3 or more months (if this new behaviour will ever been revoked).
Hi Tamas,

Sorry for your frustration and thanks for the feedback. Your use case describes a good argument why we should let the user decide to enable or disable this by default.

In addition, we will watch for feedback and data that will confirm your personal feedback.

Margaret, were you able to reproduce Tamas' second comment? I still see the share icon in 43 as well as on Nightly.
Flags: needinfo?(bbermes) → needinfo?(margaret.leibovic)
(In reply to tamas.barath from comment #40)

> I thought this is the most annoying "feature" (in close competition) what
> the mozilla idiots can do.

While we appreciate your feedback, this type of language is not constructive. You are free to criticize feature decisions, but do not criticize individuals.
Flags: needinfo?(margaret.leibovic)
Margaret, sorry for that.
I was and I am angry, not just about this feature, it is just a fresh one. I am angry as hell because there is a free sw with a lot of 3rd party plugins and tools, and it can be the best browser. Not the fastest, but the most usable.
But it's like the mozilla "develop" it to the totally wrong direction. Just same examples:
- metro style preferences panel, I disabled it
- removing the old preferences window, therefore I can't disable it
- bultin mp3 player, I disabled it
- and I disabled it again, because the previous about:config settings did not work with the next release
- builtin pdf viewer which is slow as hell (obviously disabled)
- firefox hello (disabled)
- pocket (disabled)
- valence plugin (which I just found when I updated my plugins, and obviusly disabled)
And maybe there is more, but this is enough. And the deadly born projects which are just wasted money, firefox os and android launcher.

Last weeks I found a printed paper about a firefox downloading milestone, about 100 million, I can't remember. It was in the 1.5 era I think. I don't print when possible but I printed it. I was a fan, but now I am just tired and angry.

I know it's a bugtracker and not a blog, but I wrote this to show why I am angry about it. I reread and rewrite my post, the original was much more offensive, but this one was off the radar. Sorry for that again.
(In reply to Barbara Bermes [:barbara] from comment #41)
> Margaret, were you able to reproduce Tamas' second comment? I still see the
> share icon in 43 as well as on Nightly.

Barbara, I recorded a video, you can find here: https://youtube.com/watch?v=7SOagS-bNfc

It is not visible, but after selecting text I touched the icon in the top right corner.
Nexus5, Android 6.0.1, FF43.
Earlier it had worked "normally", with a share icon in the top right corner. But after installing an update from play store (maybe in the summer) I found this.
The icons displayed in the ActionBar strip are added dynamically based on available options for the displayed text selection (Cut, Copy, Paste, Share, Search, etc).

As the images are added, we check for the total amount of space required to determine when the main ActionBar is "full", and subsequently add images to the overflow / dropdown menu.

I find on installing Fennec Beta 37.0 en-US on my GS3 (similar sized device) that previously we did indeed manage to fit the share image onto the main ActionBar [0], and now on nightly 46.0a1 it slips into the overflow [1].

I see a difference in the width of the leftmost "Close / checkmark" button that probably accounts for the unexpected behaviour. (Note the position of the thin vertical line to its right in the images).

This would seem correctable, perhaps filed as its own bug.

[0] https://www.dropbox.com/s/zjfru9xykwgdr8l/bug_1171929%20ActionBar%20prev.png?dl=0
[1] https://www.dropbox.com/s/fb1alix62fsm1pb/bug_1171929%20ActionBar%20now.png?dl=0
Thanks for digging in, Capella. I filed bug 1233709.
Please implement an option to disable this feature, either via the settings menu or at least via about:config. Like Tamas I use the share option to send marked text to translation programs like aard and google translate. 

Thanks!
Blocks: 1239503
Hi Margaret

Yes, It's essential for this kind of feature to implement an option to disable the URL-attachment. It is quite obvious that not all Apps can cope with a needless URL address. For example if you look up a selected term in a Dictionary App you will receive an error message. And more probably than not there will be much more involved Apps with such errors. LEO App for Android won't cope with the new Firefox feature. Please check it !

http://dict.leo.org/pages/smartphones/android/ende/manual_en.html

It's absolutly OK when you create new features for Firefox. But why did you take this for granted that it works with all Apps ? How can you release a new feature with such disastrous consequences ? Basically, a software engineer must be aware of these risks. It's wholly unacceptable for those who will get an error message in case of failure. Anyway, at least an option to disable the URL-attachement is a must-have. Sorry, but frankly speaking otherwise this new feature will be a botch job.

So please review the bug again

Thank you

Marcel
Flags: needinfo?(margaret.leibovic)
Hi Marcel,

please calm down and have a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1239503

Margaret has realized that the premature implementation of this feature without an option to disable it was a mistake. It will be reverted in 45+. For 44 it was to late.

Thanks,
Ralf
Flags: needinfo?(margaret.leibovic)
You need to log in before you can comment on or make changes to this bug.