597036 - Find in page does not display results on screen when page is zoomed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ayan Shah[:ashah]]

Build:
Mozilla/5.0(Android;Linux armv7l;rv2.0b7pre)Gecko/20100916/4.0b7pre Fennec/2.0b1pre

STR:
1. Open www.mozilla.com
2. Zoom in on the page.
3. Open the find bar and search for the keyword "fire"
4. Press the up/down arrow on the find bar in order to see the results

Expected result:
The word should appear on the screen in front of the user

Actual result: 
The results are hidden behind the find bar (i.e. it is not shown to the user on the screen in front of him). This might lead him to think that there are no more find results(more prominent when the page is zoomed).

FWIW, move the page in order to show the results to the user on the screen in front of him. The results should not be hidden behind the find bar

Comment 1

[Ayan Shah[:ashah]]

Also seeing this without the zoom. specially when you search for common words like "the","is","of" etc

Comment 2

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

The "zoom and pan to an element" is broken for some cases. Watch out for dupes! The same code is used for Form Assistant.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Also occurs on N900 : 

verified FIXED on build:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100916 Firefox/4.0b7pre Fennec/2.0b1pre

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

The verified FIXED on build should not be noted in the last comment please ignore that portion.

The issue still occurs on the N900:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100916 Firefox/4.0b7pre
Fennec/2.0b1pre

Comment 5

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

This seems to be working fine for me on today's Android build. I tested the "www.mozilla.com" site for "fire", like in comment 0.

I saw all the found "fire" spots. I compared with desktop firefox to make sure fennec found all the same occurrences.

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Issue still occurs on Maemo :
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100927 Firefox/4.0b7pre Fennec/2.0b1pre

Issue does not occur on Android:
Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20100927 Firefox/4.0b7pre Fennec/2.0b1pre

Changed Platform from All to Maemo to match.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #6)
> Issue still occurs on Maemo :
> Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100927 Firefox/4.0b7pre
> Fennec/2.0b1pre
> 
> Issue does not occur on Android:
> Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20100927 Firefox/4.0b7pre
> Fennec/2.0b1pre
> 
> Changed Platform from All to Maemo to match.

Naoki,
It works for me on Maemo using the steps to reproduce descrivbe in comment 0 and the latest nightly.
Do you have another steps to repro?

Comment 8

[Andreea Pod]

I can reproduce this with:

Device: Nokia N900
BuildID: Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b8pre)Gecko/20101021
Firefox/4.0b8pre Fennec /4.0b2pre 

Device: HTC Desire A8181
BuildID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre) Gecko/20101021
Firefox/4.0b8pre Fennec /4.0b2pre  

Steps to reproduce:

1. Visit www.mozilla.org
2. Bring up the “Find In Page” toolbar by tapping on the page favicon and then “Find in page”.
3. Enter string “mozilla” in the search field. 
4. Use arrow-buttons to navigate through your results 

On HTC Desire, I see ghost tab thumbnails when navigating from one match to another. Bug 604946 was logged for this but with different steps to reproduce and is Verified Fixed.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Naoki has tried to help me reproduce it on IRC but for some reasons I can't :(
 
What I suspect here is we are calculating the zoom level on a wrong height with results to misplaced the element to looks like they are above the find helper (even if they can't _really_ be above the content-navigator-helper element like it was the cas 1 year ago)

Fixing the zoom to calculate on the correct height seems a good solution here (and a must do) but I can not really affirm it will fix the bug since I can reproduce it.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #8)
> I can reproduce this with:
> 
> Device: Nokia N900
> BuildID: Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b8pre)Gecko/20101021
> Firefox/4.0b8pre Fennec /4.0b2pre 
> 
> Device: HTC Desire A8181
> BuildID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b8pre) Gecko/20101021
> Firefox/4.0b8pre Fennec /4.0b2pre  
> 
> Steps to reproduce:
> 
> 1. Visit www.mozilla.org
> 2. Bring up the “Find In Page” toolbar by tapping on the page favicon and then
> “Find in page”.
> 3. Enter string “mozilla” in the search field. 
> 4. Use arrow-buttons to navigate through your results 
> 
> On HTC Desire, I see ghost tab thumbnails when navigating from one match to
> another. Bug 604946 was logged for this but with different steps to reproduce
> and is Verified Fixed.

Sorry I've posted my comment without seeing yours. Thanks for sharing your steps to reproduce.

Using your steps, can you explain the behavior you're seeing? Basically my question is: do you have the feeling to have the find result _under_ the find bar or does the view point to something completely unrelated?

Thanks.

Comment 11

[Andreea Pod]

Matched strings are highlighted for a second then the focus jumps somewhere else in the page (see http://www.facebook.com/video/?id=100001755601293#!/video/video.php?v=100319606703226)

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #11)
> Matched strings are highlighted for a second then the focus jumps somewhere
> else in the page (see
> http://www.facebook.com/video/?id=100001755601293#!/video/video.php?v=100319606703226)

Since I don't have a facebook account I can't see the video but hopefully your comment has made me thought of it with a different approach and I've found the that we receive a "scroll" event from the content _after_ the zoom is done. This scroll event was forcing us to display the wrong view.

Thanks for your participation!

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

The patch just call the asyncSendMessage outside the FindAssist:Find message to let the scroll event propagate _before_ we're zooming to the right position.
The results is that we can see two zoom actions, the first because Fennec react to the "scroll" event and the second because Fennec react to the "FindAssist:Show" event. 

So the plan is to disabled the "scroll" event on the browser while the Find helper is opened.

Comment 14

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

Comment on attachment 485296 [details] [diff] [review]
Patch v0.2

I don't like turning scroll syncing off like this:

>+    browser.messageManager.removeMessageListener("scroll", browser._messageListenerRemote);

>+    browser.messageManager.addMessageListener("scroll", browser._messageListenerRemote);

Do we need to use this since you are using a timeout to send the FormAssist:Show?

Can we add a property to the browser to disable scrollSync? browser.scrollSync = true | false  ?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #14)
> Comment on attachment 485296 [details] [diff] [review]
> Patch v0.2
> 
> I don't like turning scroll syncing off like this:
> 
> >+    browser.messageManager.removeMessageListener("scroll", browser._messageListenerRemote);
> 
> >+    browser.messageManager.addMessageListener("scroll", browser._messageListenerRemote);
> 
> Do we need to use this since you are using a timeout to send the
> FormAssist:Show?
 
Yes, because the timeout ensure the "scroll" event arrive before the FindAssist:Show message but we still need to ignore it to ensure the view is not scrolled twice.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Comment 17

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

Comment on attachment 485684 [details] [diff] [review]
Patch v0.3

Maybe in the future we can somehow move more of this logic into the browser binding, so we don't need to expose the "scrollSync" property. For now, this is better.

Comment 18

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

Does this not affect FormHelper? I guess the find system is controlling the scrolling, which is different than FormHelper.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/8be7b23e97cb

Comment 20

[Matt Brubeck (:mbrubeck)]

Attachment: followup

The fix had an error; "this.scrollSync" always returns undefined.  This regressed bug 598391.  Here's the one-line fix.

Comment 21

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

pushed followup:
http://hg.mozilla.org/mobile-browser/rev/b064cb6eadf7

Comment 22

[Aakash Desai [:aakashd]]

verified FIXED on builds:


Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101026 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101026 Namoroka/4.0b8pre Fennec/4.0b2pre