464984 - find in page highlights words, but doesn't scroll if they are not in the current view

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

If you load up www.mozilla.org, hit ctrl+f to bring up the find in page, search for "mozilla", you will find many instances.  I do this and hit the "next" button and it finds matches that are not shown in the default window.  If I scroll down, I see the matches being made.

We need to scroll or indicate that we are finding matches.  This is very confusing for the user.

Comment 1

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

CanvasBrowser.ensureElementIsVisible should do the trick.

Comment 2

[Mike Kristoffersen (:MikeK)]

From a naive point of view, I would expect a call to selectionController->ScrollSelectionIntoView(...) as is done in nsTypeAheadFind.cpp would do this, but obviosly it doesn't.  Trying to force somehting to happen I even tried removing the ScrollSelectionIntoView, replacing it with ScrollLine, but also no effect there.

btw. can anyone explain to me why the function names in the IDL files start with a lower case letter, but the "real" function calls are uppercase?

Comment 3

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

Remember that selectionController->ScrollSelectionIntoView(...) is moving the browser, not the canvas. We need to determine if the scroll into view call is firing a "MozAfterPaint" event so we can redraw the canvas with the current browser content.

IDL methods & properties start with a lowercase and the lowercase is also used in JavaScript. In C++, it is converted to uppercase (and properties have a Get/Set prefix added). In both cases, it is done because it better suits the coding style for that language.

Comment 4

[Mike Kristoffersen (:MikeK)]

Do we need some kind of "user interested in this area" event, this could then take care of the scrolling both of the browser and the canvas, it could also be used for fancy visual effects to guide the users attention to a specific area (like waves coming from it, lightening going into the area, or ants eating everything else of the screen except the area)

Comment 5

[Mike Kristoffersen (:MikeK)]

Another issue bought up by roc is that if a script on a page want to ensure that the top of the page is visible, then it might fail if the scrollTop (https://developer.mozilla.org/en/DOM/element.scrollTop) is zero, but the top of the page isn't visible.  As it is the script on the page that holds the logic in this case, it isn't something that can be solved by the above described event.

(This is also the case for scrollLeft (https://developer.mozilla.org/en/DOM/element.scrollLeft)

I'm not sure if there are some functions that will let script code read the visible width/height of a page, if there is and this has not yet been fixed, then we need to consider a solution for this too.

Comment 6

[Mike Kristoffersen (:MikeK)]

Directly related to this bug is also the question of how to behave regarding scroll if the highligthed item is under the "search prompt" (the thing where the user enters what he want to search for) as this is semi transparent.  Will something that is under this semitransparent "window" be considered visible?  I personally don't think so, but what are the opinion of others on this?

Comment 7

[Shriram (irc: Mavericks)]

Quite an interesting problem. Is it possible to paste screenshots as I don't have access to the environment yet for fennec? Any progress on this fix.

Comment 8

[Mike Kristoffersen (:MikeK)]

Attachment: Screenshot showing issue with highlighted text hidden by find bar

Comment 9

[Mike Kristoffersen (:MikeK)]

(In reply to comment #7)
> Quite an interesting problem. Is it possible to paste screenshots as I don't
> have access to the environment yet for fennec? Any progress on this fix.

I have attached a screen shot of the issue to this bug (look for "Developer News" in the lower right corner of the image ("under" the find bar).  However I assume this issue with the find bar will be captured by https://bugzilla.mozilla.org/show_bug.cgi?id=436074 that deals witht he position and behaviour of the find bar it self (I was not aware of this bug when I initially reported the problem here).

About the progress of this particuallary bug - while this bug might seem pretty simple, it is in my view just a sign of a more fundamental problem.  The fundamental problem is that we due to performance reasons has removed the responsibility for which part of the page that is visible from the browser.  

I the following bugs could have the same cause:

https://bugzilla.mozilla.org/show_bug.cgi?id=480762
https://bugzilla.mozilla.org/show_bug.cgi?id=479753
https://bugzilla.mozilla.org/show_bug.cgi?id=464984
https://bugzilla.mozilla.org/show_bug.cgi?id=460524
https://bugzilla.mozilla.org/show_bug.cgi?id=475678
https://bugzilla.mozilla.org/show_bug.cgi?id=479862
https://bugzilla.mozilla.org/show_bug.cgi?id=452286 

There is currently a debate ongoing as how to fix this.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Bug 480762 and bug 479753 are not related to this bug, they're just bugs with the current widgetstack code.

Bug 479862 and bug 460524 would be solved by making the browser element the same size as the viewport. Stuart was opposed to this idea because it might cause pages to lay out strangely compared to a large layout+zoom, but maybe we can re-evaluate the tradeoffs there.

Bug 452286 and bug 479862 are about the browser scroll state not being synced between the browser and our canvasbrowser. We could fix that easily enough with scroll event listeners (to detect page-triggered scrolls) and .scrollTo() calls (to inform the browser of pans), I think, but we'd need to make sure those won't cause performance issues.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> Bug 479862 and bug 460524 would be solved by making the browser element the
> same size as the viewport.

I meant bug 475678 and bug 460524.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

And of the three issues I've mentioned, this bug is related to bug 452286 and bug 479862, I think. The native find code is causing the document to scroll, so this bug would probably be fixed by a fix for bug 452286. Alternatively, we could have the find code use a new API that the canvasBrowser would implement to do the scrolling. We already have canvasBrowser.ensureElementIsVisible, for example.

Comment 13

[Mike Kristoffersen (:MikeK)]

Attachment: Syncing scroll and pan - fixed size display 800*480

This is for trying out the behavior I have been talking about, it is hardcoded to a 800*480 display.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I tried applying this patch with no luck.  I assume it was meant to apply in the mobile-browser branch, not mozilla-central.

I tried a 'patch -p1 < ~/patch' as well as a 'hg import ~/patch'.  Each time I had many hunks failing.

Comment 15

[Mike Kristoffersen (:MikeK)]

Yep, it's a mobile only patch and should be applied to /mobile - it is not production quality, it was made for others to be able to do performance testing on the solution where we scroll the browser content instead of the other path where we would instead try to hook event up to what the user was doing and then pan.

Comment 16

[Shriram (irc: Mavericks)]

I found this issue on windows platform too. I tested the package made for windows on basic vista 32bit.

Comment 17

[Mike Kristoffersen (:MikeK)]

(In reply to comment #16)
> I found this issue on windows platform too. I tested the package made for
> windows on basic vista 32bit.

Thank your for testing and reporting, it is expected that all Fennec platforms have this issue (unless a patch is applied, I assume you tested a vanila build without the "patch" attached to this bug?)

Comment 18

[Mike Kristoffersen (:MikeK)]

(In reply to comment #13)
> Created an attachment (id=367622) [details]
> Syncing scroll and pan - fixed size display 800*480
> 
> This is for trying out the behavior I have been talking about, it is hardcoded
> to a 800*480 display.

Please note that apart from being hardcoded to an 800*480 display then this changelist has undefined behavior if using multiple tabs and could do a litle nicer with the lower part of the screen during search.

If anyone wan't to test the changes, then the interesting things to test for are: 

1) If it degrades performance
2) How is the feel of the "auto-zoom-out" that comes into action when jumping to an anchor or start searching on the page

Thanks

Comment 19

[Shriram (irc: Mavericks)]

(In reply to comment #17)
> (In reply to comment #16)
> > I found this issue on windows platform too. I tested the package made for
> > windows on basic vista 32bit.
> 
> Thank your for testing and reporting, it is expected that all Fennec platforms
> have this issue (unless a patch is applied, I assume you tested a vanila build
> without the "patch" attached to this bug?)

Yes, I tested it without the patch. I will try to test it with the patch and report the results

Comment 20

[Mike Kristoffersen (:MikeK)]

Attachment: Updated diff to top of tree this morning (GMT)

This updated diff has the same issues as the previous one - only difference is that it has been updated to the current version of the source tree and that the fix for bug 456621 has been removed from it.

Comment 21

[Shriram (irc: Mavericks)]

(In reply to comment #18)
> (In reply to comment #13)
> > Created an attachment (id=367622) [details] [details]
> > Syncing scroll and pan - fixed size display 800*480
> > 
> > This is for trying out the behavior I have been talking about, it is hardcoded
> > to a 800*480 display.
> 
> Please note that apart from being hardcoded to an 800*480 display then this
> changelist has undefined behavior if using multiple tabs and could do a litle
> nicer with the lower part of the screen during search.
> 
> If anyone wan't to test the changes, then the interesting things to test for
> are: 
> 
> 1) If it degrades performance
> 2) How is the feel of the "auto-zoom-out" that comes into action when jumping
> to an anchor or start searching on the page
> 
> Thanks

Auto-zoom-out works great, and it doesn't degrade performance :)

Comment 23

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

I've just post a patch extracted from the Mike's one (mainly the scroll handling part) on bug 470861.

I think we should made this one depends on bug 470861 and address here problems related to the interaction between findbar and the scrolling part:
 1. zoom mechanism if needed for findbar

 2. scroll a bit more on scroll event (the height of the findbar) to avoid as much as possible to have something under the transparent findbar
 3. Sometimes (for example if the title bar is open) the search find something that is not in the viewing rect but doesn't throw a scroll event (that's not a bug)

scrollTop/scrollLeft related issues should be handle in bug 479862.


Cause of point 2 and 3, I'm for hacking around the find API (comments #12).

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Fixed by bug 452286.

Comment 25

[Aakash Desai [:aakashd]]

This is not fixed on build: Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090828 Fennec/1.0a3

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Please don't reopen FIXED bugs unless their patch is backed out. New bugs are cheap! :)

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(in this case, the bug's already on file as bug 512277)

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

Gavin, I cannot verify the bug as being fixed, I don't understand why you don't want to have a reopened bug.  bug 512277 has no mention of a checkin and this bug is still a problem in 20090903 builds.  In QA we need to verify the resolved bugs are actually fixed which we cannot do with this bug.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

The problem is that verifications need to keep up with the rewrites :)

There's no use reopening a FIXED bug when the applicable code has since been rewritten, which is the case here. The only reason to REOPEN a FIXED bug is if its patch is being backed out.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

verified by creating a new bug to track this.