Last Comment Bug 482057 - When the Ctrl+F Find bar is enabled and the Find text box is focused, smooth scrolling does not work
: When the Ctrl+F Find bar is enabled and the Find text box is focused, smooth ...
Status: RESOLVED FIXED
[qa+]
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla14
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
: 475950 599768 739377 (view as bug list)
Depends on:
Blocks: 710372 457864
  Show dependency treegraph
 
Reported: 2009-03-07 10:16 PST by Tyler Harangozo
Modified: 2012-11-03 07:57 PDT (History)
16 users (show)
roc: wanted1.9.2+
roc: blocking1.9.1-
roc: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
patch (1.90 KB, patch)
2012-03-23 12:29 PDT, Matt Brubeck (:mbrubeck)
enndeakin: review+
mstange: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tyler Harangozo 2009-03-07 10:16:37 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090307 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090307 Minefield/3.2a1pre

When the find bar is up, and the "Find" text input box is focused (with the caret flashing), smooth scrolling (both with the arrow keys and pgup+pgdown) does not work.

Reproducible: Always

Steps to Reproduce:
0. Enable smooth scrolling.
1. Go to a webpage that is longer than your available screen height.
2. Hit Ctrl+F to pull up the find bar, and click in the "Find" text input box.
3. Press the arrow keys (or PgUp/PgDn) to scroll the page.
Actual Results:  
Smooth scrolling did not work.

Expected Results:  
Smooth scrolling should have worked.
Comment 1 Ria Klaassen (not reading all bugmail) 2009-03-07 11:27:46 PST
Confirmed on Windows XP.
Range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4f1383af8f10&tochange=920f440d7c88 
Could be caused by Bug 457864.
Comment 2 Markus Stange [:mstange] 2009-03-10 03:09:14 PDT
I'll look into this.
Comment 3 Brian Polidoro 2009-05-05 10:52:12 PDT
*** Bug 475950 has been marked as a duplicate of this bug. ***
Comment 4 Kamil Páral 2009-10-21 07:18:25 PDT
Confirmed on Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:1.9.1.3) Gecko/20090909 Fedora/3.5.3-1.fc11 Firefox/3.5.3.
Comment 5 Bobby Johnson [:rjohnson19] 2010-09-26 16:42:43 PDT
*** Bug 599768 has been marked as a duplicate of this bug. ***
Comment 6 Matt Brubeck (:mbrubeck) 2012-03-23 12:29:43 PDT
Created attachment 608817 [details] [diff] [review]
patch

This bug is much more noticeable now that smooth scrolling is enabled by default in Firefox.  I'm not familiar with this code, and I don't know if this fix is reasonable or not.  Any comments?

Some background:

It looks like this regression happened because nsScrollPortView::ScrollByPages stopped setting NS_VMREFRESH_SMOOTHSCROLL by default in this change (bug 457864):
https://hg.mozilla.org/mozilla-central/rev/89b91ae29a3d#l3.248

The code has moved around a lot since then (those functions don't even exist anymore), but the current code includes this comment:

> // It seems like it would make more sense for ScrollByPages to use
> // SMOOTH mode, but tests seem to depend on the synchronous behaviour.
> // Perhaps Web content does too.
https://hg.mozilla.org/mozilla-central/file/6470fe2fc4de/dom/base/nsGlobalWindow.cpp#l5602

If I understand correctly, this means that window.scrollByPages cannot easily be modified to use asynchronous smooth scrolling.  So my patch makes the findbar use nsISelectionController.scrollPage instead, which does not have this problem.  (This is the same method used internally by cmd_scrollPageUp/Down in nsGlobalWindowCommands.)
Comment 7 Markus Stange [:mstange] 2012-03-23 13:44:05 PDT
Comment on attachment 608817 [details] [diff] [review]
patch

Looks very reasonable!
Comment 8 Matt Brubeck (:mbrubeck) 2012-03-23 14:03:30 PDT
Is it possible to add tests for this?  Is there anything else you'd like to see here before I ask for review?
Comment 9 Markus Stange [:mstange] 2012-03-23 14:59:54 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> Is it possible to add tests for this?

Hmm, the only thing that can be tested reliably is probably that scrolling isn't instant (synchronous) any more; but discerning async from smooth will be difficult because of timing indeterminism.

> Is there anything else you'd like to
> see here before I ask for review?

No, but I'm not the expert in binding code, maybe ask Enn for review.
(Though I do notice that you access a private method of a non-"this" object. Not sure if that's objectionable.)
Comment 10 Mozilla RelEng Bot 2012-03-23 22:06:57 PDT
Try run for 8a9e65054344 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8a9e65054344
Results (out of 58 total builds):
    success: 45
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-8a9e65054344
Comment 12 Alice0775 White 2012-03-26 15:56:36 PDT
*** Bug 739377 has been marked as a duplicate of this bug. ***
Comment 13 Marco Bonardo [::mak] 2012-03-27 05:27:44 PDT
https://hg.mozilla.org/mozilla-central/rev/8c6241724db3
Comment 14 Matt Brubeck (:mbrubeck) 2012-03-30 15:31:45 PDT
This is not a new regression, but it is newly exposed in the default configuration in Firefox 13 because of bug 198964.  Nominating for tracking.
Comment 15 Matt Brubeck (:mbrubeck) 2012-03-30 15:47:34 PDT
Comment on attachment 608817 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): bug 198964 (see comment 14 above)

User impact if declined:  Keyboard scrolling with the Find Bar focused uses non-smooth scrolling, while keyboard scrolling everywhere else uses smooth scrolling.

This is basically a polish bug, but I think it's worth nominating because (a) it creates a jarring UX once you've gotten used to smooth scrolling, (b) it's exposed by a new feature in Fx13, and (c) there is a simple low-risk fix.

Testing completed (on m-c, etc.): landed on m-c on 3/27

Risk to taking this patch (and alternatives if risky): This patch is fairly low-risk.  It makes the Find Bar call the same methods that are already used for keyboard scrolling elsewhere in the UI.  The code it touches is run only during keypresses with the Find Bar focused, so there's no risk of regressions outside that scenario.

Any unexpected bugs would most likely cause the find bar to fail to respond to keyboard input.  If any such regression were found on Aurora or Beta, the patch could be backed out easily.

String changes made by this patch: None.
Comment 16 Alex Keybl [:akeybl] 2012-04-02 10:05:48 PDT
Comment on attachment 608817 [details] [diff] [review]
patch

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> Risk to taking this patch (and alternatives if risky): This patch is fairly
> low-risk.  It makes the Find Bar call the same methods that are already used
> for keyboard scrolling elsewhere in the UI.  The code it touches is run only
> during keypresses with the Find Bar focused, so there's no risk of
> regressions outside that scenario.
> 
> Any unexpected bugs would most likely cause the find bar to fail to respond
> to keyboard input.  If any such regression were found on Aurora or Beta, the
> patch could be backed out easily.

Your evaluation of risk/reward is very reasonable. I'd expect that any fallout here would be immediately obvious through QA verification of this bug and feedback from our testing population given the feature's visibility. Approved for Aurora 13.
Comment 17 Matt Brubeck (:mbrubeck) 2012-04-02 11:46:05 PDT
Pushed a followup to remove one line of old code that was accidentally left in place:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a67d911787

Pushed the original patch with the follow-up folded in to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/af44d93f2159
Comment 18 Marco Bonardo [::mak] 2012-04-03 02:01:03 PDT
https://hg.mozilla.org/mozilla-central/rev/d0a67d911787
Comment 19 Mihaela Velimiroviciu (:mihaelav) 2012-05-09 05:23:21 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0.
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0

Verified the fix using the steps from bug description (except that smooth scroll is already enabled): pages scroll smoothly when focus is in find bar.
Marking verified for Firefox 13.
Comment 20 Prakash 2012-11-02 18:22:27 PDT
Performed steps mentioned in bug. smooth scrolling is working fine with version 16.0.2. 
up/down, pageup/pagedown keys are working fine when focus is on Find text box, but  home and end key is not working..
Comment 21 Tyler Harangozo 2012-11-03 07:57:00 PDT
(In reply to Prakash from comment #20)
> Performed steps mentioned in bug. smooth scrolling is working fine with
> version 16.0.2. 
> up/down, pageup/pagedown keys are working fine when focus is on Find text
> box, but  home and end key is not working..

That is the intended behaviour - when focus is on the find bar, the Home and End keys are used for navigation within the "Find:" text input, not for scrolling the page.

Note You need to log in before you can comment on or make changes to this bug.