909539 - [TABLET] Tapping on a history item in the new UI brings the next item below the one tapped

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Mike Hommey [:glandium]]

When I tap on bookmarks in the new UI, the right thing is visually selected, but then an entirely different bookmark is opened.

Comment 1

[:Margaret Leibovic]

I can't reproduce this issue. Do you have more specific STR? Is sync set up? Is it an off-by-one error, or is there no pattern?

In bug 908423, wrote a patch for something that sounds similar, but I can't reproduce the issue here even without that patch applied.

Comment 2

[Mike Hommey [:glandium]]

So, in fact, this is not on bookmarks, this is on history. And it's not completely random, I'm just getting the following entry. So if I tap the on above, i get what i want.

Comment 3

[:Margaret Leibovic]

Thanks for the details. This probably has to do with the header rows in the history list. I can look into a fix.

Comment 4

[:Margaret Leibovic]

I'm still having trouble reproducing this... is it a specific page in the "History" tab, e.g. "Most visited" or "Most recent"? Would you mind taking a screenshot to show what your list looks like? I'm confused as to how this is happening.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Screenshot

It happens on both "Most visited" and "Most recent". The list of "Tabs from last time" is empty so I can't say for that one. If I tap on e.g. the "Index of ftp://ftp.mozilla.org/...", I get to Amazon.co.jp.

Comment 7

[:Margaret Leibovic]

Ah, I couldn't reproduce on a phone, but I could reproduce on a tablet.

I'm cc'ing Sriram and Shilpan, since they've done more of the work on tablets.

Comment 8

[:Margaret Leibovic]

I added some logging, and I found that on tablets, the position we get on item click is one higher than on phones:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/MostVisitedPage.java#106

Do we have a secret hidden item on tablets that could be causing this problem?

Comment 9

[Sriram Ramasubramanian [:sriram]]

Shilpan,

Are you hiding anything?

Comment 10

[:Margaret Leibovic]

It looks like we add a header view for tablets, but we're not accounting for that in the item click listener:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeListView.java#66

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 12

[:Margaret Leibovic]

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

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

Does this cause problems with BookmarksListView now, where we're already adjusting for the header rows?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListView.java#106

I like that you could just centralize this in HomeListView. Before giving an r+, I just want to confirm that the item click listener BookmarksListView still does the right thing with this patch.

::: mobile/android/base/home/HomeListView.java
@@ +100,5 @@
>      }
>  
> +    @Override
> +    public void setOnItemClickListener(final AdapterView.OnItemClickListener listener) {
> +        super.setOnItemClickListener(new AdapterView.OnItemClickListener() {

Add some comments in here about why we need to be doing this.

@@ +105,5 @@
> +            @Override
> +            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                if (listener == null) {
> +                    return;
> +                }

If someone calls setOnItemClickListener with a null listener, should we just bail earlier and never set a listener?

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #12)
> Comment on attachment 796359 [details] [diff] [review]
> Patch
> 
> Review of attachment 796359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this cause problems with BookmarksListView now, where we're already
> adjusting for the header rows?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> BookmarksListView.java#106
> 
> I like that you could just centralize this in HomeListView. Before giving an
> r+, I just want to confirm that the item click listener BookmarksListView
> still does the right thing with this patch.

BookmarksListView will not show top divider. Hence this logic won't be used there. The position would remain unchanged. I've tested that part.

> 
> ::: mobile/android/base/home/HomeListView.java
> @@ +100,5 @@
> >      }
> >  
> > +    @Override
> > +    public void setOnItemClickListener(final AdapterView.OnItemClickListener listener) {
> > +        super.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> 
> Add some comments in here about why we need to be doing this.
> 
> @@ +105,5 @@
> > +            @Override
> > +            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> > +                if (listener == null) {
> > +                    return;
> > +                }
> 
> If someone calls setOnItemClickListener with a null listener, should we just
> bail earlier and never set a listener?

I thought of doing that. But, even if its null, we need to inform the "super" about it to nullify the listener. Or, we could just do that job ourselves. But again, this code is not being used in multiple places. There is just one call to set the listener when lists are used. They are never made null again. The null check I've added can be removed actually.

Comment 14

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> (In reply to :Margaret Leibovic from comment #12)
> > Comment on attachment 796359 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 796359 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Does this cause problems with BookmarksListView now, where we're already
> > adjusting for the header rows?
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> > BookmarksListView.java#106
> > 
> > I like that you could just centralize this in HomeListView. Before giving an
> > r+, I just want to confirm that the item click listener BookmarksListView
> > still does the right thing with this patch.
> 
> BookmarksListView will not show top divider. Hence this logic won't be used
> there. The position would remain unchanged. I've tested that part.

Sounds good. Hopefully there won't be a bug here if things ever change, though!

> > 
> > ::: mobile/android/base/home/HomeListView.java
> > @@ +100,5 @@
> > >      }
> > >  
> > > +    @Override
> > > +    public void setOnItemClickListener(final AdapterView.OnItemClickListener listener) {
> > > +        super.setOnItemClickListener(new AdapterView.OnItemClickListener() {
> > 
> > Add some comments in here about why we need to be doing this.
> > 
> > @@ +105,5 @@
> > > +            @Override
> > > +            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> > > +                if (listener == null) {
> > > +                    return;
> > > +                }
> > 
> > If someone calls setOnItemClickListener with a null listener, should we just
> > bail earlier and never set a listener?
> 
> I thought of doing that. But, even if its null, we need to inform the
> "super" about it to nullify the listener. Or, we could just do that job
> ourselves. But again, this code is not being used in multiple places. There
> is just one call to set the listener when lists are used. They are never
> made null again. The null check I've added can be removed actually.

I'm never a big fan of unnecessary null checks, so removing it sounds good.

Comment 15

[:Margaret Leibovic]

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

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

Does this cause problems with BookmarksListView now, where we're already adjusting for the header rows?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListView.java#106

I like that you could just centralize this in HomeListView. Before giving an r+, I just want to confirm that the item click listener BookmarksListView still does the right thing with this patch.

::: mobile/android/base/home/HomeListView.java
@@ +100,5 @@
>      }
>  
> +    @Override
> +    public void setOnItemClickListener(final AdapterView.OnItemClickListener listener) {
> +        super.setOnItemClickListener(new AdapterView.OnItemClickListener() {

Add some comments in here about why we need to be doing this.

@@ +105,5 @@
> +            @Override
> +            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                if (listener == null) {
> +                    return;
> +                }

If someone calls setOnItemClickListener with a null listener, should we just bail earlier and never set a listener?

Comment 16

[:Margaret Leibovic]

Sorry, splinter duplicated all my comments.

Comment 17

[Sriram Ramasubramanian [:sriram]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

FYI, this landing ended up touching HwcComposer2D.cpp.
https://hg.mozilla.org/integration/fx-team/annotate/b7df57dc193e/widget/gonk/HwcComposer2D.cpp

I'm guessing you did an hg rebase before pushing? It has some known issues with possibly doing this. Please make sure you are running the latest version, and if you are, avoid rebasing.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[u421692]

Verified as fixed on:
Device: Samsung Galaxy Tab(Android 4.0.4)
Build: Nightly 26 (2013-09-04)