Closed Bug 801101 Opened 12 years ago Closed 12 years ago

Needs a option to scroll over one page

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: LA8H9bjHCQZy, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 719320
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Product: Firefox → Core
Hmm, I'm surprised. If mouse wheel caused over one page once, it was a bug :-)
Severity: normal → enhancement
Component: DOM: Events → Event Handling
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: New mousewheel scrolling interface is terrible → Needs a option to scroll over one page
Version: 17 Branch → Trunk
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...
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.
So why doesn't mousewheel.with_shift.action = 1 and mousewheel.with_shift.delta_multiplier_y = 99999999
work?
(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.
(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.
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.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
Attachment #674560 - Flags: review?(bugs)
Comment on attachment 674560 [details] [diff] [review]
Patch

Oops, sorry, this is old patch.
Attachment #674560 - Flags: review?(bugs)
Attached patch Patch (obsolete) — Splinter Review
Attachment #674560 - Attachment is obsolete: true
Comment on attachment 674595 [details] [diff] [review]
Patch

This is the latest patch.
Attachment #674595 - Flags: review?(bugs)
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?
Attachment #674595 - Flags: review?(bugs) → review-
(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?
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.
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?
(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.
Okay, then, it can be simple.
Attached patch PatchSplinter Review
Attachment #674595 - Attachment is obsolete: true
Attachment #676060 - Flags: review?(bugs)
Attachment #676060 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/7546a81c1c38
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 817979
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: