Closed Bug 1013419 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

We don't get notified in the prompts code when a quickshare option is picked, so we can't hide the menu there.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8425611 - Flags: review?(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
Attachment #8425611 - Flags: review?(bnicholson) → review-
Attached patch Patch v2Splinter Review
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!
Attachment #8425611 - Attachment is obsolete: true
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.
Attachment #8426455 - Flags: review+
Status: NEW → ASSIGNED
Duplicate of this bug: 1016375
Landed. Filing a follow up for the convertView thing:

https://hg.mozilla.org/integration/fx-team/rev/c843306fe76f
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
Attachment #8426455 - Flags: approval-mozilla-beta?
Attachment #8426455 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c843306fe76f
Assignee: nobody → wjohnston
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
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.
Attachment #8426455 - Flags: approval-mozilla-beta?
Attachment #8426455 - Flags: approval-mozilla-beta+
Attachment #8426455 - Flags: approval-mozilla-aurora?
Attachment #8426455 - Flags: approval-mozilla-aurora+
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)
Verified as fixed in build 30 beta 10;
Device: Google Nexus 5 (Android 4.4.2).
You need to log in before you can comment on or make changes to this bug.