482057 - When the Ctrl+F Find bar is enabled and the Find text box is focused, smooth scrolling does not work

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tyler Harangozo]

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)]

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]]

I'll look into this.

Comment 4

[Kamil Páral]

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 6

[Matt Brubeck (:mbrubeck)]

Attachment: 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]]

Comment on attachment 608817 [details] [diff] [review]
patch

Looks very reasonable!

Comment 8

[Matt Brubeck (:mbrubeck)]

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]]

(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]

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/[e-mail]-8a9e65054344

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6241724db3

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8c6241724db3

Comment 14

[Matt Brubeck (:mbrubeck)]

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)]

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]]

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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/d0a67d911787

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

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]

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]

(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.