Closed
Bug 868432
Opened 12 years ago
Closed 12 years ago
Scrolling should be animated when Home/End is pressed
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | --- | unaffected |
firefox23 | + | verified |
firefox24 | --- | verified |
firefox-esr17 | --- | unaffected |
People
(Reporter: jsbruner, Assigned: jsbruner)
References
Details
(Whiteboard: [lion-scrollbars+])
Attachments
(2 files, 3 obsolete files)
982 bytes,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Natively, using Home/End to go to the start/end of pages animates the pages and the scrollbar for about a second. This is much more visually appealing and OS X consistent. Our custom scrollbars do not accomplish this task yet and instead "jump" to the top.
Comment 1•12 years ago
|
||
Joseah, do you want to fix this? I'd start with setting a breakpoint in nsGfxScrollFrameInner::ScrollToWithOrigin. Scrolling is smooth when this method is called with aMode == nsIScrollableFrame::SMOOTH, so you'd have to find out where aMode is decided on for home/end scrolling, and then set the SMOOTH flag in the right place.
Summary: Scrollbars should animate when Home/End is pressed → Scrolling should be animated when Home/End is pressed
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Markus Stange from comment #1) > Joseah, do you want to fix this? I'd start with setting a breakpoint in > nsGfxScrollFrameInner::ScrollToWithOrigin. Scrolling is smooth when this > method is called with aMode == nsIScrollableFrame::SMOOTH, so you'd have to > find out where aMode is decided on for home/end scrolling, and then set the > SMOOTH flag in the right place. Sure, I would be glad too.
Assignee: nobody → josiah
Assignee | ||
Comment 3•12 years ago
|
||
Patch that adds the ability for Home/End to animate the page scrolling. However, I do not believe this is how we want to go about this. I am only uploading because this is a very quick fix, and maybe there is a slight chance this is good enough. Markus, here I directly modify nsGfxScrollFrameInner::ScrollBy, to essentially ignore the aMode flag when the case = nsIScrollableFrame::WHOLE:. I do believe we want to actually modify the code that calls this function and modify the flag there correct? If for some reason this is all that we need then wonderful, but I highly doubt it.
Flags: needinfo?(mstange)
Comment 4•12 years ago
|
||
(In reply to Josiah Bruner [:JosiahOne] from comment #3) > I do believe we want to actually modify the code > that calls this function and modify the flag there correct? Correct.
Flags: needinfo?(mstange)
Assignee | ||
Comment 5•12 years ago
|
||
Here we go. Patch correctly switches to smooth scrolling animations on Home/End presses. Markus, I have no idea who to put down for review, so a suggestion would be appreciated.
Attachment #746457 -
Attachment is obsolete: true
Comment 6•12 years ago
|
||
Great! You can often find good reviewers for the code in question by looking at the blame history of the code you're changing: http://hg.mozilla.org/mozilla-central/annotate/41ff3b67b692/layout/base/nsPresShell.cpp#l2221 (You can click on the file change link in the left column (e.g. robert@37057), and then on the changeset link (changeset 37057: 176699b95417) if you want to get from there to the bug where this change was made.) In this case, roc@ocallahan.org should be the right reviewer.
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 746549 [details] [diff] [review] Patch V 2 Thanks Markus! In that case, flagging roc for review.
Attachment #746549 -
Flags: review?(roc)
Attachment #746549 -
Flags: review?(roc) → review+
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff30118a4799
Flags: in-testsuite-
Keywords: checkin-needed
Comment 10•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1d1ddc189a for mochitest-chrome failures.
Assignee | ||
Comment 11•12 years ago
|
||
This patch alters the two tests that failed. Both tests where designed with an instant scroll in mind, however that is no longer the case. Roc, the first altered test is definitely something you could review. The second test however really belongs to Enn, but he is on leave, so I was wondering if you could review that as well. If not, feel free to pass it on.
Attachment #748832 -
Flags: review?(roc)
Assignee | ||
Comment 12•12 years ago
|
||
Oh, and for evidence, here's the try run with the test fix applied: https://tbpl.mozilla.org/?tree=Try&rev=cf006c0aa8ad
Comment 13•12 years ago
|
||
So this is going to change the behaviour on all platforms? Is that what we want?
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #13) > So this is going to change the behaviour on all platforms? Is that what we > want? I would assume so. However I'm not sure what the default behavior is on Windows.
Comment on attachment 748832 [details] [diff] [review] Alter Tests Review of attachment 748832 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/tests/chrome/test_showcaret.xul @@ +54,5 @@ > + var sel2 = frames[1].getSelection(); > + sel2.collapse(frames[1].document.body, 0); > + window.frames[0].focus(); > + document.commandDispatcher.getControllerForCommand("cmd_moveBottom").doCommand("cmd_moveBottom"); > + timeoutID = window.setTimeout(continueTest, 1000); setTimeout is generall a bad idea when we don't know exactly how long something will take. I suggest polling until we reach the desired position instead.
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15) > Comment on attachment 748832 [details] [diff] [review] > Alter Tests > > Review of attachment 748832 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/tests/chrome/test_showcaret.xul > @@ +54,5 @@ > > + var sel2 = frames[1].getSelection(); > > + sel2.collapse(frames[1].document.body, 0); > > + window.frames[0].focus(); > > + document.commandDispatcher.getControllerForCommand("cmd_moveBottom").doCommand("cmd_moveBottom"); > > + timeoutID = window.setTimeout(continueTest, 1000); > > setTimeout is generall a bad idea when we don't know exactly how long > something will take. I suggest polling until we reach the desired position > instead. Hmm... It seems to me like we do know how long this will take. The animation is a set speed no matter what circumstance. If it takes any longer than it does now, we have an issue. Perhaps I am wrong though?
I don't think this test should depend on that.
Assignee | ||
Comment 19•12 years ago
|
||
Here we loop the check every second. If the test doesn't succeed within 10 seconds, we can safely say it will never pass, and thereby fail the test.
Attachment #748832 -
Attachment is obsolete: true
Attachment #748832 -
Flags: review?(roc)
Attachment #749372 -
Flags: review?(roc)
Assignee | ||
Comment 20•12 years ago
|
||
Successful try run: https://tbpl.mozilla.org/?tree=Try&rev=a36ec6d8c8f7
Comment on attachment 749372 [details] [diff] [review] Alter Tests V2 Review of attachment 749372 [details] [diff] [review]: ----------------------------------------------------------------- We could just rely on the general mochitest timeout, but this is fine. But change the polling interval down from 1000ms to 10ms. It will only speed up the test.
Attachment #749372 -
Flags: review?(roc) → review+
Assignee | ||
Comment 22•12 years ago
|
||
Here we are...
Attachment #749372 -
Attachment is obsolete: true
Attachment #749534 -
Flags: review?(roc)
Attachment #749534 -
Flags: review?(roc) → review+
Assignee | ||
Comment 23•12 years ago
|
||
Cool, thanks Robert. Checkin now needed for both patches simultaneously...
Keywords: checkin-needed
Comment 24•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f51c6858ae https://hg.mozilla.org/integration/mozilla-inbound/rev/aab37a39c92a
Keywords: checkin-needed
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43f51c6858ae https://hg.mozilla.org/mozilla-central/rev/aab37a39c92a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 746549 [details] [diff] [review] Patch V 2 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 636564 User impact if declined: The new scroll bars will arrive to users without proper dimensions, causing OS-inconsistency. Testing completed (on m-c, etc.): Already landed on m-c (https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e89758cc4) and successfully built locally and on Try: https://tbpl.mozilla.org/?tree=Try&rev=09f95e211607 Risk to taking this patch (and alternatives if risky): Pretty low-risk, worst that could happen is that the scroll bars might not work 100% properly until they can be fixed, but any changes can be made almost immediately and things have already been tested rigorously on 10.8. String or IDL/UUID changes made by this patch: None
Attachment #746549 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 749534 [details] [diff] [review] Alter Tests V2.1 [Approval Request Comment] See previous comment. This test-altering is necessary for the previous patch to pass tests correctly.
Attachment #749534 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 746549 [details] [diff] [review] Patch V 2 Err. Wrong bug...
Attachment #746549 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #749534 -
Flags: approval-mozilla-aurora?
![]() |
||
Updated•12 years ago
|
status-firefox21:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
status-firefox24:
--- → fixed
status-firefox-esr17:
--- → unaffected
tracking-firefox23:
--- → ?
Whiteboard: [lion-scrollbars+]
Comment 29•12 years ago
|
||
Josiah, would you mind requesting approval for uplift to aurora on this one?
Flags: needinfo?(josiah)
Updated•12 years ago
|
Assignee | ||
Comment 30•12 years ago
|
||
Comment on attachment 746549 [details] [diff] [review] Patch V 2 [Approval Request Comment] Bug caused by: Bug 636564 User impact if declined: When "native" scroll bars arrive to users, they will lack part of the expected behavior that these scroll bars should posses. Thereby creating an inconsistent user experience. Testing completed: Already landed on M-C (https://hg.mozilla.org/mozilla-central/rev/43f51c6858ae). Also, this patch and the test altering patch was tested successfully on Aurora Try here: https://tbpl.mozilla.org/?tree=Try&rev=0d736148240c Risk to taking this patch: No risk. String or IDL/UUID changes made by this patch: Not applicable.
Attachment #746549 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(josiah)
Assignee | ||
Comment 31•12 years ago
|
||
Comment on attachment 749534 [details] [diff] [review] Alter Tests V2.1 [Approval Request Comment] Bug caused by: Bug 636564 User impact if declined: When "native" scroll bars arrive to users, they will lack part of the expected behavior that these scroll bars should posses. Thereby creating an inconsistent user experience. Testing completed: Already landed on M-C (https://hg.mozilla.org/mozilla-central/rev/aab37a39c92a). Also, this patch and the other patch that I requested aurora approval on was tested successfully on Aurora Try here: https://tbpl.mozilla.org/?tree=Try&rev=0d736148240c Risk to taking this patch: No risk. String or IDL/UUID changes made by this patch: Not applicable.
Attachment #749534 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #746549 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #749534 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 32•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f706537a360 https://hg.mozilla.org/releases/mozilla-aurora/rev/72fbaf2bef12
Comment 33•11 years ago
|
||
Verified as fixed on Firefox 23 beta 7: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0
QA Contact: ioana.budnar
Comment 34•11 years ago
|
||
Verified as fixed on Firefox 24 beta 1: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
You need to log in
before you can comment on or make changes to this bug.
Description
•