1020738 - Normal selected tab stays selected after opening a private tab

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Open a new private browsing tab
2) Look at normal tabs panel.

The normal tabs panel still has a tab selected even though the selected tab is the private tab. Our implementation only allows one tab to be selected at a time, so the selected tab in the normal tray is bogus.

Comment 1

[:Margaret Leibovic]

This might be an issue because TabsTray was originally built before we had private tabs, so it might make an assumption about there always being a selected tab in a tray.

In updateSelectedPosition, we don't actually change any selection styles if there's no selected tab:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsTray.java#205

Comment 2

[Taha Lukmanji]

I don't mind fixing this bug. Need a bit of guidance.

Taha

Comment 3

[Taha Lukmanji]

I don't mind fixing this case... Brian, do you want to assign this case to me?

Comment 4

[Brian Nicholson (:bnicholson)]

Hi Taha, thanks for taking this!

Here's my first suggestion to try:
1) Call getSelectedTab() like we do at [1], then call isPrivate() to determine whether the selected tab is private.
2) Compare this result to mIsPrivate in TabsTray.
3) If they are not equal, set 'selected' to INVALID_POSITION (a constant defined in AdapterView).

That way, if the selected tab is a private tab, the normal tabs list will have an unset position -- and vice versa. Let me know if you have any questions!

[1] http://hg.mozilla.org/mozilla-central/file/31de1a84b27f/mobile/android/base/tabspanel/TabsTray.java#l212

Comment 5

[Taha Lukmanji]

changed method updateSelectedPosition() according to the description above but seems like the bug is still there or may be I am understanding the logic incorrectly. Still confused about whether that last if statement should be there or not

TabsTray.java

        private void updateSelectedPosition() {
            
            Tab tab = Tabs.getInstance().getSelectedTab();
            int selected = getPositionForTab(tab);
            
            final boolean tabIsPrivate = tab.isPrivate();
            
            if (tabIsPrivate)
            {
                if (tabIsPrivate != this.mIsPrivate)
                {
                    selected = AdapterView.INVALID_POSITION;
                }
            }
            
            updateSelectedStyle(selected);

            if (selected != AdapterView.INVALID_POSITION) {
                TabsTray.this.setSelection(selected);
            }
        }

Comment 6

[Brian Nicholson (:bnicholson)]

Sorry for the late response! I've been on vacation.

(In reply to Taha Lukmanji from comment #5)
> changed method updateSelectedPosition() according to the description above
> but seems like the bug is still there or may be I am understanding the logic
> incorrectly. Still confused about whether that last if statement should be
> there or not

Looking at TwoWayView more closely, I see that it actually supports both a selected state and checked state for views, which is rather confusing. When a tab is selected, we update both the selection state [1] and the item checked state [2]. It looks like the style may actually be tied to the checked state, so instead of calling setSelection(INVALID_POSITION), I think you can call clearChoices() to remove the selected style.

In the future, please upload your patch as an attachment rather than pasting your changes here. Even if you're not submitting it for review yet, it's easier for me to give feedback and prevents clutter in the comments.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsTray.java#215
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/TabsTray.java#226

Lucas, can you give some background on selected vs. checked in TwoWayView/TabsTray? Why are we setting both here?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> Lucas, can you give some background on selected vs. checked in
> TwoWayView/TabsTray? Why are we setting both here?

The concept of selection in adapter views (ListView, GridView, TwoWayView, etc) is a bit unintuitive. On touch mode, it will simply be the first visible position in the adapter view. So, setSelection() calls in ListViews (and TwoWayView, GridView, etc) pretty much means "move the list to this position".

The item's checked state has more to do with choice mode[1]. We use checked state to highlight the currently selected tab in the tabs tray.

I hope that helps.

[1] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/TwoWayView.java#131

Comment 8

[Taha Lukmanji]

Hi guys, I haven't worked on it recently as I have been quite busy. Plan is to hopefully fix it by weekend.

Comment 9

[Brian Nicholson (:bnicholson)]

Hi Taha, are you still looking at this bug? If not, I'll free it up for someone else to take. Thanks!

Comment 10

[Taha Lukmanji]

Hi Brian,

I am still looking at it. I'll update the bug with new findings. Sorry I have been busy and haven't had the time to look at it.

Taha

Comment 11

[Taha Lukmanji]

Hi Brian and others, bug is fixed. Thanks for your help. I read the mercurial patch stuff but flew over my head!

Comment 12

[Taha Lukmanji]

Attachment: Fixed TabView as well as other files. Made cosmetic changes in the other files. Feel free to submit the ones you want to put in the branch

Updated method updateSelectedPosition() in TabsTray and made cosmetic changes to other files like grouping constants into an enum for easier visibility and understanding

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8474925 [details] [diff] [review]
Fixed TabView as well as other files. Made cosmetic changes in the other files. Feel free to submit the ones you want to put in the branch

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

Hi Taha,

Thanks for all the work! In general, we try to keep patches as minimal as possible, and it helps that all changes are relevant to the specific bug. Please split these patches so that each change you're making has a single purpose; e.g., one patch for fixing the selected tab highlight (this bug), one patch for event string refactoring, etc. Then upload the patch specific to this bug here, and you can file separate bugs for any additional changes you want to contribute.

Seeing that you aren't too familiar with hg, here's how I would do this:
1) Download crecord from http://mercurial.selenic.com/wiki/CrecordExtension, and make sure you update your .hgrc file as described. This step is annoying, but qcrecord is incredibly useful, and makes the rest of these steps simple.
2) Apply your patch (make sure it's shown when you type "hg qapplied").
3) Type "hg qcrefresh".
4) Hold shift and press A to de-select all of the lines.
5) Use your arrow keys/spacebar to go through each line and include the lines specific to that patch. For instance, if you're making the patch to fix the tab highlight, you would only select lines from TabsTray.java.
6) Press C to save the selected changes to your patch. Your patch now has the changes you added from step 4, and your remaining changes are now unapplied.
7) Type "hg qcrecord" (since you're making a new patch, you don't want to use "qcrefresh" this time since that updates the current patch).
8) Again, go through and select the lines for that patch, then press C.
9) Repeat steps 7 & 8 for any other patches you want to make.

For managing (pushing/popping/renaming/updating) your patches, take a look at http://mercurial.selenic.com/wiki/MqExtension#Command_Examples.

::: mobile/android/base/GeckoApp.java
@@ +185,5 @@
>      public static final String SAVED_STATE_PRIVATE_SESSION = "privateSession";
>  
>      static private final String LOCATION_URL = "https://location.services.mozilla.com/v1/submit";
> +    
> +    public enum ButtonEvent 

I think it's a good idea to factor these message strings out into constants; after all, each one is used three times (registration, handling, and unregistration). But I wouldn't put these into an enum -- instead, you could just make these class-level string constants like the ones right above this (PREFS_ALLOW_STATE_BUNDLE, PREFS_OOM_EXCEPTION, etc). In this case, it would make sense for the names to be prefixed with EVENT_: EVENT_ACCESSIBILITY_READY, EVENT_BOOKMARK_INSERT, etc.

But please file a separate bug if you want to continue with these changes! For this bug, I think the only file you should have to change is TabsTray.java.

::: mobile/android/base/tabspanel/TabsTray.java
@@ +210,5 @@
> +            int selected = getPositionForTab(tab);
> +            
> +            final boolean tabIsPrivate = tab.isPrivate();
> +            
> +            if (tabIsPrivate)

I don't think you want this if statement; it should be sufficient to check only "tabIsPrivate != this.mIsPrivate" like you do below.

@@ +218,5 @@
> +                    selected = AdapterView.INVALID_POSITION;
> +                }
> +            }
> +            
> +            clearChoices();

We don't want to clear the selection every time this method is called; we want to clear it only if private state for the tabs panel doesn't match the private state of the selected tab. Put this inside the "tabIsPrivate != this.mIsPrivate" if statement above.

::: mobile/android/base/widget/TwoWayView.java
@@ -694,5 @@
>              boolean updateIds = mCheckedIdStates != null && mAdapter.hasStableIds();
>  
>              // Clear all values if we're checking something, or unchecking the currently
>              // selected item
> -            if (value || isItemChecked(position)) {

I think TabsTray.java is the only file that needs to be touched to fix this bug. If this change is needed, can you explain why you're making it?

Comment 14

[Andrzej Hunt :ahunt]

Attachment: Up to date patch that deselects the necessary tab when switching normal/private mode

I've started working on this since it looked abandoned - I hope that's ok?

I've essentially copied Taha's approach, but ported it to TabsListLayout + TabsGridLayout (TabsTray seems to no longer exist).

The patch works fine on mobile (i.e. TabsListLayout), I still need to test it in the tablet view though.

I'm just wondering whether it's worth refactoring the common code from TabsListLayout/TabsGridLayout (as a separate issue), but there doesn't really seem to be much more other shared code so maybe not that useful to do?

Comment 15

[Brian Nicholson (:bnicholson)]

Rerouting to mcomella since he's working on private browsing now, and I've switched to iOS :)

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Andrzej, I can't seem to reproduce this with the latest fx-team on my GS5 or my N7 – can you and if so, which device are you on?

Comment 17

[Andrzej Hunt :ahunt]

I've only tested using mozilla-central. I'm able to reproduce with a mozilla-central build from ~2 weeks ago, and also nightly from today. (I'm still waiting for a new local build of mozilla-central, and will try to test fx-team later.) I've done all my testing on a Note 2 (running Android 4.4).

Should I be doing development/testing on fx-team? I've found the repository, but can't seem to find much information about what it's for?

Comment 18

[:Margaret Leibovic]

(In reply to Andrzej Hunt :ahunt from comment #17)
> I've only tested using mozilla-central. I'm able to reproduce with a
> mozilla-central build from ~2 weeks ago, and also nightly from today. (I'm
> still waiting for a new local build of mozilla-central, and will try to test
> fx-team later.) I've done all my testing on a Note 2 (running Android 4.4).
> 
> Should I be doing development/testing on fx-team? I've found the repository,
> but can't seem to find much information about what it's for?

fx-team is an integration branch that gets merged to mozilla-central a few times a day. It's the place where we land Firefox front-end code (both desktop and Android) to make sure it passes all CI tests before getting merged to our main code base.

There shouldn't be much difference writing patches against mozilla-central vs. fx-team, but if you have commit access and are pushing your own patches, it's easier with fx-team.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'll try to reproduce on a wider range of devices tomorrow.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Okay, I can reproduce on this Samsung 2.3 device I found.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8646936 [details] [diff] [review]
Up to date patch that deselects the necessary tab when switching normal/private mode

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

The patch is functional but can use a little consistency clean up.

I was thinking it'd be cool for Grid & List to share this code somehow but List is being removed by Martyn soon, so I'm okay with the patch the way it is.

Thanks Andrzej!

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +249,5 @@
>      }
>  
>      // Updates the selected position in the list so that it will be scrolled to the right place.
>      private void updateSelectedPosition() {
> +        Tab tab = Tabs.getInstance().getSelectedTab();

nit: `final Tab`

`final` here and elsewhere.

@@ +255,5 @@
> +        int selected = mTabsAdapter.getPositionForTab(tab);
> +        final boolean isPrivate = tab.isPrivate();
> +
> +        if (isPrivate != mIsPrivate) {
> +            selected = AdapterView.INVALID_POSITION;

The selected value is unused - did you mean to store this as a member variable?

@@ +256,5 @@
> +        final boolean isPrivate = tab.isPrivate();
> +
> +        if (isPrivate != mIsPrivate) {
> +            selected = AdapterView.INVALID_POSITION;
> +            clearChoices();

clearChoices doesn't appear to request layout [1], unlike setItemChecked [2] which we use to set the selected style. I would imagine requestLayout would need to be called at some point after clearChoices - is there any reason it shouldn't be called here?

[1]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/widget/AbsListView.java#970
[2]: http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/widget/AbsListView.java#983

@@ +257,5 @@
> +
> +        if (isPrivate != mIsPrivate) {
> +            selected = AdapterView.INVALID_POSITION;
> +            clearChoices();
> +        } else if (selected != AdapterView.INVALID_POSITION) {

mTabsAdapter.getPositionForTab returns -1 in the event that the tab is not selected, rather than AdapterView.INVALID_POSITION. I'd prefer if mTabsAdapter return INVALID_POSITION in the error case and we checked it in all of the error cases (such as this one), or we just check for -1 (or `< 0` which is used fairly often as a convention) for its return value.

Comment 22

[Andrzej Hunt :ahunt]

Unassigning in case anyone else wants to take this over - it doesn't seem like a huge priority right now! It would probably be nice to unify our tab grid/list implementations as mcomella mentioned before doing this, however my patch should still be useful to anyone else wanting to do this.

Comment 23

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.