Last Comment Bug 790897 - Find in page can cause tabs to be switched instead of searching for the text
: Find in page can cause tabs to be switched instead of searching for the text
Status: VERIFIED FIXED
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: :Margaret Leibovic
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-13 02:27 PDT by Adrian Tamas (:AdrianT)
Modified: 2016-07-29 14:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
fixed
verified
+


Attachments
logs (28.81 KB, text/plain)
2012-09-13 02:27 PDT, Adrian Tamas (:AdrianT)
no flags Details
proof of concept fix (1.58 KB, patch)
2012-09-21 13:03 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
simpler fix (2.17 KB, patch)
2012-09-25 12:10 PDT, :Margaret Leibovic
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Adrian Tamas (:AdrianT) 2012-09-13 02:27:14 PDT
Created attachment 660762 [details]
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?) 2012-09-13 10:16:07 PDT
Margaret - Can you get this to happen?
Comment 2 Aaron Train [:aaronmt] 2012-09-13 11:19:51 PDT
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 2012-09-13 11:28:06 PDT
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] 2012-09-13 12:00:48 PDT
(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] 2012-09-13 12:01:34 PDT
 > Yes; just ran the same test on my Galaxy Nexus and it happened too.

s/Yes/No
Comment 6 :Margaret Leibovic 2012-09-14 00:54:50 PDT
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 2012-09-14 01:12:26 PDT
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) 2012-09-14 02:00:35 PDT
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) 2012-09-14 02:10:42 PDT
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) 2012-09-14 02:16:57 PDT
(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) 2012-09-14 06:23:46 PDT
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] 2012-09-14 06:33:11 PDT
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) 2012-09-14 07:02:30 PDT
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 2012-09-14 15:49:37 PDT
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 2012-09-20 08:47:08 PDT
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 2012-09-20 09:51:04 PDT
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 2012-09-20 10:42:31 PDT
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 2012-09-20 11:09:47 PDT
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 2012-09-21 13:03:21 PDT
Created attachment 663528 [details] [diff] [review]
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 2012-09-25 12:10:04 PDT
Created attachment 664608 [details] [diff] [review]
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] 2012-09-25 12:34:46 PDT
(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 2012-09-25 13:09:01 PDT
(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?) 2012-09-25 13:34:04 PDT
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 25 :Margaret Leibovic 2012-09-25 13:46:18 PDT
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] 2012-09-25 14:56:13 PDT
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 28 Adrian Tamas (:AdrianT) 2012-09-26 00:29:11 PDT
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) 2012-09-26 04:07:47 PDT
https://hg.mozilla.org/mozilla-central/rev/aebc8d0fc3a1

Note You need to log in before you can comment on or make changes to this bug.