1013419 - Context menu doesn't hide when selecting a quick share option

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We don't get notified in the prompts code when a quickshare option is picked, so we can't hide the menu there.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This alters the MenuItemActionView to handle multiple listeners. That allows the GeckoActionProvider to know when one is clicked (so that it can share using the intent as well as update the share history), and the prompt code to know when one is clicked.

Comment 2

[Brian Nicholson (:bnicholson)]

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

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

updateActionView is called in getView [1], meaning we could be calling this on recycled views that have already had updateActionView called on them. In turn, that will cause listener.onItemClick to be fired multiple times for the same item, which will probably result in weird behavior. Is there any way that you can add this listener in getActionView instead of updateActionView so that it only gets added once?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptListAdapter.java#247

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Yes. Yes I can move it there.

These views are generally screwed if there's ever actually two around and we recycle them. Right now we've only ever got one though, so the system works!

Comment 4

[Brian Nicholson (:bnicholson)]

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

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

Thanks, LGTM.

(In reply to Wesley Johnston (:wesj) from comment #3)
> These views are generally screwed if there's ever actually two around and we
> recycle them. Right now we've only ever got one though, so the system works!

If that's still true, can we enforce this by throwing an exception in getView if convertView != null? Might be nice to avoid a footgun that could result in difficult-to-find bugs if this ever changes.

Comment 6

[Wesley Johnston (:wesj)]

Landed. Filing a follow up for the convertView thing:

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

Comment 7

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 942270
User impact if declined: Dialog has to be closed separately from clicking the share button in the dialog. i.e. User has to hit back explicitly.
Testing completed (on m-c, etc.): Landed on mc today.
Risk to taking this patch (and alternatives if risky): No other good fixes. We could let this ride. Its a minor annoyance that you won't see all the time, but its fairly visible when it does happen.
String or IDL/UUID changes made by this patch: none

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c843306fe76f

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Since this looks pretty isolated to the use case in the bug, we'll take the uplift and avoid shipping this regression in the first place.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4bd705a54a2
https://hg.mozilla.org/releases/mozilla-beta/rev/d24783ed4ab5

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Context menu is dismissed when tapping a quickshare option, so:
Verified fixed on:
Device: Alcatel One Touch (Android 4.1.2)
Builds: Firefox for Android 32.0a1 and Firefox for Android 31.0a2 (2014-05-29)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d24783ed4ab5

Comment 13

[Flaviu Cos]

Verified as fixed in build 30 beta 10;
Device: Google Nexus 5 (Android 4.4.2).