Closed Bug 482057 Opened 11 years ago Closed 8 years ago
When the Ctrl+F Find bar is enabled and the Find text box is focused, smooth scrolling does not work
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.
Confirmed on Windows XP. Range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4f1383af8f10&tochange=920f440d7c88 Could be caused by Bug 457864.
Component: General → Layout
OS: Linux → All
Product: Firefox → Core
QA Contact: general → layout
Hardware: x86 → All
Version: unspecified → Trunk
I'll look into this.
Assignee: nobody → mstange
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9.2? → wanted1.9.2+
Confirmed on Mozilla/5.0 (X11; U; Linux x86_64; cs-CZ; rv:18.104.22.168) Gecko/20090909 Fedora/3.5.3-1.fc11 Firefox/3.5.3.
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.)
Attachment #608817 - Flags: feedback?(mstange)
Comment on attachment 608817 [details] [diff] [review] patch Looks very reasonable!
Attachment #608817 - Flags: feedback?(mstange) → feedback+
Is it possible to add tests for this? Is there anything else you'd like to see here before I ask for review?
(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.)
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://firstname.lastname@example.org
Attachment #608817 - Flags: review?(enndeakin)
Target Milestone: --- → mozilla14
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 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.
Attachment #608817 - Flags: approval-mozilla-aurora?
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.
Attachment #608817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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.
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..
(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.
You need to log in before you can comment on or make changes to this bug.