ViewPager with Fragments of ListViews do not respect ContextMenus correctly

[ChrisSmith]

Sometimes when using ContextMenus, onContextItemSelected is called for the wrong Fragment. onCreateContextMenu is always correct but the ViewPager is causing all ListFragments to receive the callback. If the Fragments have unique MenuItem ids then they should act normally, but if two Fragments in the ViewPager are the same class (or subclass of each other) they will interfere with each other.

WIll post code later today demonstrating the issue.

[ChrisSmith]

Basic modification to the FragmentPagerSupport demo.

Scrolling through the pages and selecting the context menu items you'll see more than one fragment receive the event.

One possible solution is have the fragment check the current page and then return false if its not the currently visible, but ideally the FragmentManager could handle this.

Update:

This appears to be an issue not with ViewPager but with how FragmentActivity handles context menus, each Fragment receives a notification if its visible, regardless if it 'owns' the menu. I haven't found it documented anywhere but I'm 90% sure this is the expected behavior when using Fragments with ContextMenus. So I don't think this is a bug, its just annoying.

        @Override
        public void onActivityCreated(Bundle savedInstanceState) {
            super.onActivityCreated(savedInstanceState);
            setListAdapter(new ArrayAdapter<String>(getActivity(),
                    android.R.layout.simple_list_item_1, Cheeses.sCheeseStrings));

            Log.d(TAG,this.toString()+" - registerForContextMenu");
            registerForContextMenu(getListView());
        }

        @Override
        public void onCreateContextMenu(ContextMenu menu, View v, ContextMenuInfo menuInfo){    
            super.onCreateContextMenu(menu, v, menuInfo);

            Log.d(TAG,this.toString()+" - onCreateContextMenu");

            menu.add(Menu.NONE, 1, Menu.NONE, "Context MenuItem");
        }

        @Override
        public boolean onContextItemSelected(MenuItem item) {
            Log.d(TAG,this.toString()+" - onContextItemSelected");

            return super.onContextItemSelected(item);
        }

[JakeWharton]

Thanks for the code. I will test this with ActionBarSherlock fragments, compat lib fragments, and native fragments on 3.0+ just to make sure they all behave the same, even if that sameness is flawed.

[JakeWharton]

Pushed a fix to the dev branch which is thankfully trivial due to the fix for #48. I didn't test it yet but the logic is sound.

[ChrisSmith]

Found one small bug inside FragmentPagerAdapter, when destroyItem is called you unconditionally set mViewPagerParticipant = false but in instantiateItem it is conditional. Issues arrise when the item is then attached instead of added because mViewPagerParticipant is still false, causing the MenuItems not to work properly.

Fix:

 if (fragment != null) {
            fragment.mViewPagerParticipant = true;
            if (DEBUG) Log.v(TAG, "Attaching item #" + position + ": f=" + fragment);
            mCurTransaction.attach(fragment);
        } 

[virajmody]

Argh, I just got bitten by this. Spent several hours before looking here. Just to confirm, this isn't fixed in the current code, right?

[JakeWharton]

No. The fix exists outside the scope of the library for now. Depending on what Google does in the r4 compatibility library then perhaps this will be fixed natively.

There's a simple workaround I can think of, though:

Extend FragmentPagerAdapter and override onItemSelected. Copy the functionality of holding on to the last selected fragment in a WeakReference and toggling a boolean of your choice (e.g., mIsSelected). Then, in the context menu callback, just check to make sure mIsSelected is true before handling the event.

Since your fragments will likely be different classes, implement that property in a custom base class that extends from Fragment, or by applying an interface (e.g., HasViewPagerState) with a single method setIsSelected(boolean) to all your fragments.

[virajmody]

Maybe I don't understand this well enough, but won't it be easier/better to set the OnPageChangeListener on the PageIndicator and then toggle the setHasContextMenu value of the underlying Fragment?

If you're tried this already and know it won't work, I would appreciate feedback. If not, I'll give it a shot and let you know if it works.

[JakeWharton]

You can do that, but you have to be careful because the fragment actually does have a context method. You might mess with it registering properly unintentionally. There's also no deselected callback to turn that option back on so you'd still be holding on to the previous fragment in a WeakReference somewhere.

On second though, our final solution for the options menu items leaking from fragments was abstract enough potentially apply to fixing this as well.

@ChrisSmith: What do you think? The final implementation was the toggling of an internal mExposesMenu boolean. Should this apply to all kinds of menus to cover this bug or only be for the options menu?

It's difficult because we are going to pure speculation as to what the official Google API will be in the r4 release.

[virajmody]

Yes, I see that there is no deselected callback, which is a pain. It's still possible to cycle through all Fragments and set the setHasContextMenu to false, and then re-set the current Fragment's value to true. (This is assuming all fragments have/deal with context menu callbacks).

Related question: I'm using the currently released Google version of the source, but looking at the fixes you've made I think I should switch over to using yours. Apart from the regular disclaimers about being on-my-own if/when Google update their version, are there any other gotchas to using the source your have here?

[JakeWharton]

Despite being a library project rather than a .jar this library should be a near drop-in replacement for the official compatibility library.

You will probably have to change some menu-related imports since I mirror all of them in the android.support.v4.view package to provide action bar-related stuff on pre-3.0. You might have to add a cast on the getActivity() calls in your fragments because of the stupid, stupid, stupid support for map activities and the limitations of Java.

But other than than it should function exactly the same. It will only wrap the action bar if you use Theme.Sherlock or Theme.Sherlock.Light.

[virajmody]

@JakeWharton you might be interested in this: http://code.google.com/p/android/issues/detail?id=20065

[ChrisSmith]

@JakeWharton I'm not really sure how to approach this right now since Diane has said they are going to work on a fix (although I feel it will be similar to what you've already implemented).

Toggling mExposesMenu should work for all cases that I can think of. What we are trying to accomplish is essentially hiding the Fragment despite it being "visible" onscreen.

For other reasons I've still added two methods to OnPageChangeListener that enable me to have more control over when the Fragments are shown and hidden. This is useful because the current Fragment lifecycle doesn't work well with ViewPager