Last Comment Bug 801101 - Needs a option to scroll over one page
: Needs a option to scroll over one page
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks: 817979 719320
  Show dependency treegraph
 
Reported: 2012-10-12 14:32 PDT by Anonymous
Modified: 2012-12-04 02:16 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (25.43 KB, patch)
2012-10-23 23:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (25.44 KB, patch)
2012-10-24 02:04 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review-
Details | Diff | Splinter Review
Patch (17.08 KB, patch)
2012-10-29 00:01 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review

Description Anonymous 2012-10-12 14:32:12 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121010150351

Steps to reproduce:

In Firefox <= 16, I could configure Firefox to jump to the top/bottom of the page with shift-mousewheel by setting mousewheel.withshiftkey.action = 4 and mousewheel.withshiftkey.numlines to a large number like 99999999.

Firefox 17 breaks this for no good reason.  I've tried setting mousewheel.with_shift.action = 1 and mousewheel.with_shift.delta_multiplier_y = 99999999 but it doesn't work.  It only scrolls by a page.

These changes remove a feature that I liked and make me very frustrated.  Stop taking away things that used to work.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-12 20:19:12 PDT
Hmm, I'm surprised. If mouse wheel caused over one page once, it was a bug :-)
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-12 20:24:10 PDT
Smaug:

Do you think that we should have such pref? Basically, I don't think so. If we would implemented such option, it's should be available for all modifiers. So, it needs a lot of new prefs in all.js...
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-12 20:36:44 PDT
some other possible approaches:

1. If we could add DOM_DELTA_WHOLE to deltaMode at D3E, it's the simplest solution. But I'm not sure whether it's necessary for other browser venders.

2. Ignore the one page limitation only when deltaMode is DOM_DELTA_PAGE. Basically, page scrolling isn't used by so many users. And it's difficult to guess that native events causes too high speed scroll with the settings. However, if we do this approach, it may conflict with acceleration mechanism.
Comment 4 Olli Pettay [:smaug] 2012-10-13 07:11:44 PDT
So why doesn't mousewheel.with_shift.action = 1 and mousewheel.with_shift.delta_multiplier_y = 99999999
work?
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-13 17:19:48 PDT
(In reply to Olli Pettay [:smaug] from comment #4)
> So why doesn't mousewheel.with_shift.action = 1 and
> mousewheel.with_shift.delta_multiplier_y = 99999999
> work?

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2891

Here is. This limitation is useful for most cases.

And I forgot that .action cannot specify the deltaMode now. So, both ideas in comment 3 are wrong. I'm still thinking a way to pass the limitation simply.
Comment 6 Olli Pettay [:smaug] 2012-10-13 17:25:51 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > So why doesn't mousewheel.with_shift.action = 1 and
> > mousewheel.with_shift.delta_multiplier_y = 99999999
> > work?
> 
> http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> nsEventStateManager.cpp#2891
> 
> Here is. This limitation is useful for most cases.

Well, could we check there that if multiply is large enough, we would scroll more than
a page. Say, if multiply is larger than height/width of the screen.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-13 19:34:25 PDT
Hmm, x1000 is large enough, maybe. To make sure the change must be safe for all users, x10000 must not be safe. I.e., over 1000000 or less -1000000 should cause whole scroll. But then, should the delta values be multiplied by the pref? I worry that web applications could be confused with the crazy delta values.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-23 23:13:20 PDT
Created attachment 674560 [details] [diff] [review]
Patch
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-24 01:55:49 PDT
Comment on attachment 674560 [details] [diff] [review]
Patch

This patch only changes the default action behavior. I.e., it doesn't change DOM event's delta values. I think that this is better solution since the whole scroll is just a special default action of Gecko. Even if we should change the DOM event's delta values, I'm not sure what is good value for them.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-24 01:57:42 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=61e5f595e996
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-24 02:04:14 PDT
Comment on attachment 674560 [details] [diff] [review]
Patch

Oops, sorry, this is old patch.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-24 02:04:39 PDT
Created attachment 674595 [details] [diff] [review]
Patch
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-24 02:05:16 PDT
Comment on attachment 674595 [details] [diff] [review]
Patch

This is the latest patch.
Comment 14 Olli Pettay [:smaug] 2012-10-25 06:22:44 PDT
Comment on attachment 674595 [details] [diff] [review]
Patch

>   nsPresContext* pc = scrollFrame->PresContext();
>+  // Note that scrollAmount and scrollAmountInDevPixels are not familiar with
>+  // whole page scroll.
Note that scrollAmount and scrollAmountInDevPixels are not affected by
...

Or something like that

>   nsSize scrollAmount = GetScrollAmount(pc, aEvent, aScrollableFrame);
>   nsIntSize scrollAmountInDevPixels(
>     pc->AppUnitsToDevPixels(scrollAmount.width),
>     pc->AppUnitsToDevPixels(scrollAmount.height));
>   nsIntPoint actualDevPixelScrollAmount =
>     DeltaAccumulator::GetInstance()->
>-      ComputeScrollAmountForDefaultAction(aEvent, scrollAmountInDevPixels);
>+      ComputeScrollAmountForDefaultAction(aEvent, scrollAmountInDevPixels,
>+                                          aScrollableFrame);
> 

>+  // overflowDelta for whole scroll should be 0 or the delta value.
>+  // If the event didn't cause scroll, it should be the delta value.
>+  // Otherwise, 0.
Why not always 0 ?



>+bool
>+nsEventStateManager::WheelPrefs::IsWholeScrollY(widget::WheelEvent* aEvent)
>+{
>+  Index index = GetIndexFor(aEvent);
>+  Init(index);
>+  return NS_ABS(mMultiplierY[index]) >=
>+           MIN_WHOLE_SCROLL_DELTA_MULTIPLIER_VALUE;
>+}
I don't understand the word 'whole' here. 
Perhaps FullContentScrollY/X



This all feels very complicated for this rarely used case.
Could we just allow > page scrolls if multiplier is large enough and also report the right values to the page?
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-25 06:32:34 PDT
(In reply to Olli Pettay [:smaug] from comment #14)
> >+  // overflowDelta for whole scroll should be 0 or the delta value.
> >+  // If the event didn't cause scroll, it should be the delta value.
> >+  // Otherwise, 0.
> Why not always 0 ?

If we do so, it never causes gesture such as history back/forward by swipe on Mac.

> This all feels very complicated for this rarely used case.
> Could we just allow > page scrolls if multiplier is large enough and also
> report the right values to the page?

I'm not sure "report the right values to the page". What did you mean?
Comment 16 Olli Pettay [:smaug] 2012-10-25 06:58:21 PDT
You said in comment 7 "I worry that web applications could be confused with the crazy delta values."
But I'm not really worried about that.
If user wants to scroll a lot, so be it.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-25 07:05:31 PDT
Hmm, do you think that just unlock the limit if the multiplier is very large? I mean that we don't need to change the scroll amount even if it might be less than the scrolled page width/height?
Comment 18 Olli Pettay [:smaug] 2012-10-25 07:09:59 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> Hmm, do you think that just unlock the limit if the multiplier is very
> large?
Something like that. Hopefully it is a very simple change. We should try to make the
code simpler, not more complicated :)

> I mean that we don't need to change the scroll amount even if it
> might be less than the scrolled page width/height?
I think so.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-25 07:11:11 PDT
Okay, then, it can be simple.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-29 00:01:55 PDT
Created attachment 676060 [details] [diff] [review]
Patch
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-30 16:25:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7546a81c1c38
Comment 22 Ed Morley [:emorley] 2012-10-31 07:18:11 PDT
https://hg.mozilla.org/mozilla-central/rev/7546a81c1c38

Note You need to log in before you can comment on or make changes to this bug.