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

VERIFIED FIXED in fennec1.0b2

Status

defect
P2
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: jmaher, Assigned: bcombee)

Tracking

Trunk
fennec1.0b2

Firefox Tracking Flags

(fennec1.0b2+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
CanvasBrowser.ensureElementIsVisible should do the trick.
Whiteboard: [good first bug]
Blocks: 436074
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?
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.
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)

Updated

10 years ago
tracking-fennec: --- → 1.0b2+
Assignee: nobody → mkristoffersen
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.
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?
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.
(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.
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.
(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.
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.
This is for trying out the behavior I have been talking about, it is hardcoded to a 800*480 display.
(Reporter)

Comment 14

10 years ago
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.
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.
I found this issue on windows platform too. I tested the package made for windows on basic vista 32bit.
(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?)
(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
(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
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.
Attachment #367622 - Attachment is obsolete: true
(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 :)

Updated

10 years ago
Assignee: mkristoffersen → combee
Priority: -- → P2
Whiteboard: [good first bug]

Updated

10 years ago
Duplicate of this bug: 452286
(Assignee)

Updated

10 years ago
Attachment #370182 - Attachment is patch: true
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).
No longer depends on: 470861
Fixed by bug 452286.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → B2
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please don't reopen FIXED bugs unless their patch is backed out. New bugs are cheap! :)
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(in this case, the bug's already on file as bug 512277)
(Reporter)

Comment 28

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

Comment 30

10 years ago
verified by creating a new bug to track this.
Status: RESOLVED → VERIFIED
Resolution: FIXED → WONTFIX
Resolution: WONTFIX → FIXED
You need to log in before you can comment on or make changes to this bug.