Closed Bug 993407 Opened 6 years ago Closed 6 years ago

Navigation should be primary tab on image link dialogs

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox30 + verified
firefox31 + verified
fennec 30+ ---

People

(Reporter: pwd.mozilla, Assigned: wesj)

References

Details

(Keywords: uiwanted)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 942270
OS: Linux → Android
Hardware: x86 → ARM
Yeah it bothers me when menus change order.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Summary: Swap Dialog Tabs on Image Link → Swap Dialog Tabs on any shareable link
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.
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.
(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.
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.
(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?
Ian - Thoughts on the order? Making "LINK" the leftmost and the default?
Flags: needinfo?(ibarlow)
Keywords: uiwanted
(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.
Flags: needinfo?(ibarlow)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8405579 - Flags: review?(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
Attachment #8405579 - Flags: review?(bnicholson) → review+
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.
Attachment #8405579 - Flags: approval-mozilla-aurora?
This (expectedly) broke some tests that depend on this menu ordering. Fixing...
Duplicate of this bug: 995815
Status: NEW → ASSIGNED
Summary: Swap Dialog Tabs on any shareable link → Navigation should be primary tab on image link dialogs
Attached patch Patch v2Splinter Review
Fixed up tests. Try run at
https://tbpl.mozilla.org/?tree=Try&rev=7beac26e3329
Attachment #8405579 - Attachment is obsolete: true
Attachment #8405579 - Flags: approval-mozilla-aurora?
Attachment #8407668 - Flags: review?(bnicholson)
Attachment #8407668 - Flags: review?(bnicholson) → review+
Grr. I used the wrong bug number. Landed on central:
https://hg.mozilla.org/mozilla-central/rev/a5cee95d219e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: nobody → wjohnston
tracking-fennec: ? → 30+
needinfo to get this on aurora.
Flags: needinfo?(wjohnston)
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.
Attachment #8407668 - Flags: approval-mozilla-aurora?
"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)
Target Milestone: --- → Firefox 31
Attachment #8407668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(wjohnston)
Based on comment21 I will change the status 31 from fixed to verified.
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.