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)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: LA8H9bjHCQZy, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
17.08 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Blocks: 719320
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Product: Firefox → Core
Assignee | ||
Comment 1•12 years ago
|
||
Hmm, I'm surprised. If mouse wheel caused over one page once, it was a bug :-)
Assignee | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 2•12 years ago
|
||
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...
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
So why doesn't mousewheel.with_shift.action = 1 and mousewheel.with_shift.delta_multiplier_y = 99999999 work?
Assignee | ||
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=61e5f595e996
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 674560 [details] [diff] [review] Patch Oops, sorry, this is old patch.
Attachment #674560 -
Flags: review?(bugs)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #674560 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 674595 [details] [diff] [review] Patch This is the latest patch.
Attachment #674595 -
Flags: review?(bugs)
Comment 14•12 years ago
|
||
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-
Assignee | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
Okay, then, it can be simple.
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #674595 -
Attachment is obsolete: true
Attachment #676060 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #676060 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7546a81c1c38
Target Milestone: --- → mozilla19
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7546a81c1c38
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•