Last Comment Bug 658155 - High resolution scrolling should be enabled even when scrolling speed is customized by prefs
: High resolution scrolling should be enabled even when scrolling speed is cust...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: x86 Windows 7
: -- enhancement (vote)
: mozilla8
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks: 605648 657634
  Show dependency treegraph
 
Reported: 2011-05-18 21:00 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2013-12-27 14:28 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (15.74 KB, patch)
2011-05-19 05:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v1.0.1 (16.12 KB, patch)
2011-05-21 22:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch v1.0.2 (16.17 KB, patch)
2011-07-01 18:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
bugs: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 21:00:39 PDT
This is a follow up bug for bug 657634.

We should return the computed delta value to nsWindow. And nsWindow should prefer it at dispatching pixel scroll event.

This shouldn't block bug 658111. This is actually a new feature.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-19 05:21:42 PDT
Created attachment 533610 [details] [diff] [review]
Patch v1.0

This makes high resolution scrolling can be used with user customized scrolling speed.

This returns the computed scroll amount with its unit. nsWindow computes the pixel scroll event delta value with the information.

This is riskier than other patches because this changed the main logic in nsWindow::OnMouseWheel() and this feature isn't used by most users. So, I think this should be landed for Fx7.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-21 22:52:56 PDT
Created attachment 534274 [details] [diff] [review]
Patch v1.0.1
Comment 3 Asa Dotzler [:asa] 2011-06-30 12:48:36 PDT
Masayuki, is this feature going to be able to get reviewed and landed before next Tuesday? It would be nice to have this along with the other pixel scrolling changes in Firefox 7.  Also, will pixel scrolling be on or off by default in 7?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-07-01 18:13:34 PDT
Created attachment 543554 [details] [diff] [review]
Patch v1.0.2

This patch also returns computed scroll action to widget. E.g., if user customized wheel scroll action without any modifier keys from line scrolling to page scrolling, the pixel event delta value would be computed from page width/height.

Asa: I'm sorry I forgot to request review this. However, I don't think this should go to Fx7 at this timing. This is a little risky. If you wanted that and this patch passed reviews quickly, I would try to land this.
Comment 5 Olli Pettay [:smaug] 2011-07-04 07:07:09 PDT
Comment on attachment 543554 [details] [diff] [review]
Patch v1.0.2

So in which case is sQueryContentEvent::SCROLL_ACTION_NONE used?
And how does that affect to scrolling and to dispatched events?
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-07-04 09:14:54 PDT
(In reply to comment #5)
> Comment on attachment 543554 [details] [diff] [review] [review]
> Patch v1.0.2
> 
> So in which case is sQueryContentEvent::SCROLL_ACTION_NONE used?

Currently, it shouldn't be used because DoScrollText() is called for query only when the action is line scroll or page scroll. SCROLL_ACTION_NONE means illegal case actually.

> And how does that affect to scrolling and to dispatched events?

Good point, currently, we don't dispatch pixel event from nsWindow if user customized the prefs because the query event failed always.  So, the delta value should be same as computed value of SendLineScrollEvent() in most cases.  However, SendLineScrollEvent() computes the delta value with event target but nsWindow computes it with actual scroll target.  So, the delta value might be different.  The root cause of this difference is that we need to compute the count of pixels with logical unit (line/page) and the scroll target may be different by wheel scroll transaction.  I think that we cannot fix this issue now because we need to implement DOM3 wheel event and drop our strange wheel events.
Comment 7 Olli Pettay [:smaug] 2011-07-13 04:21:54 PDT
Our wheel events are quite close to DOM 3 wheel events ;)
Comment 8 Olli Pettay [:smaug] 2011-07-13 04:33:27 PDT
Comment on attachment 543554 [details] [diff] [review]
Patch v1.0.2

>+  // Don't modify the test event which in in mInput.
"in in"?

>+  nsMouseScrollEvent msEvent(*aEvent->mInput.mMouseScrollEvent);
This is a bit wrong. In practice it should work here, but if someone
happens to set .target or .relatedTarget or something, the addref/release
of those values break. The object will be released too many times.
So I think you should manually copy the necessary member 
variables from aEvent to msEvent.

With that fixed, r=me
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-07-14 15:15:25 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8025a4d8b7bb

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