I think that we can relatively easily implement D3E's WheelEvent. All widgets should dispatch a new internal event for each native event. And ESM should generate old mouse scroll event and pixel scroll event for compatibility. Now, I'm working on refactoring the complex implementation on Windows (bug 672175). After that, maybe, I can fix this bug.

Smaug: There are only the DOM events class whose names are nsDOM*Event. Which is better for the new D3E wheel event, nsDOMWheelEvent or mozilla::events::DOMWheelEvent (or mozilla::DOMWheelEvent)?

do we have mozilla::events for anything else? If not, lets not add new namespaces. mozilla::dom should be fine. So, perhaps mozilla::dom::DOMWheelEvent

OK, sounds reasonable. Thanks.

Hmm, I have a trouble. DOM 3 Events defines the type of delta values as float. I don't find the definition of float in DOM spec. Our Javascript engine always uses double for float. Therefore, if we used float for the delta attributes, it would cause error of casting float to double. E.g., following function returned false. function () { var event = new WheelEvent("wheel", { deltaX: 1.0 }); return (event.deltaX == 1.0); } the actual deltaX may be 1.000000xxxxxxxxx (x means random number). When I defined them as double, the problem has gone. So, I think that we need to use double for them.

Yeah, double sounds good.

Comment on attachment 606531 [details] [diff] [review] part.1 Add DOM3 WheelEvent > - elif realtype.count("PRUint16"): > + elif realtype.count("PRUint32") or realtype.count("PRUint16"): > fd.write(" uint32_t u;\n") > fd.write(" NS_ENSURE_STATE(JS_ValueToECMAUint32(aCx, v, &u));\n") > fd.write(" aDict.%s = u;\n" % a.name) Extra assignment is not required for PRUint32. > + elif realtype.count("double"): > + fd.write(" MOZ_ALWAYS_TRUE(JS_ValueToNumber(aCx, v, &aDict.%s));\n" % a.name) Use NS_ENSURE_STATE. JS_ValueToNumber can fail. It should be: + elif realtype.count("PRUint32"): + fd.write(" NS_ENSURE_STATE(JS_ValueToECMAUint32(aCx, v, &aDict.%s));\n" % a.name) + elif realtype.count("double"): + fd.write(" NS_ENSURE_STATE(JS_ValueToNumber(aCx, v, &aDict.%s));\n" % a.name) See codegen.py, qsgen.py, and JSData2Native to verify how to convert various types. https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/codegen.py#98 https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/qsgen.py#428 https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#395

> E.g., following function returned false. > function () { > var event = new WheelEvent("wheel", { deltaX: 1.0 }); > return (event.deltaX == 1.0); > } I couldn't reproduce it with the attached patch. > var event = document.createEvent("WheelEvent"); > event.initWheelEvent("wheel", false, false, null, null, 0, 0, 0, 0, 0, null, null, 0.1, 0, 0, 0); > event.deltaX == 0.1 Indeed returns false. However, I think we should follow the spec here because: - It's consistent with IE9/10. - Sooner or later, JS programmers will have to know about floating-point numbers precision. Single-precision floating-point numbers will be used more and more in specs. (WebGL, Typed Array, etc.)

Forgot qrefresh

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4) > DOM 3 Events defines the type of delta values as float. I don't find the > definition of float in DOM spec. > > Our Javascript engine always uses double for float. This is not implementation defined but well defined in ECMA-262 and Web IDL. It is an expected behavior for the following function to return false: function () { var event = new WheelEvent("wheel", { deltaX: 0.1 }); return (event.deltaX == 0.1); }

Test fix added

Kimura-san, thank you for your work. I think IE 9 doesn't use float for the delta values. Perhaps, I suggested the type of delta values after IE 9 was released (at implementing high resolution mouse wheel event handling on Windows). I've not tested on IE 10. And I don't think that using float is better than using double because looks like using Float32Array is pretty redundant... Hmm...

So, there are two options: 1. Use float because comparing delta value is not real situation. 2. Use double for avoiding confusion of web developers.

float vs. double issue was discussed long ago in a DOM Events conf call, but I don't recall now what was decided and why. http://www.w3.org/2008/webapps/track/issues/177 If no one else implement it as a float, we could still change the spec to use double.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12) > I think IE 9 doesn't use float for the delta values. Ah, you are right. IE9 uses integer for delta values. > I've not tested on IE 10. In my testing, IE10 uses single-precision floating-point number for delta. > And I don't think that using float is better than using double because looks > like using Float32Array is pretty redundant... Hmm... In general, floating-point values should not be compared with expressions such as "f1 == f2". It should be "Math.abs(f1 - f2) < 0.000001" if required. Also I think it's unlikely to compare delta values in real-world. (In reply to Olli Pettay [:smaug] from comment #14) > If no one else implement it as a float, we could still change the spec to > use double. IE10 already implements it as a float.

Well, IE10 is not released yet ;)

(In reply to Masatoshi Kimura [:emk] from comment #10) > > Our Javascript engine always uses double for float. > This is not implementation defined but well defined in ECMA-262 and Web IDL. If JS's fractional number and Web IDL's double are always same, I think that it doesn't make sense to use "float" for delta values. Using double makes simpler Web application's code. Additionally, the size difference isn't problem because typically allocated event is deleted after it's dispatched. So, it seems that using double in D3E spec is better. I have no idea about its demerit.

I'm rewriting ESM. I think that we should discard *all* mousewheel settings which are handled by ESM. Nowadays, replacing scrolling delta values with pref values doesn't make sense. Coming delta values are not same always. On the other hand, I understand some platform's users want to customize it like Linux. I think that each widget should implement the prefs for overriding their system settings, like Windows. http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinMouseScrollHandler.cpp#1175 So, I'll remove all mousewheel.*.sysnumlines, mousewheel.*.numlines. And I need to change the .action prefs. They should only specify "scroll", "history", "zoom" or "none". I.e., scrolling by lines, pixels and pages shouldn't be specified by this pref. And the .action shouldn't be separated by the difference between vertical or horizontal because new wheel event can handle both events at once.

Is there enough information from the system to differentiate devices? Looking at WheelEvent it looks like a trackpad or smooth scroll wheel may want to use DOM_DELTA_PIXEL while a mouse with a non-smooth scroll wheel may want to use DOM_DELTA_LINE or DOM_DELTA_PAGE.

(In reply to Daniel Friesen from comment #19) > Is there enough information from the system to differentiate devices? No. It's difficult for native applications too on most platforms. > Looking at WheelEvent it looks like a trackpad or smooth scroll wheel may > want to use DOM_DELTA_PIXEL while a mouse with a non-smooth scroll wheel may > want to use DOM_DELTA_LINE or DOM_DELTA_PAGE. It depends on the platform's native event. On Mac, some pointing devices provide delta values in pixels, the others provide it in lines. On Windows, the delta value means lines or pages (but I'm not sure for tablet PC). On Linux, it always means lines. Even if it means lines, it may cause smooth scrolling, e.g., on Windows. Please be aware that the delta values are not integer.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7df866b71e58 I'll request reviews after I'll be back.

Updated for latest trunk. This patch adds mozilla::dom::DOMWheelEvent in content/events/src and mozilla::widget::WheelEvent in widget. And also, it adds "case NS_WHEEL_EVENT" next to "case NS_MOUSE_SCROLL_EVENT". The "onwheel" attribute is for XUL for now.

Comment on attachment 618960 [details] [diff] [review] part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText() This patch separates nsEventStateManager::DoScrollText() to 2 methods. One is just computing the scroll target frame. The other modifiers the mouse wheel transaction and scroll the given frame actually. I'll attach -w diff.

Comment on attachment 618961 [details] [diff] [review] part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events The scroll target for default action is computed with some additional checks such as whether it's scrollable for the direction, whether it's not overflow:hidden and honoring mouse wheel transaction. On the other hand, legacy event's detail (delta value) is computed from nearest scrollable element. This patch makes nsEventStateManager::ComputeScrollTarget() usable for computing the nearest scrollable element for computing the detail value of legacy mouse scroll events. The actual scroll amount is computed by nsEventStateManager::GetScrollAmount().

Comment on attachment 618962 [details] [diff] [review] part.4 Add reverting delta value prefs instead of customizing number of scroll lines prefs This patch removes the prefs for customizing legacy mouse scroll event's detail value. D3E WheelEvent has 3 delta modes such as PIXEL, LINE and PAGE. And the delta values are double. So, we will fully support high-resolution scrolling. So, specifying delta values don't make sense for now. But user may want to customize the scroll amount actually. I think that it should be possible on each widget. Therefore, I think these prefs are not necessary for ESM at least. On the other hand, ESM should be possible to change the delta value direction ( *= -1) because it's impossible on every platform. If somebody customized the prefs as negative values, it would be a big regression for them if we don't provide these prefs. And also, I'd like to change the rules for naming mousewheel prefs. Currently, we're deciding a used pref with following code. > 362 if (aEvent->IsShift()) { > 363 aPref.Append(withshift); > 364 } else if (aEvent->IsControl()) { > 365 aPref.Append(withcontrol); > 366 } else if (aEvent->IsAlt()) { > 367 aPref.Append(withalt); > 368 } else if (aEvent->IsMeta()) { > 369 aPref.Append(withmetakey); > 370 } else { > 371 aPref.Append(withno); > 372 } This code doesn't make sense if two or more modifiers are pressed. I think that only when a modifier key is pressed, it takes a special action. Otherwise, the "default" action should be performed. And changing the naming rules allow to sync profiles of both ESR10 and newer Fx.

Comment on attachment 618963 [details] [diff] [review] part.5 Don't separate wheel action prefs by direction D3E WheelEvent can indicate oblique scrolling. On Mac, the native event can indicate oblique scrolling too. So, it doesn't make sense we keep two separated action prefs for vertical and horizontal. We should provide only one pref for actions. And also, it shouldn't be able to specify the deltaMode for scrolling action. It doesn't make sense.

Comment on attachment 618964 [details] [diff] [review] part.6 Separate pixel delta value accumulation code This patch separates the delta accumulation code from PreHandleEvent(). It will be useful for D3E wheel event's pixel scroll events.

Comment on attachment 618966 [details] [diff] [review] part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM NS_QUERY_SCROLL_TARGET_INFO event isn't necessary after we implement D3E WheelEvent. This patch drops the handler from ESM (from widget is preformed later).

Comment on attachment 618967 [details] [diff] [review] part.8 Replace legacy mouse scroll event handlers with D3E wheel event handler This patch is actual implementation of D3E WheelEvent in ESM. This patch add intDeltaX and intDeltaY to widget::WheelEvent. They are set line scroll amount by widget. Except the gesture handler for Windows, native event tells us when we should dispatch legacy line scroll event. When this isn't zero, PoseHandleEvent() dispatches legacy line scroll event. And also, if intDeltaX and intDeltaY are never sets non-zero, the dispatcher sets isPixelOnlyDevice to true. Then, PixelDeltaAccumulator which is used by PreHandleEvent() accumulates pixel delta values for setting intDeltaX and intDeltaY. If the value becomes over 1 or less -1, it sets intDelta values automatically. History and Zoom should be performed only when the intDelta value isn't 0. Finally, DoScrollText() always calls nsIScrollableFrame::ScrollBy() with DEVICE_PIXELS. The amount is computed with the result of GetScrollAmount() and event's delta value and event's deltaMode value. By passing the "origin", it shouldn't break the new smooth scrolling. I'll request other reviews after part.8's review is granted. FYI: A lot of behavior which is implemented by this patch will be tested by part.10.

Comment on attachment 621863 [details] [diff] [review] part.1 Add DOM3 WheelEvent >@@ -623,16 +623,20 @@ NON_IDL_EVENT(draggesture, > NON_IDL_EVENT(overflow, > NS_SCROLLPORT_OVERFLOW, > EventNameType_XUL, > NS_EVENT_NULL) > NON_IDL_EVENT(underflow, > NS_SCROLLPORT_UNDERFLOW, > EventNameType_XUL, > NS_EVENT_NULL) >+NON_IDL_EVENT(wheel, >+ NS_WHEEL_WHEEL, >+ EventNameType_XUL, >+ NS_WHEEL_EVENT) Ok for now, but there could be a followup to add this to idl and html elements. >@@ -829,16 +834,18 @@ nsEventDispatcher::CreateEvent(nsPresCon > // And if we didn't get an event, check the type argument. > > if (aEventType.LowerCaseEqualsLiteral("mouseevent") || > aEventType.LowerCaseEqualsLiteral("mouseevents") || > aEventType.LowerCaseEqualsLiteral("popupevents")) > return NS_NewDOMMouseEvent(aDOMEvent, aPresContext, nsnull); > if (aEventType.LowerCaseEqualsLiteral("mousescrollevents")) > return NS_NewDOMMouseScrollEvent(aDOMEvent, aPresContext, nsnull); >+ if (aEventType.LowerCaseEqualsLiteral("wheelevent")) >+ return NS_NewDOMWheelEvent(aDOMEvent, aPresContext, nsnull); I think we don't need this. >+ void initWheelEvent(in DOMString typeArg, >+ in boolean canBubbleArg, >+ in boolean cancelableArg, >+ in nsIDOMWindow viewArg, >+ in long detailArg, >+ in long screenXArg, >+ in long screenYArg, >+ in long clientXArg, >+ in long clientYArg, >+ in unsigned short buttonArg, >+ in nsIDOMEventTarget relatedTargetArg, >+ in DOMString modifiersListArg, >+ in double deltaXArg, >+ in double deltaYArg, >+ in double deltaZArg, >+ in unsigned long deltaMode); Make this [noscript]. The spec doesn't have init* anymore. Looks good, but could you update the test to not test initWheelEvent. I could look at them then.

Comment on attachment 618960 [details] [diff] [review] part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText() >+ >+ /** >+ * ComputeScrollTarget() returns a scrollable frame which should be >+ * scrolled. the scrollable frame >+ * >+ * @param aTargetFrame The event target of wheel event. of the wheel event >+ * @param aEvent The handling mouse wheel event. of the wheel event This patch could land separately, right?

Comment on attachment 618961 [details] [diff] [review] part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events > /** > * ComputeScrollTarget() returns a scrollable frame which should be > * scrolled. > * > * @param aTargetFrame The event target of wheel event. > * @param aEvent The handling mouse wheel event. >+ * @param aForDefaultAction Whether this uses wheel transaction or not. >+ * If true, returns the latest scrolled frame if >+ * there is it. Otherwise, the nearest ancestor >+ * scrollable frame from aTargetFrame. > * @return The scrollable frame which should be scrolled. > */ aForDefaultAction is a bit odd name, but I couldn't come up with anything better. >+ /** >+ * GetScrollAmount() returns the scroll amount in app units of one line or >+ * one page. If the mouse scroll event scroll a page, returns the page width scrolls

Thank you, smaug.

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent Could you review dictionary_helper_gen.py part?

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent D3E Spec defines deltaX, deltaY and deltaZ as float. But if we do so, there are some issues, see comment 4 and latter comments (my example in comment 4 is wrong, replace "1.0" with "1/3" or something). Therefore, this patch uses double for them.

(In reply to Olli Pettay [:smaug] from comment #54) > Comment on attachment 618960 [details] [diff] [review] > part.2 Separate code for deciding scroll target from > nsEventStateManager::DoScrollText() > > >+ * @param aEvent The handling mouse wheel event. > of the wheel event ?? I ignore this... > This patch could land separately, right? Yeah, but I think that we shouldn't do so actually. For the risk. I want to land all patches for this bug in early stage of 16.

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent Redirecting to Olli, since he will probably get to this before I can.

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent khuey: He's done already :-) I want you to confirm the dictionary part.

Comment on attachment 618962 [details] [diff] [review] part.4 Add reverting delta value prefs instead of customizing number of scroll lines prefs >-nsEventStateManager::GetScrollLinesFor(nsMouseScrollEvent* aMouseEvent) >-{ >- NS_ASSERTION(!UseSystemScrollSettingFor(aMouseEvent), >- "GetScrollLinesFor() called when should use system settings"); >- nsCAutoString prefName; >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); >- prefName.Append(".numlines"); >- return Preferences::GetInt(prefName.get()); >-} >- >-bool >-nsEventStateManager::UseSystemScrollSettingFor(nsMouseScrollEvent* aMouseEvent) >-{ >- nsCAutoString prefName; >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); >- prefName.Append(".sysnumlines"); >- return Preferences::GetBool(prefName.get()); >-} So what handles these cases with new prefs?

Comment on attachment 618964 [details] [diff] [review] part.6 Separate pixel delta value accumulation code >-static PRUint32 gPixelScrollDeltaTimeout = 0; >+PRInt32 nsEventStateManager::PixelDeltaAccumulator::sX = 0; >+PRInt32 nsEventStateManager::PixelDeltaAccumulator::sY = 0; >+TimeStamp nsEventStateManager::PixelDeltaAccumulator::sLastTime; I was told that this kind of static initializers should be avoided (TimeStamp has a ctor) See for example bug 569629 I don't quite understand the details. Could we perhaps keep PixelDeltaAccumulator in heap and create the object in ESM ctor when first ESM is created.

Comment on attachment 618966 [details] [diff] [review] part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM I guess some other patch explains why this can be dropped.

Comment on attachment 626732 [details] [diff] [review] part.8 Replace legacy mouse scroll event handlers with D3E wheel event handler >+ // If the event don't scroll to both X and Y, we don't need to do here. s/don't/doesn't/ we don't need to do here what? I guess anything >- bool isTrusted = (aEvent->flags & NS_EVENT_FLAG_TRUSTED) != 0; >- nsMouseScrollEvent event(isTrusted, NS_MOUSE_SCROLL, nsnull); >+ nsMouseScrollEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_SCROLL, >+ aEvent->widget); >+ if (*aStatus == nsEventStatus_eConsumeNoDefault) { >+ event.flags |= NS_EVENT_FLAG_NO_DEFAULT; >+ } Why is this needed? >- bool isTrusted = (aEvent->flags & NS_EVENT_FLAG_TRUSTED) != 0; >- nsMouseScrollEvent event(isTrusted, NS_MOUSE_PIXEL_SCROLL, nsnull); >+ nsMouseScrollEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_PIXEL_SCROLL, >+ aEvent->widget); >+ if (*aStatus == nsEventStatus_eConsumeNoDefault) { >+ event.flags |= NS_EVENT_FLAG_NO_DEFAULT; >+ } ditto I rarely ask this, but could you split this to smaller pieces. This is hard to review.

(In reply to Olli Pettay [:smaug] from comment #67) > Comment on attachment 618962 [details] [diff] [review] > part.4 Add reverting delta value prefs instead of customizing number of > scroll lines prefs > > > >-nsEventStateManager::GetScrollLinesFor(nsMouseScrollEvent* aMouseEvent) > >-{ > >- NS_ASSERTION(!UseSystemScrollSettingFor(aMouseEvent), > >- "GetScrollLinesFor() called when should use system settings"); > >- nsCAutoString prefName; > >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); > >- prefName.Append(".numlines"); > >- return Preferences::GetInt(prefName.get()); > >-} > >- > >-bool > >-nsEventStateManager::UseSystemScrollSettingFor(nsMouseScrollEvent* aMouseEvent) > >-{ > >- nsCAutoString prefName; > >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); > >- prefName.Append(".sysnumlines"); > >- return Preferences::GetBool(prefName.get()); > >-} > So what handles these cases with new prefs? The new delta reverting prefs just switch the scroll direction, not change the scroll amount but the old prefs changed line scroll events to page scroll events. Therefore, the old prefs make some problems with acceleration and deciding its default action but the new ones don't so. I think that for some users who want to customize scroll speed only on Fx, we should provide other prefs in widget level. E.g., on Windows, scroll amount is computed from ((native delta value) / 120 * (scroll amount per 120)). The native delta value can be various values if the mouse supports high-resolution scroll. So, current prefs, which replacing delta values with fixed values, don't make sense for high resolution scroll supporting devices. Similarly, on Mac, some devices cause line scroll, but the others cause pixel scroll. And the delta values are specified by OS directly and the value can be various. Although I don't have good idea for customizing the scrolling speed on Mac, but I think replacing delta values doesn't make sense. Note that part.12 (Win) and part.13 (GTK) have new prefs for customizing scrolling speed.

(In reply to Olli Pettay [:smaug] from comment #69) > Comment on attachment 618966 [details] [diff] [review] > part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM > > I guess some other patch explains why this can be dropped. The event is used only by the widget for Windows. The event returns line height, page height and page width of the default action's scroll target. For supporting high resolution scrolling, the widget dispatches pixel scroll event whose delta value is computed with the result. However, new D3E WheelEvent supports high resolution line/page scroll by default (because the delta values are fraction). Therefore, the widget don't need to dispatch pixel wheel event after this bug. So, we don't need the event after this.

(In reply to Olli Pettay [:smaug] from comment #70) > Comment on attachment 626732 [details] [diff] [review] > part.8 Replace legacy mouse scroll event handlers with D3E wheel event > handler > I rarely ask this, but could you split this to smaller pieces. This is hard > to review. Hmm, okay, I'll separate them by purposes. However, some of separated patches could not make sense and be able to build.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #71) > (In reply to Olli Pettay [:smaug] from comment #67) > > Comment on attachment 618962 [details] [diff] [review] > > part.4 Add reverting delta value prefs instead of customizing number of > > scroll lines prefs > > > > > > >-nsEventStateManager::GetScrollLinesFor(nsMouseScrollEvent* aMouseEvent) > > >-{ > > >- NS_ASSERTION(!UseSystemScrollSettingFor(aMouseEvent), > > >- "GetScrollLinesFor() called when should use system settings"); > > >- nsCAutoString prefName; > > >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); > > >- prefName.Append(".numlines"); > > >- return Preferences::GetInt(prefName.get()); > > >-} > > >- > > >-bool > > >-nsEventStateManager::UseSystemScrollSettingFor(nsMouseScrollEvent* aMouseEvent) > > >-{ > > >- nsCAutoString prefName; > > >- GetBasePrefKeyForMouseWheel(aMouseEvent, prefName); > > >- prefName.Append(".sysnumlines"); > > >- return Preferences::GetBool(prefName.get()); > > >-} > > So what handles these cases with new prefs? > > The new delta reverting prefs just switch the scroll direction, not change > the scroll amount but the old prefs changed line scroll events to page > scroll events. Therefore, the old prefs make some problems with acceleration > and deciding its default action but the new ones don't so. > > I think that for some users who want to customize scroll speed only on Fx, > we should provide other prefs in widget level. E.g., on Windows, scroll > amount is computed from ((native delta value) / 120 * (scroll amount per > 120)). The native delta value can be various values if the mouse supports > high-resolution scroll. So, current prefs, which replacing delta values with > fixed values, don't make sense for high resolution scroll supporting > devices. Similarly, on Mac, some devices cause line scroll, but the others > cause pixel scroll. And the delta values are specified by OS directly and > the value can be various. Although I don't have good idea for customizing > the scrolling speed on Mac, but I think replacing delta values doesn't make > sense. > > Note that part.12 (Win) and part.13 (GTK) have new prefs for customizing > scrolling speed. And if user customized the prefs, nsMouseScrollEvent::customizedByUserPrefs would be true. Then, handlers can decide anything from the value like checking the result of UseSystemScrollSettingFor().

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent sr=jst, and if this is needed for the next uplift, please go ahead and land, Kyle is on vacation this whole week, and he can look later if needed. I did look over the dictionary changes, and it looks good to me.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #75) > Comment on attachment 626661 [details] [diff] [review] > part.1 Add DOM3 WheelEvent > > sr=jst, and if this is needed for the next uplift, please go ahead and land, > Kyle is on vacation this whole week, and he can look later if needed. I did > look over the dictionary changes, and it looks good to me. Thank you, but no problem. This bug should be fixed in early stage due to big change.

Comment on attachment 626661 [details] [diff] [review] part.1 Add DOM3 WheelEvent Canceling the dictionary part review due to jst confirmation. But you're welcome to marking r as minus if you found something wrong.

(In reply to Olli Pettay [:smaug] from comment #70) > Comment on attachment 626732 [details] [diff] [review] > part.8 Replace legacy mouse scroll event handlers with D3E wheel event > handler > >- bool isTrusted = (aEvent->flags & NS_EVENT_FLAG_TRUSTED) != 0; > >- nsMouseScrollEvent event(isTrusted, NS_MOUSE_SCROLL, nsnull); > >+ nsMouseScrollEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_SCROLL, > >+ aEvent->widget); > >+ if (*aStatus == nsEventStatus_eConsumeNoDefault) { > >+ event.flags |= NS_EVENT_FLAG_NO_DEFAULT; > >+ } > Why is this needed? I checked current ESM behavior again. Then, the dispatchers are not set them. I think that current behavior is wrong. If an event in a series of events is consumed, other events should be consumed or not dispatched, right? Currently, our keypress event is consumed at dispatch if keydown event was consumed. So, I think that legacy mouse scroll events should be same (for compatibility, we should dispatch but consume it).

So, I think that MozMousePixelScroll is default action of DOMMouseScroll. And both of them are default action of D3E WheelEvent.

LimitToOnePageScroll() is removed by this patch. Same check will be implemented by part.8-4 in nsEventStateManager::DoScrollText().

This patch implements simple wheel event handler which only supports scroll for the default action. There are two |#if 0|, they are not necessary for the simple event handling, but they cause bustage, so, temporarily disable them.

This makes legacy mouse events dispatched. This patch still consume following pixel event if mouse scroll event is consumed. If you still think they're not necessary, I'll remove them.

Windows' gesture causes wheel events which cannot specify intDelta values because there is no information about lines. This patch supports such pixel scroll only devices. The intDelta values are initialized by the accumulator.

This patch implements moving history and zooming in/out for the default action of wheel event. If you have better idea of widget::WheelEvent::GetPreferredIntDelta(), let me know.

Current implementation allows widget to specify scroll type which is, whether it allows smooth scrolling or not. This patch allows that.

Comment on attachment 628948 [details] [diff] [review] part.6 Separate pixel delta value accumulation code >+ PRInt32 numLines; >+ if (isHorizontal) { >+ mX += aEvent->delta; >+ numLines = >+ static_cast<PRInt32>(NS_round(static_cast<double>(mX) / pixelsPerLine)); >+ mX -= numLines * pixelsPerLine; >+ } else { >+ mY += aEvent->delta; >+ numLines = >+ static_cast<PRInt32>(NS_round(static_cast<double>(mY) / pixelsPerLine)); >+ mY -= numLines * pixelsPerLine; >+ } This is somewhat hard to follow. Some comment would be good.

Comment on attachment 628961 [details] [diff] [review] part.8-6 Init intDeltaX and intDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device intDelta is strange name. (It is added in some other patch.) Perhaps lineDelta?

Comment on attachment 628966 [details] [diff] [review] part.8-9 Handle WheelEvent.deltaX in ESM this is about deltaZ, not deltaX

Comment on attachment 628963 [details] [diff] [review] part.8-7 Implement the other default actions of WheelEvent >+ >+ /** >+ * Computes the action for the aEvent with prefs. The result is >+ * one of WheelAction. >+ * >+ * @param aUseSystemSettings Set the result of UseSystemScrollSettingFor(). >+ */ >+ WheelAction ComputeWheelActionFor(mozilla::widget::WheelEvent* aEvent); There is no param 'aUseSystemSettings' >+ >+ /** >+ * Gets the wheel action for the aMouseEvent ONLY with the pref. >+ * When you actually do something for the event, probably you should use >+ * ComputeWheelActionFor(). >+ */ >+ WheelAction GetWheelActionPrefFor(mozilla::widget::WheelEvent* aEvent); > Could you somehow improve the comments of these two methods. It is hard to understand the difference between them.

Comment on attachment 628955 [details] [diff] [review] part.8-3 Redesign nsMouseWheelTransaction for D3E WheelEvent >+struct DeltaValues { { should be in its own line Good stuff :)

Comment on attachment 628959 [details] [diff] [review] part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the wheel event isn't consumed >+ /** >+ * DeltaDirection is used for specifying whether the called method should >+ * handle vertical delta or horizontal delta. >+ * This is clearer than using bool. >+ */ >+ enum DeltaDirection { { should be on its own line >+ // If widget sets intDelta, nsEventStateManager will dispatch NS_MOUSE_SCROLL >+ // event for compatibility. >+ PRInt32 intDeltaX; >+ PRInt32 intDeltaY; These are oddly named. lineDeltaX/Y perhaps Also, shouldn't ESM always try to dispatch DOMMouseScroll. That is what all the web pages expect from Gecko now. Or do all the widget implementations set these deltas?

(In reply to Olli Pettay [:smaug] from comment #96) > Comment on attachment 628959 [details] [diff] [review] > part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the > wheel event isn't consumed > >+ // If widget sets intDelta, nsEventStateManager will dispatch NS_MOUSE_SCROLL > >+ // event for compatibility. > >+ PRInt32 intDeltaX; > >+ PRInt32 intDeltaY; > These are oddly named. lineDeltaX/Y perhaps "line" doesn't sounds good for page scroll but it's ok for me. Do you think it's not problem? > Also, shouldn't ESM always try to dispatch DOMMouseScroll. That is what all > the web pages expect from Gecko now. Did you mean that we should dispatch DOMMouseScroll even if its detail value is zero? If so, I don't think so. Both widgets of Mac and Windows don't dispatch NS_MOUSE_SCROLL event if detail value is less than 0. > Or do all the widget implementations set these deltas? Almost yes. On Mac, native mouse wheel event has both delta values for lines and pixels. If lines are not zero, part.14 sets the values. On Windows, native mouse wheel messages only have wheel turned distance as the delta value. Actual scroll amount for 120 of the delta values are defined by registry. Our widget for Windows also accumulates native delta values and when it becomes over one line (or page), part.12 sets the integer values. But on Windows, native gesture event handler only know how much pixels should be scrolled current gesture. In this case, the handler always sets 0 for the values. And delta accumulator in ESM will compute the values from scroll target's line height. On the other platforms, deltaX/deltaY and these values are always same because they don't have high-resolution scroll.

See comment 71 and comment 74 for my replies to your first review.

See comment 97.

I removed |GetWheelActionPrefFor()| because the user is only |ComputeWheelActionFor()|. I think that it is better to prevent confusion than the old code.