309030 - Don't scroll to found link on findbar close

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

If a link is found in FAYT mode, when Find Toolbar is closed, we set focus to
found link. But in this time, if the link is not in view, contents are scrolled
to found link. It should not be so. we should lock scroll when we set focus to
found link.

Comment 1

[Asaf Romano (gone)]

Personally, I find the current behavior pretty good (for FAYT).

Comment 2

[Asaf Romano (gone)]

Oh, nvm, i see the bug.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

Attachment: Patch rv1.0

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 196520 [details] [diff] [review]
Patch rv1.0

>Index: toolkit/components/typeaheadfind/content/findBar.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.js,v
>retrieving revision 1.24
>diff -u -8 -p -r1.24 findBar.js
>--- toolkit/components/typeaheadfind/content/findBar.js	26 Aug 2005 04:21:37 -0000	1.24
>+++ toolkit/components/typeaheadfind/content/findBar.js	18 Sep 2005 11:52:02 -0000
>@@ -392,19 +392,27 @@ function delayedCloseFindBar()
>   var findToolbar = document.getElementById("FindToolbar");
>   var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>                      .getService(Components.interfaces.nsIWindowWatcher);
> 
>   if (window == ww.activeWindow) { 
>     var focusedElement = document.commandDispatcher.focusedElement;
>     if (focusedElement && (focusedElement.parentNode == findToolbar ||
>                            focusedElement.parentNode.parentNode == findField)) {
>-      if (gFoundLink)
>+      if (gFoundLink) {
>+        // Don't scroll to found link. If contents are scrolled by the user,
>+        // the scrolling is always occured after FAYT's scrolling.
>+        // So, we should not cancel user's operations.
>+        var suppressedScroll = document.commandDispatcher.suppressFocusScroll;
>+        if (!suppressedScroll)
>+          document.commandDispatcher.suppressFocusScroll = true;
>         gFoundLink.focus();
>-      else
>+        if (!suppressedScroll)
>+          document.commandDispatcher.suppressFocusScroll = false;
>+      } else
>         window.content.focus();

this works, but I'd rather see the comment and style changed to the following:

if (gFoundLink) {
  // block scrolling on focus since find already scrolls, further scrolling
  // is due to user action, so don't override this
  var suppressedScroll = document.commandDispatcher.suppressFocusScroll;
  document.commandDispatcher.suppressFocusScroll = true;
  gFoundLink.focus();
  document.commandDispatcher.suppressFocusScroll = suppressedScroll;
}
else

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

Attachment: Patch for check-in

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

checked-in.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

The risk is low. And this is regression.

Comment 8

[Mike Schroepfer]

Comment on attachment 196617 [details] [diff] [review]
Patch for check-in

Approved for 1.8b5 per bug meeting

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Got a cold so that using sick leaves)]

checed-in to 1.8branch.