Closed Bug 868432 Opened 7 years ago Closed 7 years ago

Scrolling should be animated when Home/End is pressed

Categories

(Core :: Widget: Cocoa, defect, minor)

All
macOS
defect
Not set
minor

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)

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.
See Also: → 868397
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
(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
Attached patch Patch 1. (obsolete) — Splinter Review
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)
(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)
Attached patch Patch V 2Splinter Review
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
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.
Comment on attachment 746549 [details] [diff] [review]
Patch V 2

Thanks Markus!

In that case, flagging roc for review.
Attachment #746549 - Flags: review?(roc)
Alrighty. Let's check her in.
Keywords: checkin-needed
Attached patch Alter Tests (obsolete) — Splinter Review
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)
Oh, and for evidence, here's the try run with the test fix applied:

https://tbpl.mozilla.org/?tree=Try&rev=cf006c0aa8ad
So this is going to change the behaviour on all platforms? Is that what we want?
(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.
(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.
Fair enough. I'll have an update tomorrow.
Attached patch Alter Tests V2 (obsolete) — Splinter Review
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)
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+
Attached patch Alter Tests V2.1Splinter Review
Here we are...
Attachment #749372 - Attachment is obsolete: true
Attachment #749534 - Flags: review?(roc)
Cool, thanks Robert. 


Checkin now needed for both patches simultaneously...
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/43f51c6858ae
https://hg.mozilla.org/mozilla-central/rev/aab37a39c92a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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?
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?
Comment on attachment 746549 [details] [diff] [review]
Patch V 2

Err. Wrong bug...
Attachment #746549 - Flags: approval-mozilla-aurora?
Attachment #749534 - Flags: approval-mozilla-aurora?
Josiah, would you mind requesting approval for uplift to aurora on this one?
Flags: needinfo?(josiah)
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)
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?
Attachment #746549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #749534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 873727
Keywords: verifyme
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
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.