993407 - Navigation should be primary tab on image link dialogs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140408060700

Steps to reproduce:

Navigation (Open...) is the secondary tab, these should be in the primary tab while the actions (Copy, Save, Set...) should be in the secondary tab. This would follow the behaviour of Desktop whereby Navigation is at the top of the menu and the actions are lower down.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Yeah it bothers me when menus change order.

Comment 2

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

I am having trouble understanding the issue. Pictures? More explanation?

I assume this has to do with having "Share" at the top of the context menu. This was done for a reason. The context menu supports Quick Share now and the multiple button-like images look very bad in the middle of a menu.

Comment 3

[Wesley Johnston (:wesj)]

I think this is the two tabs on image links: "Image" and "Link". The suggestion is that the "Link" one should be first (and default). "Image" should be second.

Comment 4

[Paul [pwd]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> I think this is the two tabs on image links: "Image" and "Link". The
> suggestion is that the "Link" one should be first (and default). "Image"
> should be second.

Indeed.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

But if I never share this stays as a menu item that looks just like every other menu item which I dislike because it takes more effort to find new tab. I love the look of the menu once I have a share preference because then it is visibly different than the new tab menu item.

Comment 6

[Paul [pwd]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> But if I never share this stays as a menu item that looks just like every
> other menu item which I dislike because it takes more effort to find new
> tab. I love the look of the menu once I have a share preference because then
> it is visibly different than the new tab menu item.

Are you suggesting that the quick share should be populated by default?

Comment 7

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

Ian - Thoughts on the order? Making "LINK" the leftmost and the default?

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Ian - Thoughts on the order? Making "LINK" the leftmost and the default?

Works for me. I don't really think "desktop menu order consistency" should drive our decision making process, but I have no problem making "link" the default and left tab here.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch

This sorts the keys before we build our tabs list. All the sorting does is move the "link" tab to the front. Everything else is considered equal.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8405579 [details] [diff] [review]
Patch

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

Nice, LGTM with nits fixed.

::: mobile/android/chrome/content/browser.js
@@ +8410,5 @@
>            if (!elt) {
>              return;
>            }
>  
>            var items = NativeWindow.contextmenus._getHTMLContextMenuItemsForMenu(elt, target);

Nit: s/var/let/ while here

@@ +8411,5 @@
>              return;
>            }
>  
>            var items = NativeWindow.contextmenus._getHTMLContextMenuItemsForMenu(elt, target);
> +          // This menu will always only have one context, but we still make sure its the "right" one.

Nit: New line above comment

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/4c16d08e358b

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8405579 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Uplifting quick share in context menus (bug 942270)
User impact if declined: Default options people want aren't visible right away.
Testing completed (on m-c, etc.): Landed on mc today. 
Risk to taking this patch (and alternatives if risky): This is low risk. Just changing the ordering of an array. We could just not ship this I guess, and fix it in 31+.  
String or IDL/UUID changes made by this patch: none.

Comment 13

[Wesley Johnston (:wesj)]

This (expectedly) broke some tests that depend on this menu ordering. Fixing...

Comment 14

[Wesley Johnston (:wesj)]

also backout: https://hg.mozilla.org/integration/fx-team/rev/296776ed5d4b

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Fixed up tests. Try run at
https://tbpl.mozilla.org/?tree=Try&rev=7beac26e3329

Comment 17

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/a5cee95d219e

Comment 18

[Wesley Johnston (:wesj)]

Grr. I used the wrong bug number. Landed on central:
https://hg.mozilla.org/mozilla-central/rev/a5cee95d219e

Comment 19

[:Margaret Leibovic]

needinfo to get this on aurora.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 8407668 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 942270
User impact if declined: harder to find common options in context menus (i.e. Open in new tab)
Testing completed (on m-c, etc.): Landed yesterday.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Just reordering an array.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Teodora Vermesan (:TeoVermesan)]

"Link" is the first one displayed and "Image" is the second.
Verified fixed on:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 31.0a1 (2014-04-18)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08116c1e9a26

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Based on comment21 I will change the status 31 from fixed to verified.

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Link" is the first one displayed and "Image" is the second on latest Aurora so:
Verified fixed on:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 30.0a2 (2014-04-23)