Closed Bug 730941 Opened 12 years ago Closed 8 years ago

image context menu needs "view image"

Categories

(Firefox for Android Graveyard :: General, enhancement)

ARM
Android
enhancement
Not set
normal

Tracking

(firefox48 verified, blocking-fennec1.0 -)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified
blocking-fennec1.0 --- -

People

(Reporter: dietrich, Assigned: daleharvey, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(2 files, 10 obsolete files)

58.12 KB, image/png
Details
9.86 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Often while reading articles online with Fennec i would like to view an image, since the image embedded in the article is too small.

however, the image context menu only has an option to save, not to view.

it *does* show the URL at the top of the context menu, so maybe instead of a new menu item, we could just make that image URL a link?

but it'd be more visually explicit to provide a menu item that says "view image".
Severity: normal → enhancement
blocking-fennec1.0: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
blocking-fennec1.0: ? → -
Still missing, although we he have Copy Image Location now
Hi, I'm new over here and would like to take this up. I've been working at it for a few hours now and have a patch file containing code changes and a test case (giving final touches and testing some more). Please let me know if I can take this bug. Thanks.
Welcome Anuj. This bug is all yours. Feel free to ask more questions and to attach a patch when you're ready.
Assignee: nobody → sahai.anuj16
Status: NEW → ASSIGNED
Hey Aaron, thanks a lot. Just one question, when I'm done, should I attach two patches, one for code changes and one for a test case, or just a single patch?
We originally had some qualms about adding too many menu items, but now that the context menus are split, I don't think this is as much of an issue. I just want to double-check with Ian what the expected UX should be.
Flags: needinfo?(ibarlow)
A separate patch is ideal.
(In reply to Michael Comella (:mcomella) from comment #6)
> We originally had some qualms about adding too many menu items, but now that
> the context menus are split, I don't think this is as much of an issue. I
> just want to double-check with Ian what the expected UX should be.

Thanks for asking. This is fine, I'd like to have this option too :)
Flags: needinfo?(ibarlow)
Looks like the same patch attached twice by accident.
Attachment #8406316 - Attachment is obsolete: true
Yeah, sorry about that. Exporting with mercurial to a file using -o flag kept appending the changeset to the patch file, instead of replacing. Reattached the correct one.
Comment on attachment 8406326 [details] [diff] [review]
Added "View Image" to context menu

># HG changeset patch
># User Anuj Sahai <sahai.anuj16@gmail.com>
># Date 1397501747 -19800
># Node ID 5a43b72ca6bee42486242832de26540bedbba73e
># Parent  d8c1b10c3a3dc57e9fa9ff47db13950a59befc64
>Bug 730941: Image context menu needs "view image"
>
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>@@ -650,16 +650,24 @@ var BrowserApp = {
>       });
>   
>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.unmute"),
>       NativeWindow.contextmenus.mediaContext("media-muted"),
>       function(aTarget) {
>         aTarget.muted = false;
>       });
> 
>+    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.viewImage"),
>+      NativeWindow.contextmenus.imageLocationCopyableContext,
>+      function(aTarget) {
>+        let url = aTarget.src;
>+        ContentAreaUtils.urlSecurityCheck(url, aTarget.ownerDocument.nodePrincipal);
>+        BrowserApp.loadURI(url);
>+      });
>+
>     NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.copyImageLocation"),
>       NativeWindow.contextmenus.imageLocationCopyableContext,
>       function(aTarget) {
>         let url = aTarget.src;
>         NativeWindow.contextmenus._copyStringToDefaultClipboard(url);
>       });
> 
>     NativeWindow.contextmenus.add({
>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties
>--- a/mobile/android/locales/en-US/chrome/browser.properties
>+++ b/mobile/android/locales/en-US/chrome/browser.properties
>@@ -169,16 +169,17 @@ contextmenu.copyLink=Copy Link
> contextmenu.shareLink=Share Link
> contextmenu.bookmarkLink=Bookmark Link
> contextmenu.copyEmailAddress=Copy Email Address
> contextmenu.shareEmailAddress=Share Email Address
> contextmenu.copyPhoneNumber=Copy Phone Number
> contextmenu.sharePhoneNumber=Share Phone Number
> contextmenu.changeInputMethod=Select Input Method
> contextmenu.fullScreen=Full Screen
>+contextmenu.viewImage=View Image
> contextmenu.copyImageLocation=Copy Image Location
> contextmenu.shareImage=Share Image
> # LOCALIZATION NOTE (contextmenu.search):
> # The label of the contextmenu item which allows you to search with your default search engine for
> # the text you have selected. %S is the name of the search engine. For example, "Google".
> contextmenu.search=%S Search
> contextmenu.saveImage=Save Image
> contextmenu.setImageAs=Set Image As
Anuj, you can use `hg blame <filename>` to find an appropriate reviewer for your code by seeing who has changed related code most recently (and in largest quantity - use your judgement). Once you find someone you think may be appropriate, feel free to flag them for review (click details next your patch, select "?" from review, and enter their email address; Bugzilla will typically auto-complete IRC nicks as well, if you start with a ":").
Fixed indentation.
Attachment #8406326 - Attachment is obsolete: true
Reordered some lines, to keep hg blame clean.
Attachment #8406318 - Attachment is obsolete: true
Attachment #8406781 - Flags: review?(wjohnston)
Attachment #8406779 - Flags: review?(wjohnston)
Comment on attachment 8406779 [details] [diff] [review]
Added "View Image" to context menu [v2, Fixed indentation]

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

::: mobile/android/chrome/content/browser.js
@@ +658,5 @@
> +    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.viewImage"),
> +      NativeWindow.contextmenus.imageLocationCopyableContext,
> +      function(aTarget) {
> +        let url = aTarget.src;
> +        ContentAreaUtils.urlSecurityCheck(url, aTarget.ownerDocument.nodePrincipal);

I wonder if we should pass flags like Desktop does:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#976

lets try to match them if we can.
Attachment #8406779 - Flags: review?(wjohnston) → review+
Comment on attachment 8406781 [details] [diff] [review]
Test Case for "View Image" context menu item [v2, Reduced changes, to keep hg blame clean]

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

This looks really nice. Thanks :) I'm sending it to mcomella as well, because our testing infrastructure has changed a lot. Unfortunately, we haven't updated some of the tests like this one yet. We may want to do that in a separate bug, but I'd like his signoff.
Attachment #8406781 - Flags: review?(wjohnston)
Attachment #8406781 - Flags: review?(michael.l.comella)
Attachment #8406781 - Flags: review+
Comment on attachment 8406781 [details] [diff] [review]
Test Case for "View Image" context menu item [v2, Reduced changes, to keep hg blame clean]

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

(In reply to Wesley Johnston (:wesj) from comment #20)
> Unfortunately, we haven't updated some of the tests like this
> one yet. We may want to do that in a separate bug, but I'd
> like his signoff.

While I never drew a consensus with everyone else, I was thinking we write all the new tests in the new framework, and only merge the tests over when they require significant upgrades/rewriting - why waste time fixing what isn't broken? :)

So this patch works for me! Thanks Anuj!

::: mobile/android/base/tests/ContentContextMenuTest.java
@@ +7,5 @@
>  
>  import android.util.DisplayMetrics;
>  
> +import com.jayway.android.robotium.solo.Condition;
> +

nit: Generally we try to avoid excess whitespace and I don't see any reason to add this line here. However, it's probably not worth the extra cycle to fix it in this patch.

::: mobile/android/base/tests/testPictureLinkContextMenu.java
@@ +1,4 @@
>  package org.mozilla.gecko.tests;
>  
>  
> +

nit: Again, unnecessary whitespace.
Attachment #8406781 - Flags: review?(michael.l.comella) → review+
Attachment #8406779 - Attachment is obsolete: true
Attachment #8407603 - Flags: review?(wjohnston)
Attachment #8406781 - Attachment is obsolete: true
Attachment #8407604 - Flags: review?(michael.l.comella)
Comment on attachment 8407604 [details] [diff] [review]
Test Case for "View Image" context menu item [v3, Removed unnecessary whitespace]

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

Nice, thank you!
Attachment #8407604 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8407603 [details] [diff] [review]
Added "View Image" to context menu [v3, Pass flag to urlSecurityCheck]

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

Thanks!
Attachment #8407603 - Flags: review?(wjohnston) → review+
Keywords: checkin-needed
Backed out for robocop failures. Please make sure this is green on Try before requesting checkin next time.
https://hg.mozilla.org/integration/fx-team/rev/d26a2463dc6b

https://tbpl.mozilla.org/php/getParsedLog.php?id=38413383&tree=Fx-Team
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Attached file Robocop Test Results (obsolete) —
Hi Ryan, I had tested it a number of times and all of the robocop tests had passed. I've retested this again around 10 times now (Galaxy S Vibrant Android 4.2 and Nexus 7 2012 Android 4.4) and got a pass every time. Not sure how to proceed? Can someone please be kind enough to test this patch?

Please find attached the test log.
Attachment #8417070 - Flags: feedback?(ryanvm)
You might just need to add a switchTabs(imageTitle); line before your test. Since we show the lists in tabs, now, you need to switch to the image tab before you look for iamge menu items. That sort of error should be reproducible locally though...
Comment on attachment 8417070 [details]
Robocop Test Results

Local testing is good, but ultimately it needs to pass in automation as well. If you don't have Level 1 commit access, you should consider requesting it so you can push to Try.
http://www.mozilla.org/hacking/commit-access-policy/
Attachment #8417070 - Flags: feedback?(ryanvm)
Almost a year passed since the last comment - what is the current status of this bug?
Wes, Comella ^
Flags: needinfo?(wjohnston)
Flags: needinfo?(michael.l.comella)
(In reply to MaximYanchenko from comment #32)
> Almost a year passed since the last comment - what is the current status of
> this bug?

We had a few test failures that prevented this from landing, however, I think it could be a good time to re-evaluate whether we really want this feature or not considering it can be worked around.

Anthony, do you think it'd be useful to have a "View image" context menu item when long-pressing on an image? Ian was fine with it:

(In reply to Ian Barlow (:ibarlow) from comment #8)
> (In reply to Michael Comella (:mcomella) from comment #6)
> > We originally had some qualms about adding too many menu items, but now that
> > the context menus are split, I don't think this is as much of an issue. I
> > just want to double-check with Ian what the expected UX should be.
> 
> Thanks for asking. This is fine, I'd like to have this option too :)
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
Yeah I'm fine with this also.
Flags: needinfo?(alam)
Unassigning due to inactivity.

(In reply to MaximYanchenko from comment #32)
> Almost a year passed since the last comment - what is the current status of
> this bug?

To be explicit, seems like this just needs a push over the finish line. There are some test failures but we introduced a test with this patch so I wonder if it wasn't just that test that was failing.
Assignee: sahai.anuj16 → nobody
Mentor: wjohnston, michael.l.comella
Status: ASSIGNED → NEW
Flags: needinfo?(wjohnston)
Whiteboard: [lang=java]
Any chance to get it finally done?
(In reply to MaximYanchenko from comment #37)
> Any chance to get it finally done?

Sorry, no one is actively working on this – there are higher priority bugs at the moment.
Ill take a look at this
Assignee: nobody → dale
Rebased patch, will ensure try is nice before flagging review
Attachment #8407603 - Attachment is obsolete: true
Attachment #8407604 - Attachment is obsolete: true
Attachment #8417070 - Attachment is obsolete: true
Hi Michael, having some problem with this I was wondering if you could help.

On the try run I am seeing |testPictureLinkContextMenu| fail intermittently (so far 50%). It passes for me locally 100%. 

The new test clicks the 'view image' menu, navigates to a new page, once it has verified viewing the image works then it presses back, when it returns the next test should show the context menu and verify the context menu contents as it did before.

I am not sure if the page navigation is interfering with the test, I know that we had focus issues with that on fxos but not familiar with how robocop works.
Flags: needinfo?(michael.l.comella)
Just an update on the pass rate, its actually 8/10 passing, trying to figure out how stable the tests are currently
So the verifyShare test does a waitForText(pageTitle) after goBack() to ensure the page is loaded so will give that a shot, pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=768c44d028dd
Ok the 'waitForText(pageTitle)' looks to have fixed the robocop intermittents, theres 1 failure there out of 11 runs and its a test I didnt touch / shouldnt effect. I am not absolutely certain about the mda1/2 failures but they seem unrelated / existing.

So I think this is good for review :) (flagged Nick instead of Michael since Michael had like 15 in queue)
Attachment #8728440 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attachment #8728848 - Flags: review?(nalexander)
Comment on attachment 8728848 [details] [diff] [review]
Add 'View Image' to image context menu

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

One real concern and a question about the test implementation.  In general, a product question -- "View Image" or "View Image in New Tab".  (Or both?)

::: mobile/android/chrome/content/browser.js
@@ +901,5 @@
>  
> +    NativeWindow.contextmenus.add(stringGetter("contextmenu.viewImage"),
> +      NativeWindow.contextmenus.imageLocationCopyableContext,
> +      function(aTarget) {
> +        let url = aTarget.src;

We definitely want to add UITelemetry -- I suggest "web_view_image".  In fact, it looks like we want:

        UITelemetry.addEvent("action.1", "contextmenu", null, "web_view_image");
        UITelemetry.addEvent("loadurl.1", "contextmenu", null);

-- the latter because we navigate.

@@ +904,5 @@
> +      function(aTarget) {
> +        let url = aTarget.src;
> +        ContentAreaUtils.urlSecurityCheck(url, aTarget.ownerDocument.nodePrincipal,
> +                                          Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
> +        BrowserApp.loadURI(url);

`BrowserApp.selectedBrowser.loadURI`...?

Consider if this should be loading in a new tab, with a snackbar.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/ContentContextMenuTest.java
@@ +103,5 @@
>          mSolo.goBack();
>          waitForText(pageTitle);
>      }
>  
> +  protected void verifyViewImageOption(String viewImageOption, final String imageUrl, String pageTitle) {

Indentation looks odd here.  Make sure all tabs are replaced with four spaces.

@@ +108,5 @@
> +        if (!mSolo.searchText(viewImageOption)) {
> +          openWebContentContextMenu(viewImageOption);
> +        }
> +        mSolo.clickOnText(viewImageOption);
> +        boolean viewedImage = waitForCondition(new Condition() {

I see a `BaseTest.verifyUrl`.  Is it not equivalent?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPictureLinkContextMenu.java
@@ +13,2 @@
>      private static final String tabs [] = { "Image", "Link" };
> +    private static final String photoMenuItems [] = { "Copy Image Location", "Share Image", "View Image", "Set Image As", "Save Image" };

Not germane to your patch, but: all these string copy-pastes aren't helping anything.  We should refer to `R.string....` to ease finding usages; and we should just refer to any static strings in `StringHelper` directly.  mcomella probably has an opinion on this, and perhaps I will try to align with him sometime.
Attachment #8728848 - Flags: review?(nalexander) → feedback+
> One real concern and a question about the test implementation.  
> In general, a product question -- "View Image" or "View Image in New Tab".  (Or both?)

"View Image" is consistent with Firefox Desktop and Chrome, I can see possible reasons for "View Image in New Tab" but there are questions about consistency. Barbara you or anyone from UX want to make a call on this?

The rest of the review issues I will address, thanks
Flags: needinfo?(bbermes)
> I see a `BaseTest.verifyUrl`.  Is it not equivalent?

verifyUrl doesnt wait for the url to change (only waiting in the case the url is null) this assertion seems like an anti pattern for selenium type tests but definitely breaks this test
> Not germane to your patch, but: all these string copy-pastes aren't helping anything.  We 
> should refer to `R.string....` to ease finding usages; and we should just refer to any 
> static strings in `StringHelper` directly.  mcomella probably has an opinion on this, 
> and perhaps I will try to align with him sometime.

Cool, somewhat makes sense but with my unfamiliarity with Java / our code base I have avoided refactoring it for now until I know what it is supposed to look like in the end, if you want the refactoring done along with this patch it would be very helpful to point to some example code of how it should be done. Otherwise happy to do it in a seperate bug.
Added telemetry, fixed indentation and switched to |BrowserApp.selectedBrowser.loadURI| + theres a few comments above about the other review points.

Flagging review assuming that 'View Image' is the implementation we want.
Attachment #8728848 - Attachment is obsolete: true
Attachment #8730321 - Flags: review?(nalexander)
Comment on attachment 8730321 [details] [diff] [review]
Add "View Image" item to context menu

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

lgtm.
Attachment #8730321 - Flags: review?(nalexander) → review+
Awesome, pushed to try just to make sure - https://treeherder.mozilla.org/#/jobs?repo=try&revision=10fa24e5438c
(In reply to Dale Harvey (:daleharvey) from comment #47)
> > One real concern and a question about the test implementation.  
> > In general, a product question -- "View Image" or "View Image in New Tab".  (Or both?)
> 
> "View Image" is consistent with Firefox Desktop and Chrome, I can see
> possible reasons for "View Image in New Tab" but there are questions about
> consistency. Barbara you or anyone from UX want to make a call on this?
> 
> The rest of the review issues I will address, thanks

Dale, thanks for flagging this. I'm fine either or, since we are a mobile app in a world of tabs, I'd go with "View Image in New Tab". Anthony, thoughts?
Flags: needinfo?(bbermes) → needinfo?(alam)
Let's just keep it simple - 

"View Image" :)
Flags: needinfo?(alam)
https://hg.mozilla.org/mozilla-central/rev/d2ce28624461
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
"View Image" option appears on an image context menu, after long tapping on it. It displays the image in the same tab.
Verified as fixed using:
Device: One A2001 (Android 5.1.1)
Build: Firefox for Android 48.0a1 (2016-03-20)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.