790897 - Find in page can cause tabs to be switched instead of searching for the text

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: logs

Firefox Mobile 16.0b3
Device: Samsung Galaxy Tab 2 7.0 (Android 4.0.4)/ HTC Desire Z (Android 2.3.3)/Samsung Galaxy R (Android 2.3.4)

Steps to reproduce:
1. Open in different tabs planet.mozilla.org, cnn.com, metacafe.com and a few other pages - 3 to 5 tabs should be ok
2. Go to the planet.mozilla.org and search for "mozilla"
3. Use the down arrow to go to the next match until you reach the last match

Expected results:
After the last match the page is scrolled to the top and the search begins again

Actual results:
At some point instead of moving to the next match the app starts to cycle through the open tabs.
This is reproducible on the Samsung Tab only in Landscape mode for some reason and works ok in Portrait mode but on the HTC Desire Z and Samsung Galaxy R I was able to reproduce the issue in Portrait mode.

Notes:
Please see the videocaptures: http://youtu.be/0R9Q2DYjidA, http://youtu.be/cKYLMsM1r5s
Not seeing anything in the logs but I am also attaching them.
Issue is not reproducible on Firefox Mobile 15.0.1

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Margaret - Can you get this to happen?

Comment 2

[Aaron Train [:aaronmt]]

Trying this on the Galaxy Tab 2 7.0 noted above and I was able to reproduce. I rapidly tapped the next down arrow button and my current tab swapped from Planet Mozilla over to CNN, and then over to MetaCafe.

Adrian, can you narrow down a regression-window?

Comment 3

[:Margaret Leibovic]

Is this only happening on tablets? I don't have a tablet with me, so I wouldn't be able to debug this until later next week if that's the case.

Comment 4

[Aaron Train [:aaronmt]]

(In reply to Margaret Leibovic [:margaret] from comment #3)
> Is this only happening on tablets? I don't have a tablet with me, so I
> wouldn't be able to debug this until later next week if that's the case.

Yes; just ran the same test on my Galaxy Nexus and it happened too.

Comment 5

[Aaron Train [:aaronmt]]

 > Yes; just ran the same test on my Galaxy Nexus and it happened too.

s/Yes/No

Comment 6

[:Margaret Leibovic]

I was also able to reproduce on my Galaxy Nexus, but it didn't always happen. I wonder if this is a regression from bug 771997. I'll investigate further.

Comment 7

[:Margaret Leibovic]

Looking at the code, I'm not sure how we're ending up switching tabs. I'm also having trouble reliably reproducing this. I saw it happen once, but not again. Aaron or adrian, can you help me drill down minimal STR?

Comment 8

[Adrian Tamas (:AdrianT)]

I can reproduce the issue about 60+% off the time following the steps:
1. Load news.google.com, cnn.com, support.mozilla.org in different tabs
2. Go to the news.google.com and search for a term that does not appear very often like "riot", "news", "cnn"
3. Tap repeatedly on the down arrow. Doing this ~ 15-20 times should change the tab to support.mozilla.org

Comment 9

[Adrian Tamas (:AdrianT)]

Tried to reproduce the issue on Nightly 16.0a1 2012-07-11 which has the fix for bug 771997 and I am not able to reproduce the issue. One thing I am seeing is that on that build the keyboard is dismissed on the first match. Issue may be produced by that bug and the fixes for bug 760087 and bug 771989. I will investigate further trying to narrow down a regression range

Comment 10

[Adrian Tamas (:AdrianT)]

(In reply to adrian tamas from comment #9)
> Tried to reproduce the issue on Nightly 16.0a1 2012-07-11 which has the fix
> for bug 771997 and I am not able to reproduce the issue. One thing I am
> seeing is that on that build the keyboard is dismissed on the first match.
> Issue may be produced by that bug and the fixes for bug 760087 and bug
> 771989. I will investigate further trying to narrow down a regression range
Sorry I forgot to mention that I can only reproduce the issue if the virtual keyboard is opened. If the keyboard is closed I could not find a way to reproduce the issue.

Comment 11

[Adrian Tamas (:AdrianT)]

Please disregard comment 9. Due to the fact that this only happens when the vkb is opened it is very hard to reproduce the bug on older builds before the fix for Bug 771989 was integrated.

I believe I have narrowed down a regression range:
Good build: 2012-07-07
Bad build: 2012-07-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9abaedb956a1&tochange=1751d97cc9e4

I have seen a few more details that need to happen for the issue to be reproducible:
- the keyboard needs to be opened with find in page 
- there has to be a match for the search in one of the other tabs

Comment 12

[Aaron Train [:aaronmt]]

Sounds like a race condition of events due to the fact that you have to navigate rather quickly in order to reproduce. Due to that, I would lessen the severity of this bug.

Comment 13

[Adrian Tamas (:AdrianT)]

Actually I just found the correct STR of the bug. Here are a set of simple steps to reproduce:
1. Load any 3 pages in 3 different tabs
2. Go to the first tab and open the Find in page
3. Tap in the search field to open the keyboard
4. Tap between the 2 up and down arrows

Every time you tab between the 2 arrows the tab is changed. I have confirmed this by using the stylus of the Galaxy Note (Android 4.0.4) for precision. I can also confirm that the regression range should be correct as I am unable to reproduce the issue on the 2012-07-07 build.

Sorry about about the other comments and presumptions but it seems that while tapping on the down arrow sometimes I would tap between them reproducing the issue and the rest was just some unfortunate coincidences. The issue has nothing to do with content or specific pages the only requirement is for the vkb to be opened with the Find in page bar and in case of the tablet for the device to be in landscape - not sure why the issue is not reproducible in portrait.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this has an assignee and clear STR we'll go track this, Margaret do you need any more from QA to get to the bottom of this?

Comment 15

[:Margaret Leibovic]

I haven't been able to reproduce the tab switching with the up/down arrows, but I've found that hitting the "x" on the find in page bar will sometimes close a background tab (I've only been able to reproduce that when I've had at least 3 tabs).

There's definitely something bad going on here. It seems like our message passing is getting screwed up somehow. I will continue to investigate.

Comment 16

[:Margaret Leibovic]

Aha, I found that the tab closing issue was caused by a call to Tabs.closeTab() from inside TabsTray.animateTo:
http://hg.mozilla.org/mozilla-central/annotate/6a2d1a3556b9/mobile/android/base/TabsTray.java#l312

There's a similar call to Tabs.selectTab() from inside TabsTray.onTouchEnd, so I suspect that's what's getting called and causing the odd tab switching as well.

I'm not familiar with this part of the TabsTray code. Sriram, do you have any idea how this code could be executing after tapping buttons in the find in page bar? I added some logging and found that FindInPageBar's onClick handler is never even being called, so maybe something else is managing to capture our click events? It seems like the only relevant changeset in the regression range is:
http://hg.mozilla.org/mozilla-central/rev/8dee2a85c0c9

Perhaps changing the layout around changed the way clicks are getting handled? Any help would be appreciated!

Comment 17

[:Margaret Leibovic]

Continuing my investigation, I found that what's happening is that the close button click listener is being fired:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#159

Comment 18

[:Margaret Leibovic]

I also verified that the selectTab call is coming from TabsTray.onTouchEnd. It seems the root problem here is that TabsTray is getting these click events. I'm having a tough time following the logic behind hiding the tabs tray, but it seems like maybe this is a bad side effect of the way we have our layout set up for animations. I'm starting to think I really need Sriram's help here.

Comment 19

[:Margaret Leibovic]

Attachment: proof of concept fix

It seems like actually hiding the panel instead of just changing its dimensions fixes this issue. However, this patch feels hacky. I feel like it would be nicer to incorporate this in TabsPanel's show/hide methods, but we need to wait for the animations to end.

Sriram, do you know a better way of doing this? And does it make sense that the visibility could affect whether or not touch events are captured?

Comment 20

[:Margaret Leibovic]

Attachment: simpler fix

I found that the problem was actually that when a click would miss hitting the button directly (like when you're going really fast and rushing), it would get captured by the TabsTray view below the find in page bar. Capturing these clicks and eating them seems to fix the problem.

Build here for QA to verify:
http://people.mozilla.org/~mleibovic/tmp/findinpage.apk

Comment 21

[Aaron Train [:aaronmt]]

(In reply to Margaret Leibovic [:margaret] from comment #20)
> Build here for QA to verify:
> http://people.mozilla.org/~mleibovic/tmp/findinpage.apk

Took a run through this with the same approach in comment #2, and wasn't able to reproduce.

>+import android.util.Log;
>+

Remove?

Comment 22

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #21)
> (In reply to Margaret Leibovic [:margaret] from comment #20)
> > Build here for QA to verify:
> > http://people.mozilla.org/~mleibovic/tmp/findinpage.apk
> 
> Took a run through this with the same approach in comment #2, and wasn't
> able to reproduce.
> 
> >+import android.util.Log;
> >+
> 
> Remove?

Oops, yeah :)

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 664608 [details] [diff] [review]
simpler fix

>diff --git a/mobile/android/base/FindInPageBar.java b/mobile/android/base/FindInPageBar.java

>+import android.util.Log;

Yeah, looks like it's not needed

Comment 24

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aebc8d0fc3a1

Comment 25

[:Margaret Leibovic]

Comment on attachment 664608 [details] [diff] [review]
simpler fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression caused by a combination of changes to the tabs/find in page bar layouts
User impact if declined: tapping on the find in page bar can cause tabs to unexpectedly switch or close
Testing completed (on m-c, etc.): just landed on inbound 
Risk to taking this patch (and alternatives if risky): low-risk fix to prevent clicks on the find in page bar from getting captured by other views
String or UUID changes made by this patch: n/a

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 664608 [details] [diff] [review]
simpler fix

[Triage Comment]
Fix looks very low risk. once green on m-i, please land on m-a and m-b.

Comment 27

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6a6b5f8c22c
https://hg.mozilla.org/releases/mozilla-beta/rev/1dca6212be3a

Comment 28

[Adrian Tamas (:AdrianT)]

Verified on Firefox Mobile 16.0b5 on Samsung Galaxy R running Android 2.3.4. I am no longer able to change the tabs by tapping between the up and down arrows of the "Find in page" bar.

Comment 29

[Mounir Lamouri (:mounir)]

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