High resolution scrolling should be enabled even when scrolling speed is customized by prefs

RESOLVED FIXED in mozilla8



8 years ago
2 months ago


(Reporter: masayuki, Assigned: masayuki)


Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)

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.
Posted patch Patch v1.0 (obsolete) — Splinter Review
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.
Posted patch Patch v1.0.1 (obsolete) — Splinter Review
Attachment #533610 - Attachment is obsolete: true
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?
Posted patch Patch v1.0.2Splinter Review
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.
Attachment #534274 - Attachment is obsolete: true
Attachment #543554 - Flags: review?(jmathies)
Attachment #543554 - Flags: review?(Olli.Pettay)
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?
(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.
Our wheel events are quite close to DOM 3 wheel events ;)
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
Attachment #543554 - Flags: review?(Olli.Pettay) → review+


8 years ago
Attachment #543554 - Flags: review?(jmathies) → review+

Comment 10

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.