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

VERIFIED FIXED in Firefox 16

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: AdrianT, Assigned: Margaret)

Tracking

({regression})

Trunk
Firefox 18
ARM
Android
regression
Points:
---

Firefox Tracking Flags

(firefox15 unaffected, firefox16+ verified, firefox17 fixed, firefox18 verified, fennec+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
Margaret - Can you get this to happen?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
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?
Severity: major → normal
Keywords: regression, regressionwindow-wanted
Whiteboard: regression
(Assignee)

Comment 3

5 years ago
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.
(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.
 > Yes; just ran the same test on my Galaxy Nexus and it happened too.

s/Yes/No
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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?
(Reporter)

Comment 8

5 years ago
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
(Reporter)

Comment 9

5 years ago
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
(Reporter)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: regressionwindow-wanted
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.
(Reporter)

Comment 13

5 years ago
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.

Updated

5 years ago
tracking-firefox16: --- → ?
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?
tracking-firefox16: ? → +
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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!
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
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?
Attachment #663528 - Flags: feedback?(sriram)
(Assignee)

Comment 20

5 years ago
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
Attachment #663528 - Attachment is obsolete: true
Attachment #663528 - Flags: feedback?(sriram)
Attachment #664608 - Flags: review?(mark.finkle)
(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?
(Assignee)

Comment 22

5 years ago
(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 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
Attachment #664608 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aebc8d0fc3a1
Target Milestone: --- → Firefox 18
(Assignee)

Comment 25

5 years ago
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
Attachment #664608 - Flags: approval-mozilla-beta?
Attachment #664608 - Flags: approval-mozilla-aurora?
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.
Attachment #664608 - Flags: approval-mozilla-beta?
Attachment #664608 - Flags: approval-mozilla-beta+
Attachment #664608 - Flags: approval-mozilla-aurora?
Attachment #664608 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 27

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6a6b5f8c22c
https://hg.mozilla.org/releases/mozilla-beta/rev/1dca6212be3a
status-firefox16: affected → fixed
status-firefox17: affected → fixed
status-firefox18: affected → fixed
(Reporter)

Comment 28

5 years ago
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.
status-firefox16: fixed → verified
https://hg.mozilla.org/mozilla-central/rev/aebc8d0fc3a1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox18: fixed → verified
You need to log in before you can comment on or make changes to this bug.