868432 - Scrolling should be animated when Home/End is pressed

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josiah Bruner [:jsbruner]]

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

[Markus Stange [:mstange]]

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.

Comment 2

[Josiah Bruner [:jsbruner]]

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

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: Patch 1.

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.

Comment 4

[Markus Stange [: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.

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Patch V 2

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.

Comment 6

[Markus Stange [:mstange]]

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, [e-mail] should be the right reviewer.

Comment 7

[Josiah Bruner [:jsbruner]]

Comment on attachment 746549 [details] [diff] [review]
Patch V 2

Thanks Markus!

In that case, flagging roc for review.

Comment 8

[Josiah Bruner [:jsbruner]]

Alrighty. Let's check her in.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff30118a4799

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1d1ddc189a for mochitest-chrome failures.

Comment 11

[Josiah Bruner [:jsbruner]]

Attachment: Alter Tests

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.

Comment 12

[Josiah Bruner [:jsbruner]]

Oh, and for evidence, here's the try run with the test fix applied:

https://tbpl.mozilla.org/?tree=Try&rev=cf006c0aa8ad

Comment 13

[Timothy Nikkel (:tnikkel)]

So this is going to change the behaviour on all platforms? Is that what we want?

Comment 14

[Josiah Bruner [:jsbruner]]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Josiah Bruner [:jsbruner]]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this test should depend on that.

Comment 18

[Josiah Bruner [:jsbruner]]

Fair enough. I'll have an update tomorrow.

Comment 19

[Josiah Bruner [:jsbruner]]

Attachment: Alter Tests V2

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.

Comment 20

[Josiah Bruner [:jsbruner]]

Successful try run: https://tbpl.mozilla.org/?tree=Try&rev=a36ec6d8c8f7

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 22

[Josiah Bruner [:jsbruner]]

Attachment: Alter Tests V2.1

Here we are...

Comment 23

[Josiah Bruner [:jsbruner]]

Cool, thanks Robert. 


Checkin now needed for both patches simultaneously...

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f51c6858ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab37a39c92a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43f51c6858ae
https://hg.mozilla.org/mozilla-central/rev/aab37a39c92a

Comment 26

[Josiah Bruner [:jsbruner]]

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

Comment 27

[Josiah Bruner [:jsbruner]]

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.

Comment 28

[Josiah Bruner [:jsbruner]]

Comment on attachment 746549 [details] [diff] [review]
Patch V 2

Err. Wrong bug...

Comment 29

[Stephen A Pohl [:spohl] (OOO until 9/9)]

Josiah, would you mind requesting approval for uplift to aurora on this one?

Comment 30

[Josiah Bruner [:jsbruner]]

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.

Comment 31

[Josiah Bruner [:jsbruner]]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f706537a360
https://hg.mozilla.org/releases/mozilla-aurora/rev/72fbaf2bef12

Comment 33

[Ioana (away)]

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

Comment 34

[Ioana (away)]

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