Closed Bug 947321 Opened 7 years ago Closed 7 years ago

Inconsistencies in action-bar overflow menu for selected text

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 + fixed
firefox29 --- fixed

People

(Reporter: aaronmt, Assigned: wesj)

References

()

Details

(Keywords: regression, reproducible, testcase)

Attachments

(2 files)

See screenshot; we overflow a menu in selected text in inputs but not on raw text. I couldn't gather from the discussion on IRC if this was intentional or not.

--
Nightly (12.06) | LG Nexus 4 (Android 4.4)
Flags: needinfo?(ibarlow)
This was not as the design intended, no. Wes said that there are certain functions that ask to be put in the menu somehow, which might be why this is happening? It's not really clear to me though why they want to be put in the menu for some cases but not others. Wes, any idea?
Flags: needinfo?(ibarlow)
I think I need a better spec for this before I move forward. From http://cl.ly/image/001d1L1l3B3q I assumed at first that we didn't want to show Share or Search for for text inputs, but I think we talked and I took the final decision as "Show them, but not in the primary toolbar ui".

Now I think you're saying "Show them in the main toolbar UI if there is room, otherwise, show them in overflow". Is the correct?

Doing that will require a little code to ensure order in the list. Shouldn't be hard.
(In reply to Wesley Johnston (:wesj) from comment #2)

> Now I think you're saying "Show them in the main toolbar UI if there is
> room, otherwise, show them in overflow". Is the correct?

Yes. Sorry for the confusion :/
Attached patch PatchSplinter Review
This sorts the items using "order" which is also the key android uses for sorting menu items [1]. All of our actions now ask to be shown as action bar buttons, so I just removed those handlers.

Android actually supports a few values there. "always", "ifRoom", and "never". It would be nice to support those, but its a pretty large change in our current menu system to do so. (i.e. the current system adds items one at a time in on onItemChanged() and has no idea if more items are in the pipes or not. Android's runs through the entire list every time items are changed, and so can prioritize always over ifRoom).

[1] http://developer.android.com/reference/android/view/Menu.html#add%28int,%20int,%20int,%20int%29
Attachment #8344944 - Flags: review?(margaret.leibovic)
Comment on attachment 8344944 [details] [diff] [review]
Patch

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

LGTM. Do we have docs somewhere about how to add new contextual actions? Should we make a new MDN page?
Attachment #8344944 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8344944 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 768667
User impact if declined: Items show up in overflow menu when there's plenty of room in the action bar
Testing completed (on m-c, etc.): landed on mc today.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Just sorts this list.
String or IDL/UUID changes made by this patch: None
Attachment #8344944 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8f25ce2772b6
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Attachment #8344944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Nightly 29.0a1 (2013-12-26)
Galaxy Tab (Android 4.0.3 )

I do get consistency in all instances for the selection of the text. 

Settings this to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.