Implement DOM3 wheel event

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
7 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla17
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(30 attachments, 124 obsolete attachments)

2.47 KB, text/html
Details
3.13 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.36 KB, patch
Dave Yeo
: review+
Details | Diff | Splinter Review
3.55 KB, patch
smaug
: review+
Details | Diff | Splinter Review
32.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.88 KB, patch
Details | Diff | Splinter Review
5.43 KB, patch
Details | Diff | Splinter Review
4.69 KB, patch
Details | Diff | Splinter Review
60.66 KB, patch
Details | Diff | Splinter Review
15.06 KB, patch
Details | Diff | Splinter Review
18.34 KB, patch
Details | Diff | Splinter Review
33.43 KB, patch
Details | Diff | Splinter Review
24.09 KB, patch
Details | Diff | Splinter Review
10.76 KB, patch
Details | Diff | Splinter Review
9.98 KB, patch
Details | Diff | Splinter Review
7.55 KB, patch
Details | Diff | Splinter Review
17.57 KB, patch
Details | Diff | Splinter Review
34.12 KB, patch
Details | Diff | Splinter Review
23.81 KB, patch
Details | Diff | Splinter Review
18.21 KB, patch
Details | Diff | Splinter Review
16.21 KB, patch
Details | Diff | Splinter Review
6.80 KB, patch
Details | Diff | Splinter Review
2.06 KB, patch
Details | Diff | Splinter Review
14.89 KB, patch
Details | Diff | Splinter Review
320.94 KB, patch
Details | Diff | Splinter Review
69.67 KB, patch
Details | Diff | Splinter Review
19.09 KB, patch
Details | Diff | Splinter Review
3.88 KB, patch
Details | Diff | Splinter Review
10.29 KB, patch
Details | Diff | Splinter Review
2.16 KB, text/html
Details
Created attachment 589763 [details]
testcase

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.
Blocks: 725851
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.
Depends on: 731878
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.
Status: NEW → ASSIGNED
Created attachment 606531 [details] [diff] [review]
part.1 Add DOM3 WheelEvent
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
Created attachment 606950 [details] [diff] [review]
Use float for deltas

> 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.)
Created attachment 606951 [details] [diff] [review]
Use float for deltas

Forgot qrefresh
Attachment #606950 - Attachment is obsolete: true
Attachment #606951 - Attachment is patch: true
(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);
}
Created attachment 606965 [details] [diff] [review]
Use float for deltas

Test fix added
Attachment #606951 - Attachment is obsolete: true
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.
Depends on: 739760

Comment 19

5 years ago
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.
Created attachment 618959 [details] [diff] [review]
part.1 Add DOM3 WheelEvent
Attachment #606531 - Attachment is obsolete: true
Attachment #606965 - Attachment is obsolete: true
Created attachment 618960 [details] [diff] [review]
part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText()
Created attachment 618961 [details] [diff] [review]
part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events
Created attachment 618962 [details] [diff] [review]
part.4 Add reverting delta value prefs instead of customizing number of scroll lines prefs
Created attachment 618963 [details] [diff] [review]
part.5 Don't separate wheel action prefs by direction
Created attachment 618964 [details] [diff] [review]
part.6 Separate pixel delta value accumulation code
Created attachment 618966 [details] [diff] [review]
part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM
Created attachment 618967 [details] [diff] [review]
part.8 Replace legacy mouse scroll event handlers with D3E wheel event handler
Created attachment 618968 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests
Created attachment 618969 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Created attachment 618971 [details] [diff] [review]
part.11 Fix new test failures
Created attachment 618972 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows
Created attachment 618973 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK
Created attachment 618974 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa
Created attachment 618975 [details] [diff] [review]
part.15 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Qt
Created attachment 618976 [details] [diff] [review]
part.16 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on OS/2
Created attachment 618977 [details] [diff] [review]
part.17 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on nsWidgetUtils
Created attachment 618978 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7df866b71e58

I'll request reviews after I'll be back.
Created attachment 618990 [details] [diff] [review]
part.1 Add DOM3 WheelEvent
Attachment #618959 - Attachment is obsolete: true
Created attachment 621863 [details] [diff] [review]
part.1 Add DOM3 WheelEvent

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.
Attachment #618990 - Attachment is obsolete: true
Attachment #621863 - Flags: review?(bugs)
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.
Attachment #618960 - Flags: review?(bugs)
Created attachment 621864 [details] [diff] [review]
part.2 -w
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().
Attachment #618961 - Flags: review?(bugs)
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.
Attachment #618962 - Flags: review?(bugs)
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.
Attachment #618963 - Flags: review?(bugs)
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.
Attachment #618964 - Flags: review?(bugs)
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).
Attachment #618966 - Flags: review?(bugs)
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.
Attachment #618967 - Flags: review?(bugs)
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.
Attachment #621863 - Flags: review?(bugs)
Created attachment 624675 [details] [diff] [review]
part.1 Add DOM3 WheelEvent
Attachment #621863 - Attachment is obsolete: true
Attachment #624675 - Flags: review?(bugs)
Created attachment 624676 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests
Attachment #618968 - Attachment is obsolete: true
Created attachment 624677 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Attachment #618969 - Attachment is obsolete: true
Attachment #624675 - Flags: review?(bugs) → review+
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?
Attachment #618960 - Flags: review?(bugs) → review+
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
Attachment #618961 - Flags: review?(bugs) → review+
Created attachment 626661 [details] [diff] [review]
part.1 Add DOM3 WheelEvent

Thank you, smaug.
Attachment #624675 - Attachment is obsolete: true
Comment on attachment 626661 [details] [diff] [review]
part.1 Add DOM3 WheelEvent

Could you review dictionary_helper_gen.py part?
Attachment #626661 - Flags: review?(khuey)
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.
Attachment #626661 - Flags: superreview?(jst)
Created attachment 626664 [details] [diff] [review]
part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText()

(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.
Attachment #618960 - Attachment is obsolete: true
Created attachment 626669 [details] [diff] [review]
part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events
Attachment #618961 - Attachment is obsolete: true
Attachment #621864 - Attachment is obsolete: true
Created attachment 626732 [details] [diff] [review]
part.8 Replace legacy mouse scroll event handlers with D3E wheel event handler
Attachment #618967 - Attachment is obsolete: true
Attachment #618967 - Flags: review?(bugs)
Attachment #626732 - Flags: review?(bugs)
Created attachment 626733 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Attachment #624677 - Attachment is obsolete: true
Created attachment 626734 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa
Attachment #618974 - Attachment is obsolete: true
Created attachment 626735 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event
Attachment #618978 - Attachment is obsolete: true
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.
Attachment #626661 - Flags: review?(khuey) → review?(bugs)
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.
Attachment #626661 - Flags: review?(bugs) → review?(khuey)
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?
Attachment #618962 - Flags: review?(bugs) → review-
Attachment #618963 - Flags: review?(bugs) → review+
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.
Attachment #618964 - Flags: review?(bugs) → review-
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.
Attachment #618966 - Flags: review?(bugs) → review+
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.
Attachment #626732 - Flags: review?(bugs) → 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.
Attachment #626661 - Flags: superreview?(jst) → superreview+
(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.
Attachment #626661 - Flags: review?(khuey)
(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.
Created attachment 628948 [details] [diff] [review]
part.6 Separate pixel delta value accumulation code
Attachment #618964 - Attachment is obsolete: true
Attachment #628948 - Flags: review?(bugs)
Created attachment 628952 [details] [diff] [review]
part.8-1 Drop legacy mouse scroll events from IPC
Attachment #626732 - Attachment is obsolete: true
Attachment #628952 - Flags: review?(bugs)
Created attachment 628953 [details] [diff] [review]
part.8-2 GetScrollAmount() should return both width and height for oblique wheel event
Attachment #628953 - Flags: review?(bugs)
Created attachment 628955 [details] [diff] [review]
part.8-3 Redesign nsMouseWheelTransaction for D3E WheelEvent

LimitToOnePageScroll() is removed by this patch.

Same check will be implemented by part.8-4 in nsEventStateManager::DoScrollText().
Attachment #628955 - Flags: review?(bugs)
Created attachment 628956 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll

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.
Attachment #628956 - Flags: review?(bugs)
Created attachment 628959 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the wheel event isn't consumed

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.
Attachment #628959 - Flags: review?(bugs)
Created 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

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.
Attachment #628961 - Flags: review?(bugs)
Created attachment 628963 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent

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.
Attachment #628963 - Flags: review?(bugs)
Created attachment 628964 [details] [diff] [review]
part.8-8 widget should be able to speicify the scroll type

Current implementation allows widget to specify scroll type which is, whether it allows smooth scrolling or not. This patch allows that.
Attachment #628964 - Flags: review?(bugs)
Created attachment 628966 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaX in ESM
Attachment #628966 - Flags: review?(bugs)
Created attachment 628969 [details] [diff] [review]
part.8-10 Remove the code handling legacy mouse events in layout
Attachment #628969 - Flags: review?(bugs)
Attachment #628952 - Flags: review?(bugs) → review+
Attachment #628969 - Flags: review?(bugs) → review+
Attachment #628964 - Flags: review?(bugs) → review+
Attachment #628953 - Flags: review?(bugs) → review+
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.
Attachment #628948 - Flags: review?(bugs) → review+
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?
Attachment #628961 - Flags: review?(bugs) → review+
Comment on attachment 628966 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaX in ESM

this is about deltaZ, not deltaX
Attachment #628966 - Flags: review?(bugs) → review+
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.
Attachment #628963 - Flags: review?(bugs) → review-
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 :)
Attachment #628955 - Flags: review?(bugs) → review+
Attachment #628956 - Flags: review?(bugs) → review+
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?
Attachment #628959 - Flags: review?(bugs) → review-
(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.
Created attachment 634776 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)
Attachment #626661 - Attachment is obsolete: true
Created attachment 634777 [details] [diff] [review]
part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText() (r=smaug)
Attachment #626664 - Attachment is obsolete: true
Created attachment 634778 [details] [diff] [review]
part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events (r=smaug)
Attachment #626669 - Attachment is obsolete: true
Created attachment 634779 [details] [diff] [review]
part.4 Add reverting delta value prefs instead of customizing number of scroll lines prefs

See comment 71 and comment 74 for my replies to your first review.
Attachment #618962 - Attachment is obsolete: true
Attachment #634779 - Flags: review?(bugs)
Created attachment 634780 [details] [diff] [review]
part.5 Don't separate wheel action prefs by direction (r=smaug)
Attachment #618963 - Attachment is obsolete: true
Created attachment 634781 [details] [diff] [review]
part.6 Separate pixel delta value accumulation code (r=smaug)
Attachment #628948 - Attachment is obsolete: true
Created attachment 634783 [details] [diff] [review]
part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM (r=smaug)
Attachment #618966 - Attachment is obsolete: true
Created attachment 634784 [details] [diff] [review]
part.8-1 Drop legacy mouse scroll events from IPC (r=smaug)
Attachment #628952 - Attachment is obsolete: true
Created attachment 634785 [details] [diff] [review]
part.8-2 GetScrollAmount() should return both width and height for oblique wheel event (r=smaug)
Attachment #628953 - Attachment is obsolete: true
Created attachment 634786 [details] [diff] [review]
part.8-3 Redesign nsMouseWheelTransaction for D3E WheelEvent (r=smaug)
Attachment #628955 - Attachment is obsolete: true
Created attachment 634787 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)
Attachment #628956 - Attachment is obsolete: true
Created attachment 634788 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the wheel event isn't consumed

See comment 97.
Attachment #628959 - Attachment is obsolete: true
Attachment #634788 - Flags: review?(bugs)
Created attachment 634790 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device (r=smaug)
Attachment #628961 - Attachment is obsolete: true
Created attachment 634792 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent

I removed |GetWheelActionPrefFor()| because the user is only |ComputeWheelActionFor()|. I think that it is better to prevent confusion than the old code.
Attachment #628963 - Attachment is obsolete: true
Attachment #634792 - Flags: review?(bugs)
Created attachment 634793 [details] [diff] [review]
part.8-8 widget should be able to speicify the scroll type (r=smaug)
Attachment #628964 - Attachment is obsolete: true
Created attachment 634794 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #628966 - Attachment is obsolete: true
Created attachment 634795 [details] [diff] [review]
part.8-10 Remove the code handling legacy mouse events in layout (r=smaug)
Attachment #628969 - Attachment is obsolete: true
Created attachment 634796 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests
Attachment #624676 - Attachment is obsolete: true
Attachment #634796 - Flags: review?(bugs)
Created attachment 634797 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Attachment #626733 - Attachment is obsolete: true
Created attachment 634798 [details] [diff] [review]
part.11 Fix new test failures
Attachment #618971 - Attachment is obsolete: true
Created attachment 634799 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows
Attachment #618972 - Attachment is obsolete: true
Created attachment 634800 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK
Attachment #618973 - Attachment is obsolete: true
Created attachment 634801 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa
Attachment #626734 - Attachment is obsolete: true
Created attachment 634802 [details] [diff] [review]
part.15 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Qt
Attachment #618975 - Attachment is obsolete: true
Created attachment 634803 [details] [diff] [review]
part.16 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on OS/2
Attachment #618976 - Attachment is obsolete: true
Created attachment 634804 [details] [diff] [review]
part.17 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on nsWidgetUtils
Attachment #618977 - Attachment is obsolete: true
Created attachment 634805 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event
Attachment #626735 - Attachment is obsolete: true
Attachment #634805 - Flags: review?(bugs)
Comment on attachment 634804 [details] [diff] [review]
part.17 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on nsWidgetUtils

I'm not sure who are using this method. I think smaug's review is enough for such change even if there is another owner.
Attachment #634804 - Flags: review?(bugs)
Comment on attachment 634801 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa

By this patch, there is no user of nsMouseScrollEvent::kIsMomentum.

# Of course, I'll request to review to cocoa widget reviewer later.
Attachment #634801 - Flags: review?(bugs)
Comment on attachment 634799 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows

By this patch, we can completely remove NS_QUERY_SCROLL_TARGET_INFO event.

And also we can remove nsMouseScrollEvent::kNoLines, nsMouseScrollEvent::kNoDefer, nsMouseScrollEvent::kAllowSmoothScroll and nsMouseScrollEvent::kFromLines.
Attachment #634799 - Flags: review?(bugs)
I'll request to Smaug to review for part.10 and part.11 after part.9. But you're welcome if you review them before I'll request them.
Attachment #634794 - Attachment description: part.8-9 Handle WheelEvent.deltaX in ESM (r=smaug) → part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> > 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?
>
Oh, right, it can be page scroll too. then lineDelta isn't good. but int* is just very vague.


> > 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.

Ah, right. No DOMMouseScroll if there isn't really anything to report.
(In reply to Olli Pettay [:smaug] from comment #129)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #97)
> > > 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?
> >
> Oh, right, it can be page scroll too. then lineDelta isn't good. but int* is
> just very vague.

Hmm...

For the purpose, legacyDeltaX/legacyDeltaY might be better. But I think the developers of widgets cannot understand the meaning of them by the names...
Maybe lineOrPageDeltaX/Y ? That is a bit ugly but should be reasonable clear.
(In reply to Olli Pettay [:smaug] from comment #131)
> Maybe lineOrPageDeltaX/Y ? That is a bit ugly but should be reasonable clear.

Okay, I'll replace lineDelta* with them.
Created attachment 634878 [details] [diff] [review]
part.8-3 Redesign nsMouseWheelTransaction for D3E WheelEvent (r=smaug)
Attachment #634786 - Attachment is obsolete: true
Created attachment 634879 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the wheel event isn't consumed
Attachment #634788 - Attachment is obsolete: true
Attachment #634788 - Flags: review?(bugs)
Attachment #634879 - Flags: review?(bugs)
Created attachment 634880 [details] [diff] [review]
part.8-6 Init lineOrPageDeltaX and lineOrPageDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device (r=smaug)
Attachment #634790 - Attachment is obsolete: true
Created attachment 634881 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent
Attachment #634792 - Attachment is obsolete: true
Attachment #634792 - Flags: review?(bugs)
Attachment #634881 - Flags: review?(bugs)
Created attachment 634882 [details] [diff] [review]
part.8-8 widget should be able to speicify the scroll type (r=smaug)
Attachment #634793 - Attachment is obsolete: true
Created attachment 634883 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #634794 - Attachment is obsolete: true
Created attachment 634884 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests
Attachment #634796 - Attachment is obsolete: true
Attachment #634796 - Flags: review?(bugs)
Attachment #634884 - Flags: review?(bugs)
Created attachment 634885 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Attachment #634797 - Attachment is obsolete: true
Created attachment 634887 [details] [diff] [review]
part.11 Fix new test failures
Attachment #634798 - Attachment is obsolete: true
Created attachment 634888 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows
Attachment #634799 - Attachment is obsolete: true
Attachment #634799 - Flags: review?(bugs)
Attachment #634888 - Flags: review?(bugs)
Created attachment 634889 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK
Attachment #634800 - Attachment is obsolete: true
Created attachment 634890 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa
Attachment #634801 - Attachment is obsolete: true
Attachment #634801 - Flags: review?(bugs)
Attachment #634890 - Flags: review?(bugs)
Created attachment 634891 [details] [diff] [review]
part.15 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Qt
Attachment #634802 - Attachment is obsolete: true
Created attachment 634892 [details] [diff] [review]
part.16 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on OS/2
Attachment #634803 - Attachment is obsolete: true
Created attachment 634893 [details] [diff] [review]
part.17 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on nsWidgetUtils
Attachment #634804 - Attachment is obsolete: true
Attachment #634804 - Flags: review?(bugs)
Attachment #634893 - Flags: review?(bugs)
I filed a spec bug for the type of delta attributes:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=17628
Comment on attachment 634779 [details] [diff] [review]
part.4 Add reverting delta value prefs instead of customizing number of scroll lines prefs

So could you explain why we have
mousewheel.withaltkey.action, but the new stuff is 
mousewheel.with_alt.*

Also, can we remove .numlines without adding something similar back?
I thought people use that pretty often.
Perhaps we could have something like mousewheel.fookey.accelerationLevel.
It would be 1 by default. -1 would reverse the direction, 2 would double
the scrolling speed (-2 opposite direction)
Attachment #634779 - Flags: review?(bugs) → review-
Attachment #634893 - Flags: review?(bugs) → review+
Comment on attachment 634805 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event


>+#ifndef nsMouseScrollEvent_h__
>+#define nsMouseScrollEvent_h__

I don't like moving nsMouseScrollEvent to its own .h
nsMutationEvent.h is an exception, and it makes things just harder than they should be.
Attachment #634805 - Flags: review?(bugs) → review-
Comment on attachment 634879 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the wheel event isn't consumed

Why this way. I was thinking we should dispatch first the legacy events and then
if those aren't preventDefault'ed, dispatch wheel event.
That way for example addons could listen for wheel events only and handle them
knowing that content hasn't consumed the old legacy events.
Attachment #634879 - Flags: review?(bugs) → review-
Comment on attachment 634881 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent


>+  // Momentum events shouldn't run special actions.
>+  if (aEvent->isMomentum) {
Should this be 
!aEvent->isMomentum
Attachment #634881 - Flags: review?(bugs) → review+
Attachment #634884 - Flags: review?(bugs) → review+
Attachment #634881 - Flags: review+ → review-

Comment 153

5 years ago
(In reply to Olli Pettay [:smaug] from comment #151)
> Comment on attachment 634879 [details] [diff] [review]
> part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the
> wheel event isn't consumed
> 
> Why this way. I was thinking we should dispatch first the legacy events and
> then
> if those aren't preventDefault'ed, dispatch wheel event.
> That way for example addons could listen for wheel events only and handle
> them
> knowing that content hasn't consumed the old legacy events.

Most tactics I've seen for dealing with multiple events of the same type (legacy and new better events) involve registering handlers for all the events. And when the newer events are fired flipping a boolean that tells the code to ignore the legacy events.

If we dispatch the legacy events first and handle preventDefault that way we risk making code already written to handle wheel events react twice to the same scroll making things buggy. And if the app is coded to use a e.type == string instead of multiple booleans (easier way to handle the fact we now have 3 possible events) we risk making it only use legacy events instead of taking advantage of modern events.
Comment on attachment 634890 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa

I think some OSX widget peer should look at this too.
Attachment #634890 - Flags: review?(bugs) → review+
(In reply to Daniel Friesen from comment #153)
> Most tactics I've seen for dealing with multiple events of the same type
> (legacy and new better events)
Just curious which case have you in mind? I don't recall any other case when legacy event is being
deprecated this way.

> involve registering handlers for all the
> events.
Old code won't do that. And new code should just use the new events

 And when the newer events are fired flipping a boolean that tells
> the code to ignore the legacy events.
> 
> If we dispatch the legacy events first and handle preventDefault that way we
> risk making code already written to handle wheel events react twice to the
> same scroll making things buggy.
How so? If you handle the old events, you call preventDefault() and the listener for new wheel event doesn't get
called at all.
Comment on attachment 634888 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows

Jimm or bbondy or felipe should review this too.
Attachment #634888 - Flags: review?(bugs) → review+
And still to clarify, web page doesn't need to listen for legacy events if it does
the usual feature testing first.
if ("WheelEvent" in window) {
  // new event handling...
} else {
  // legacy
}

Comment 158

5 years ago
(In reply to Olli Pettay [:smaug] from comment #157)
> And still to clarify, web page doesn't need to listen for legacy events if
> it does
> the usual feature testing first.
> if ("WheelEvent" in window) {
>   // new event handling...
> } else {
>   // legacy
> }

I was worried about this kind of testing not working. Historically this has not been a very reliable way of testing for event existence since browsers are not consistent in whether they make this available or not. Notably Firefox has not placed these classes on window in the past. I guess this works in IE9 so I don't need to worry about that.

But doing some testing turned up a different issue with this plan. For some reason current versions of chrome have a window.WheelEvent present, but document.addEventListener('wheel', ..., false); does not fire any wheel events. So this kind of testing could lead to a site not functioning in Chrome.
I hope you have filed a bug on Chrome ;)
Hmm, in which way does IE dispatch wheel and mousewheel. We should probably use the same
order for wheel and DOMMouseScroll/MozPixelScroll.
...yet dispatching wheel first would effectively force chrome/addons use the legacy events.

One approach, if we need to dispatch wheel first, would be to dispatch legacy events
using nsDispatchingCallback. Then chrome could use system event group listeners for wheel.
Effectively events would be
wheel (in default group), legacy events (in default group), legacy events (in system group),  wheel (in system group)
(In reply to Olli Pettay [:smaug] from comment #149)
> Comment on attachment 634779 [details] [diff] [review]
> part.4 Add reverting delta value prefs instead of customizing number of
> scroll lines prefs
> 
> So could you explain why we have
> mousewheel.withaltkey.action, but the new stuff is 
> mousewheel.with_alt.*

The reasons for changing the naming rules of the prefs are:

1. "withnokey" doesn't make sense if we use them for two or modifier keys are pressed. "default" is clearer for such purpose.

2. by changing the naming rules, we don't worry about the conflict between older prefs and newer prefs. The older prefs are kept in user pref if they are customized. So, it would break future change if the developer uses same name accidentally.

3. and the change notices users who want to customize wheel behavior that the all prefs are reimplemented.

4. and users can keep the each setting for both in normal build and ESR.

> Also, can we remove .numlines without adding something similar back?

First, I think current approach is wrong at least after we support pixel scroll events because if we multiply the delta value by something come from prefs, it would work only when delta mode line and page. For example:

event1: pixel=3, line=1  ---->  pixel=6, line=2
event2: pixel=3, line=0         pixel=6, line=0
event3: pixel=3, line=0         pixel=6, line=0
event4: pixel=3, line=1         pixel=6, line=2
event5: pixel=3, line=0         pixel=6, line=0
event6: pixel=3, line=0         pixel=6, line=0

So, normal scrolling must work fine if the delta mode is pixel. However, I think that event1, event3, event4 and event6 should have line=1.

I was thinking that we should overwrite the delta values in widget level due to the native event differences. However, I still don't have an idea for sorting out this issue on MacOS X. If you have the idea and it can be implemented in ESM, we can implement the acceleration pref in the ESM.

(In reply to Olli Pettay [:smaug] from comment #150)
> Comment on attachment 634805 [details] [diff] [review]
> part.18 Clean up legacy mouse scroll event
> 
> 
> >+#ifndef nsMouseScrollEvent_h__
> >+#define nsMouseScrollEvent_h__
> 
> I don't like moving nsMouseScrollEvent to its own .h
> nsMutationEvent.h is an exception, and it makes things just harder than they
> should be.

Do you think that we should keep the event in nsGUIEvent.h? Then, I think that widget implementers might be confused by the difference between nsMouseScrollEvent and widget::WheelEvent. nsMouseScrollEvent is only used for the internal event of the legacy DOM events. So, I don't think that they should be defined in widget/.

(In reply to Olli Pettay [:smaug] from comment #151)
> Comment on attachment 634879 [details] [diff] [review]
> part.8-5 Dispatch legacy mouse scroll events from PostHandleEvent() if the
> wheel event isn't consumed
> 
> Why this way. I was thinking we should dispatch first the legacy events and
> then
> if those aren't preventDefault'ed, dispatch wheel event.
> That way for example addons could listen for wheel events only and handle
> them
> knowing that content hasn't consumed the old legacy events.

IE9 dispatches new "wheel" event, first. And then, dispatches legacy "mousewheel" events.

I think that this event order does make sense for web developers. Web developers may want to use "wheel" event for newer browsers. In such case, if we would dispatch legacy events first, they cannot know if wheel event would be fired, without UA check. Then, the path for older browsers would run for new Gecko browsers.

(In reply to Daniel Friesen from comment #153)
> Most tactics I've seen for dealing with multiple events of the same type
> (legacy and new better events) involve registering handlers for all the
> events. And when the newer events are fired flipping a boolean that tells
> the code to ignore the legacy events.

That's interesting, why don't you use preventDefault() for the first event if the first event is the most preferred event? Such approach doesn't work with other browsers?

> If we dispatch the legacy events first and handle preventDefault that way we
> risk making code already written to handle wheel events react twice to the
> same scroll making things buggy.

I think that it doesn't depend on the event order.

> And if the app is coded to use a e.type ==
> string instead of multiple booleans (easier way to handle the fact we now
> have 3 possible events) we risk making it only use legacy events instead of
> taking advantage of modern events.

Yeah, I assumed such implementations for deciding the order.

(In reply to Olli Pettay [:smaug] from comment #155)
> > involve registering handlers for all the
> > events.
> Old code won't do that. And new code should just use the new events
> 
>  And when the newer events are fired flipping a boolean that tells

That means such application ignores the DOM event set of first wheel event? At handling first DOM event, the application don't know if there is next event.

> > If we dispatch the legacy events first and handle preventDefault that way we
> > risk making code already written to handle wheel events react twice to the
> > same scroll making things buggy.
> How so? If you handle the old events, you call preventDefault() and the
> listener for new wheel event doesn't get
> called at all.

If the handler for legacy events have some limitations due to the difference of the event structure, our user may loose the better UX in such web application.

(In reply to Olli Pettay [:smaug] from comment #161)
> ...yet dispatching wheel first would effectively force chrome/addons use the
> legacy events.

I'm not sure what you meant here.

> One approach, if we need to dispatch wheel first, would be to dispatch
> legacy events
> using nsDispatchingCallback. Then chrome could use system event group
> listeners for wheel.
> Effectively events would be
> wheel (in default group), legacy events (in default group), legacy events
> (in system group),  wheel (in system group)

Hmm, I don't understand why we need to dispatch in the order.

(In reply to Olli Pettay [:smaug] from comment #157)
> And still to clarify, web page doesn't need to listen for legacy events if
> it does
> the usual feature testing first.
> if ("WheelEvent" in window) {
>   // new event handling...
> } else {
>   // legacy
> }

Awesome! I didn't have the idea.
(In reply to Olli Pettay [:smaug] from comment #152)
> Comment on attachment 634881 [details] [diff] [review]
> part.8-7 Implement the other default actions of WheelEvent
> 
> 
> >+  // Momentum events shouldn't run special actions.
> >+  if (aEvent->isMomentum) {
> Should this be 
> !aEvent->isMomentum

Why is that? If the event is caused by momentum scroll, it should cause only scroll or nothing.
Created attachment 639240 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)
Attachment #634776 - Attachment is obsolete: true
Created attachment 639241 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests (r=smaug)
Attachment #634884 - Attachment is obsolete: true
Attachment #639241 - Flags: superreview?(jst)
Created attachment 639242 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa (r=smaug)
Attachment #634890 - Attachment is obsolete: true
Created attachment 639243 [details] [diff] [review]
part.16 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on OS/2
Attachment #634892 - Attachment is obsolete: true
Created attachment 639244 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event

# Just updated for the latest m-c.
Attachment #634805 - Attachment is obsolete: true
I'm still thinking about the new pref design.

First, replacing delta values with fixed values doesn't make sense. That way kills some events which are caused by turning mouse wheel faster. And also it kills high resolution scrolling too.

These facts mean that the pref values should be multiplied by delta values.

Then, the values should be float, I think. It should be x100 values. Nobody need such resolution, but we're usually using x100 for float values in int prefs.

If the delta mode is pixel and the pref values are not 100 or -100, the delta accumulator should compute the lineOrPageDelta and replace the native event's value.

By this approach, it may be possible to implement in XP level.

Perhaps, the pref name should be .deltaMultiplier(X|Y|X)?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #162)
 
> > So could you explain why we have
> > mousewheel.withaltkey.action, but the new stuff is 
> > mousewheel.with_alt.*
> 
> The reasons for changing the naming rules of the prefs are:
> 
> 1. "withnokey" doesn't make sense if we use them for two or modifier keys
> are pressed. "default" is clearer for such purpose.
> 
> 2. by changing the naming rules, we don't worry about the conflict between
> older prefs and newer prefs. The older prefs are kept in user pref if they
> are customized. So, it would break future change if the developer uses same
> name accidentally.
But mousewheel.withaltkey.reverse_deltaX wouldn't conflict with old prefs.
I'd just like to have mousewheel.withXXX in one way, not in two different ways.

> First, I think current approach is wrong at least after we support pixel
> scroll events because if we multiply the delta value by something come from
> prefs, it would work only when delta mode line and page.
Right. But it could work for line scrolling. I just don't want to force people, who have got used to
fast scrolling to use slower scrolling.


> Do you think that we should keep the event in nsGUIEvent.h? 
Yes.

> Then, I think
> that widget implementers might be confused by the difference between
> nsMouseScrollEvent and widget::WheelEvent. nsMouseScrollEvent is only used
> for the internal event of the legacy DOM events. So, I don't think that they
> should be defined in widget/.
nsGUIEvent contains lots of DOM only stuff.
(We could and should get rid of those ns*Event structs but not in this bug.)


> IE9 dispatches new "wheel" event, first. And then, dispatches legacy
> "mousewheel" events.
Ok, then I think we need to dispatch legacy events using the dispatching callback so that
addons/chrome can still use wheel event (in system group)


> (In reply to Olli Pettay [:smaug] from comment #161)
> > ...yet dispatching wheel first would effectively force chrome/addons use the
> > legacy events.
> 
> I'm not sure what you meant here.
The problem is that chrome/addons need to know whether preventDefault was called.
So, if they use wheel event and it is dispatched before legacy events, chrome/addons can't know
whether preventDefault was called on the legacy events.



> 
> > One approach, if we need to dispatch wheel first, would be to dispatch
> > legacy events
> > using nsDispatchingCallback. Then chrome could use system event group
> > listeners for wheel.
> > Effectively events would be
> > wheel (in default group), legacy events (in default group), legacy events
> > (in system group),  wheel (in system group)
> 
> Hmm, I don't understand why we need to dispatch in the order.
Because of the problem to detect event.defaultPrevented.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #169)
> Perhaps, the pref name should be .deltaMultiplier(X|Y|X)?
Sounds ok.
Ok, then, I'll rewrite the patches. Thanks!
(In reply to Olli Pettay [:smaug] from comment #170)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #162)
>  
> > > So could you explain why we have
> > > mousewheel.withaltkey.action, but the new stuff is 
> > > mousewheel.with_alt.*
> > 
> > The reasons for changing the naming rules of the prefs are:
> > 
> > 1. "withnokey" doesn't make sense if we use them for two or modifier keys
> > are pressed. "default" is clearer for such purpose.
> > 
> > 2. by changing the naming rules, we don't worry about the conflict between
> > older prefs and newer prefs. The older prefs are kept in user pref if they
> > are customized. So, it would break future change if the developer uses same
> > name accidentally.
> But mousewheel.withaltkey.reverse_deltaX wouldn't conflict with old prefs.
> I'd just like to have mousewheel.withXXX in one way, not in two different
> ways.

Er, but the ".action" would conflict. Should we rename it to ".default_action"?
Hmm, which patch is adding mousewheel.with_alt.action ?
(So many patches here ;) )
(In reply to Olli Pettay [:smaug] from comment #174)
> Hmm, which patch is adding mousewheel.with_alt.action ?
> (So many patches here ;) )

See part.5 that removes the .horizontal and changes the meaning of the value.
https://bugzilla.mozilla.org/attachment.cgi?id=634780&action=diff
Ah, right. So after that change all the prefs are in form mousewheel.with_foo.x ?
That is good, and sorry that I missed it.
okay, no problem, the count of patches are too many for me too :-(
Attachment #634780 - Flags: review-
Attachment #634787 - Flags: review-

Updated

5 years ago
Attachment #639241 - Flags: superreview?(jst) → superreview+
Created attachment 639590 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)

Updated for bug 769190.
Attachment #639240 - Attachment is obsolete: true
Created attachment 639591 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_*

not tested yet.
Attachment #634779 - Attachment is obsolete: true
Created attachment 639593 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs
Attachment #634780 - Attachment is obsolete: true
Created attachment 639597 [details] [diff] [review]
part.6 Separate pixel delta value accumulation code (r=smaug)
Attachment #634781 - Attachment is obsolete: true
Attachment #634880 - Flags: review-
Created attachment 639600 [details] [diff] [review]
part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM (r=smaug)
Attachment #634783 - Attachment is obsolete: true
Comment on attachment 634787 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)

Er, 8-4 doesn't need to be reviewed again.
Attachment #634787 - Flags: review-
Created attachment 639604 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)
Attachment #634787 - Attachment is obsolete: true
Created attachment 639608 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event into system group if the wheel event isn't consumed

not tested yet.
Attachment #634879 - Attachment is obsolete: true
Created attachment 639614 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device or the delta values have been modified with prefs

not tested yet.
Attachment #634880 - Attachment is obsolete: true
Created attachment 639622 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent
Attachment #634881 - Attachment is obsolete: true
Created attachment 639639 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #634883 - Attachment is obsolete: true
Created attachment 639650 [details] [diff] [review]
part.8-11 Cancel applying delta multipliers from overflowDelta
Created attachment 640937 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)
Attachment #639590 - Attachment is obsolete: true
Comment on attachment 639591 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_*

We should support only faster setting (i.e., >= 1.0 or <= -1.0) for making simpler implementation for now.
Attachment #639591 - Flags: review?(bugs)
Comment on attachment 639593 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs

Using nsEventStateManager::WheelPrefs for managing action prefs. The actual behavior isn't changed from the previous (you reviewed) patch.
Attachment #639593 - Flags: review?(bugs)
Created attachment 640938 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)

Just adjusting the reviewed patch to the new patches.
Attachment #639604 - Attachment is obsolete: true
Created attachment 640939 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event into system group if the wheel event isn't consumed

The event order would be tested by the tests in part.10.
Attachment #639608 - Attachment is obsolete: true
Attachment #640939 - Flags: review?(bugs)
Created attachment 640943 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device or the delta values have been modified with prefs

This patch computes the lineOrPageDelta values when one or more of the multiplier settings is changed. Note that we shouldn't ignore the settings for the delta values because it may cause unexpected behavior. We should always compute both accumulated delta values.
Attachment #639614 - Attachment is obsolete: true
Attachment #640943 - Flags: review?(bugs)
Created attachment 640945 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent

I still think that you misunderstood at previous review (comment 152). This patch is easier to read than the previous patch (by the WheelPrefs class). Could you review this again?
Attachment #639622 - Attachment is obsolete: true
Attachment #640945 - Flags: review?(bugs)
Created attachment 640946 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #639639 - Attachment is obsolete: true
Created attachment 640947 [details] [diff] [review]
part.8-11 Cancel applying delta multipliers from overflowDelta

I think that we need to cancel the inflation of overflowDelta which is caused by multiplier prefs.
Attachment #639650 - Attachment is obsolete: true
Attachment #640947 - Flags: review?(bugs)
Created attachment 640950 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler

I gave up to fix test_bug350471.xul. But other new tests covers same things. So, we can just remove it.
Attachment #634885 - Attachment is obsolete: true
Attachment #640950 - Flags: review?(bugs)
Created attachment 640951 [details] [diff] [review]
part.11 Fix new test failures
Attachment #634887 - Attachment is obsolete: true
Attachment #640951 - Flags: review?(bugs)
Created attachment 640954 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows (r=smaug)

I'll request to review to jimm after other requested reviews.
Attachment #634888 - Attachment is obsolete: true
Created attachment 640956 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK
Attachment #634889 - Attachment is obsolete: true
Created attachment 640957 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event
Attachment #639244 - Attachment is obsolete: true
Attachment #640957 - Flags: review?(bugs)
Comment on attachment 640957 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event


>+/**
>+ * nsMouseScrollEvent is used for legacy DOM mouse scroll events, i.e.,
>+ * DOMMouseScroll and MozMousePixelScroll event.  These events are NOT hanbled
>+ * by ESM even if widget dispatches them.  Use new widget::WheelEvent instead.
s/ESM/nsEventStateManager/ That would be more clear for someone not too familiar with
event handling.
Attachment #640957 - Flags: review?(bugs) → review+
test build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-b617e4e53fea/
Blocks: 422132
Blocks: 659071
Created attachment 643298 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_*

Including the fix for bug 310003.
Attachment #639591 - Attachment is obsolete: true
Attachment #639591 - Flags: review?(bugs)
Attachment #643298 - Flags: review?(bugs)
Created attachment 643299 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs
Attachment #639593 - Attachment is obsolete: true
Attachment #639593 - Flags: review?(bugs)
Attachment #643299 - Flags: review?(bugs)
Created attachment 643300 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #640946 - Attachment is obsolete: true
Blocks: 310003
Smaug, could you review part.4 and part.5 ASAP? Then, the widget patches shouldn't be affected by other patches for ESM. So, I can request reviews to each widget reviewer.
Sorry about the delay. I'll review all the patches this weekend, starting from
part 4 and 5 today.
Argh, looks like tomorrow.
No problem, don't mind. I think that we need a week or more for reviews by widget people, so, I'd like you to review only the 4 and the 5 for now if you don't have much time (and I know that).
Comment on attachment 643298 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_*

>+    enum Index
>+    {
>+      INDEX_DEFAULT,
perhaps = 0,
Attachment #643298 - Flags: review?(bugs) → review+
Comment on attachment 643299 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs


>+
>+  NS_ERROR("Gets new wheel action pref value but it's not implemented yet.");

Perhaps just NS_WARNING
and something like
"Unsupported wheel action pref value!")

>+  nsCAutoString prefNameAction(basePrefName);
>+  prefNameAction.AppendLiteral("action");
>+  mActions[aIndex] =
>+    static_cast<Action>(Preferences::GetInt(prefNameAction.get(),
>+                                            ACTION_SCROLL));
>+  if (mActions[aIndex] < ACTION_NONE || mActions[aIndex] > ACTION_LAST) {
>+    mActions[aIndex] = ACTION_SCROLL;
Should we warn (NS_WARNING) in this case?
>+    enum Action
>+    {
>+      ACTION_NONE,
= 0
Attachment #643299 - Flags: review?(bugs) → review+
Thank you, I'll update the patches soon.
Comment on attachment 640954 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows (r=smaug)

D3E WheelEvent allows high resolution line or page scroll. Therefore, our mouse wheel handler for Windows becomes very simple!

1. We don't need to dispatch pixel scroll event. Therefore, we can remove the dispatcher code.

2. We don't need to get scroll target's information due to #1. So, we can remove the dispatcher of NS_QUERY_SCROLL_TARGET_INFO (MouseScrollHandler::GetScrollTargetInfo()).

And also, a wheel event can handle both X and Y direction scrolling. Therefore, nsWinGesture::PanDeltaToPixelScrollX() and nsWinGesture::PanDeltaToPixelScrollY() are merged to a method.

For compatibility, nsEventStateManager needs to dispatch DOMMouseScroll event. WheelEvent::lineOrPageDelta(X|Y) tell the timing and amount. Therefore, we're still computing the accumulated delta in MouseScrollHandler.

On the other hand, we cannot set lineOrPageDelta(X|Y) in nsWinGesture because there is no information for the line height. Therefore, it should be computed in nsEventStateManager. If WheelEvent::isPixelOnlyDevice is true, nsEventStateManager will do it. Therefore, only nsWinGesture sets true to isPixelOnlyDevice.

Note that the XP part has been reviewed by smaug already, so, you don't need to review the part if you don't familiar with it.
Attachment #640954 - Flags: review?(jmathies)
Comment on attachment 640956 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK

On Linux, native wheel event doesn't support high resolution scrolling. Therefore, the widget for Linux only sets same delta value to delta(X|Y) and lineOrPageDelta(X|Y).
Attachment #640956 - Flags: review?(karlt)
Comment on attachment 634891 [details] [diff] [review]
part.15 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Qt

Same as GTK.
Attachment #634891 - Flags: review?(karlt)
Comment on attachment 639243 [details] [diff] [review]
part.16 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on OS/2

From the change log of nsWindow for OS/2, I'm requesting the review to Dave Yeo. But if you don't familiar with this code, let me know.

I have never built this patch actually.

Looks like the native wheel event of OS/2 doesn't support high resolution scroll, so, similar to Linux, we need to set same delta value to delta(X|Y) and lineOrPageDelta(X|Y).
Attachment #639243 - Flags: review?(daveryeo)
Comment on attachment 639242 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa (r=smaug)

D3E WheelEvent's delta values can have the amount in either lines or pixels. So, we don't need to dispatch both line and pixel events. We need to set just WheelEvent::deltaMode to DOM_DELTA_LINE or DOM_DELTA_PIXEL.

And also, a D3E WheelEvent can have both X/Y delta values (i.e., supports oblique scroll). Therefore, the dispatcher sets both values at once.

WheelEvent::delta(X|Y) are the delta value which indicates the amount of scroll. On the other hand, lineOrPageDelta(X|Y) are used for dispatching legacy DOMMouseScroll events in nsEventStateManager. They must be set when we have dispatched NS_MOUSE_SCROLL event.

I've never tested the carbon event's part because I don't know which plugin supports mouse wheel scrolling on Mac. If you know it, I'll test it.

And note that the XP part has been reviewed by smaug already, so, you don't need to check it.
Attachment #639242 - Flags: review?(smichaud)
Comment on attachment 640956 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK

>+    if (!wheelEvent.deltaX && !wheelEvent.deltaY) {
>+        return;
>+    }

This shouldn't happen so no need to have this code in release builds.
Make this an assertion if you like.

>@@ -2747,30 +2762,34 @@ nsWindow::OnButtonPressEvent(GtkWidget *
>         break;
>     case 3:
>         domButton = nsMouseEvent::eRightButton;
>         break;
>     // These are mapped to horizontal scroll
>     case 6:
>     case 7:

Cases 6 and 7 shouldn't be reached.  They look like remnants from an ancient
GTK version (pre GTK+ 2.10 at least), so feel free to remove that code.
Attachment #640956 - Flags: review?(karlt) → review+
Attachment #634891 - Flags: review?(karlt) → review+
Created attachment 645536 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)
Attachment #640937 - Attachment is obsolete: true
Created attachment 645537 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_* (r=smaug)
Attachment #643298 - Attachment is obsolete: true
Created attachment 645539 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs (r=smaug)
Attachment #643299 - Attachment is obsolete: true
Created attachment 645540 [details] [diff] [review]
part.8-1 Drop legacy mouse scroll events from IPC (r=smaug)
Attachment #634784 - Attachment is obsolete: true
Created attachment 645542 [details] [diff] [review]
part.8-2 GetScrollAmount() should return both width and height for oblique wheel event (r=smaug)
Attachment #634785 - Attachment is obsolete: true
Created attachment 645544 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)
Attachment #640938 - Attachment is obsolete: true
Created attachment 645546 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event into system group if the wheel event isn't consumed
Attachment #640939 - Attachment is obsolete: true
Attachment #640939 - Flags: review?(bugs)
Attachment #645546 - Flags: review?(bugs)
Created attachment 645547 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device or the delta values have been modified with prefs
Attachment #640943 - Attachment is obsolete: true
Attachment #640943 - Flags: review?(bugs)
Attachment #645547 - Flags: review?(bugs)
Created attachment 645549 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent
Attachment #640945 - Attachment is obsolete: true
Attachment #640945 - Flags: review?(bugs)
Attachment #645549 - Flags: review?(bugs)
Created attachment 645551 [details] [diff] [review]
part.8-9 Handle WheelEvent.deltaZ in ESM (r=smaug)
Attachment #643300 - Attachment is obsolete: true
Created attachment 645555 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler
Attachment #640950 - Attachment is obsolete: true
Attachment #640950 - Flags: review?(bugs)
Attachment #645555 - Flags: review?(bugs)
Created attachment 645558 [details] [diff] [review]
part.13 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on GTK (r=karlt)
Attachment #640956 - Attachment is obsolete: true

Updated

5 years ago
Attachment #639243 - Flags: review?(daveryeo) → review+
Comment on attachment 639242 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa (r=smaug)

This looks fine to me (though I haven't tried to build or test it).  But I do have a couple of suggestions:

+#ifndef NP_NO_CARBON
+- (void)sendCarbonWheelEvent:(SInt)delta
+                     forAxis:(EventMouseWheelAxis)carbonAxis;
+#endif

+- (void)sendCarbonWheelEvent:(SInt)delta
+                     forAxis:(EventMouseWheelAxis)carbonAxis

Shouldn't "SInt" be "SInt32"?

+  // overflowDeltaX tells us when the user has tried to scroll past the edge
+  // of a page (in those cases it's non-zero).
+  // It only means left/right overflow when Gecko is processing a horizontal
+  // event.

I think this should be changed to something like:

"overflowDeltaX tells us when the user has tried to scroll past the
edge of a page to the left or the right (in those cases it's
non-zero)."
Attachment #639242 - Flags: review?(smichaud) → review+
Attachment #640951 - Flags: review?(bugs) → review+
Comment on attachment 645546 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event into system group if the wheel event isn't consumed


>+  nsEventStatus statusX = *aStatus;
>+  nsEventStatus statusY = *aStatus;
>+  if (scrollDeltaY) {
>+    SendLineScrollEvent(aTargetFrame, aEvent, &statusY,
>+                        scrollDeltaY, DELTA_DIRECTION_Y);
>+    if (!targetFrame.IsAlive()) {
>+      *aStatus = nsEventStatus_eConsumeNoDefault;
>+      return;
>+    }
>+  }
>+
>+  if (pixelDeltaY) {
>+    SendPixelScrollEvent(aTargetFrame, aEvent, &statusY,
>+                         pixelDeltaY, DELTA_DIRECTION_Y);
So same nsEventStatus variable is used for line and pixel scroll events.
Is that what happens currently too?
Attachment #645546 - Flags: review?(bugs) → review+
Attachment #645549 - Flags: review?(bugs) → review+
Hey, could you provide yet some new tryserver builds. I could test them for few days.
Comment on attachment 645555 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler

Make sure to re-trigger these tests few times on tryserver.
Attachment #645555 - Flags: review?(bugs) → review+
Comment on attachment 645547 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device or the delta values have been modified with prefs


>+  if (mHandlingDeltaMode != PR_UINT32_MAX) {
Could you add a comment what PR_UINT32_MAX value means here.
Attachment #645547 - Flags: review?(bugs) → review+
Attachment #640947 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #235)
> Comment on attachment 645546 [details] [diff] [review]
> part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event
> into system group if the wheel event isn't consumed
> 
> 
> >+  nsEventStatus statusX = *aStatus;
> >+  nsEventStatus statusY = *aStatus;
> >+  if (scrollDeltaY) {
> >+    SendLineScrollEvent(aTargetFrame, aEvent, &statusY,
> >+                        scrollDeltaY, DELTA_DIRECTION_Y);
> >+    if (!targetFrame.IsAlive()) {
> >+      *aStatus = nsEventStatus_eConsumeNoDefault;
> >+      return;
> >+    }
> >+  }
> >+
> >+  if (pixelDeltaY) {
> >+    SendPixelScrollEvent(aTargetFrame, aEvent, &statusY,
> >+                         pixelDeltaY, DELTA_DIRECTION_Y);
> So same nsEventStatus variable is used for line and pixel scroll events.
> Is that what happens currently too?

Yes. And I think that it's good behavior.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3316

Updated

5 years ago
Attachment #640954 - Flags: review?(jmathies) → review+
(In reply to Olli Pettay [:smaug] from comment #236)
> Hey, could you provide yet some new tryserver builds. I could test them for
> few days.

Same here!
(In reply to Jim Mathies [:jimm] from comment #240)
> (In reply to Olli Pettay [:smaug] from comment #236)
> > Hey, could you provide yet some new tryserver builds. I could test them for
> > few days.
> 
> Same here!

Here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-d7da8d83a2ef/
And I'm testing if there is random orange in the new tests.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d7da8d83a2ef
Created attachment 650054 [details] [diff] [review]
part.1 Add DOM3 WheelEvent (r=smaug, sr=jst)

Updated for latest trunk (including to replace nsnull with nullptr).
Attachment #645536 - Attachment is obsolete: true
Created attachment 650055 [details] [diff] [review]
part.2 Separate code for deciding scroll target from nsEventStateManager::DoScrollText() (r=smaug)
Attachment #634777 - Attachment is obsolete: true
Created attachment 650057 [details] [diff] [review]
part.3 Use ComputeScrollTarget() for deciding detail value of legacy mouse scroll events (r=smaug)
Attachment #634778 - Attachment is obsolete: true
Created attachment 650058 [details] [diff] [review]
part.4 Remove mousewheel.*.*numlines and add mousewheel.*.delta_multiplier_* (r=smaug)
Attachment #645537 - Attachment is obsolete: true
Created attachment 650059 [details] [diff] [review]
part.5 Redesign mouse wheel action prefs (r=smaug)
Attachment #645539 - Attachment is obsolete: true
Created attachment 650061 [details] [diff] [review]
part.6 Separate pixel delta value accumulation code (r=smaug)
Attachment #639597 - Attachment is obsolete: true
Created attachment 650062 [details] [diff] [review]
part.7 Drop NS_QUERY_SCROLL_TARGET_INFO handler from ESM (r=smaug)
Attachment #639600 - Attachment is obsolete: true
Created attachment 650064 [details] [diff] [review]
part.8-2 GetScrollAmount() should return both width and height for oblique wheel event (r=smaug)
Attachment #645542 - Attachment is obsolete: true
Created attachment 650065 [details] [diff] [review]
part.8-3 Redesign nsMouseWheelTransaction for D3E WheelEvent (r=smaug)
Attachment #634878 - Attachment is obsolete: true
Created attachment 650066 [details] [diff] [review]
part.8-4 Implement D3E WheelEvent handler for scroll (r=smaug)
Attachment #645544 - Attachment is obsolete: true
Created attachment 650067 [details] [diff] [review]
part.8-5 Dispatch legacy mouse scroll events before dispatching wheel event into system group if the wheel event isn't consumed (r=smaug)
Attachment #645546 - Attachment is obsolete: true
Created attachment 650070 [details] [diff] [review]
part.8-6 Init lineDeltaX and lineDeltaY from accumulated delta values if the wheel event is caused by pixel scroll only device or the delta values have been modified with prefs (r=smaug)
Attachment #645547 - Attachment is obsolete: true
Created attachment 650071 [details] [diff] [review]
part.8-7 Implement the other default actions of WheelEvent (r=smaug)
Attachment #645549 - Attachment is obsolete: true
Created attachment 650072 [details] [diff] [review]
part.8-8 widget should be able to speicify the scroll type (r=smaug)
Attachment #634882 - Attachment is obsolete: true
Created attachment 650073 [details] [diff] [review]
part.8-10 Remove the code handling legacy mouse events in layout (r=smaug)
Attachment #634795 - Attachment is obsolete: true
Created attachment 650074 [details] [diff] [review]
part.9 Implement nsIDOMWindowUtils::sendWheelEvent() for tests (r=smaug)
Attachment #639241 - Attachment is obsolete: true
Created attachment 650075 [details] [diff] [review]
part.10 Add new tests and remove tests for old ESM's legacy mouse scroll event handler (r=smaug)
Attachment #645555 - Attachment is obsolete: true
Created attachment 650076 [details] [diff] [review]
part.12 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Windows (r=smaug+jimm)
Attachment #640954 - Attachment is obsolete: true
Created attachment 650077 [details] [diff] [review]
part.14 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on Cocoa (r=smaug+smichaud)
Attachment #639242 - Attachment is obsolete: true
Created attachment 650080 [details] [diff] [review]
part.17 Replace legacy mouse scroll event dispatchers with D3E wheel event dispatcher on nsWidgetUtils (r=smaug)
Attachment #634893 - Attachment is obsolete: true
Created attachment 650081 [details] [diff] [review]
part.18 Clean up legacy mouse scroll event (r=smaug)
Attachment #640957 - Attachment is obsolete: true
The results of the tests fine:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d7da8d83a2ef
I don't land them today. If you want to test with the tryserver builds, please do it ASAP. If you're busy and you want to test it before landing, let me know. I don't mind to wait a couple of days.
Created attachment 650444 [details]
testcase #2

This test logs wheel, DOMMouseScroll and MozMousePixelScroll.
Could you wait for few days before landing, like until the end of this week.
(Sunday might be anyway a good day to land big stuff like this.)
Indeed.
https://hg.mozilla.org/mozilla-central/rev/61d8207c4fa3
https://hg.mozilla.org/mozilla-central/rev/d1f62bfbc5e4
https://hg.mozilla.org/mozilla-central/rev/514cd0d4702e
https://hg.mozilla.org/mozilla-central/rev/863b63c60cdb
https://hg.mozilla.org/mozilla-central/rev/6a4cc533c88b
https://hg.mozilla.org/mozilla-central/rev/9953c53532ab
https://hg.mozilla.org/mozilla-central/rev/2d5c36f4d6db
https://hg.mozilla.org/mozilla-central/rev/28a9293fb872
https://hg.mozilla.org/mozilla-central/rev/f8dc49fed352
https://hg.mozilla.org/mozilla-central/rev/745475f156d2
https://hg.mozilla.org/mozilla-central/rev/81a0791331da
https://hg.mozilla.org/mozilla-central/rev/8a59a17c195c
https://hg.mozilla.org/mozilla-central/rev/842d2eaa63bc
https://hg.mozilla.org/mozilla-central/rev/b9472dc9837c
https://hg.mozilla.org/mozilla-central/rev/c648f9903e58
https://hg.mozilla.org/mozilla-central/rev/db912be9bc19
https://hg.mozilla.org/mozilla-central/rev/baeed7c05786
https://hg.mozilla.org/mozilla-central/rev/9f71b2d92ca5
https://hg.mozilla.org/mozilla-central/rev/70f1befd3216
https://hg.mozilla.org/mozilla-central/rev/e978181b5940
https://hg.mozilla.org/mozilla-central/rev/738efdde4d4c
https://hg.mozilla.org/mozilla-central/rev/4dd145ea74d2
https://hg.mozilla.org/mozilla-central/rev/e9a626db6c69
https://hg.mozilla.org/mozilla-central/rev/a43a1923ccea
https://hg.mozilla.org/mozilla-central/rev/208dc5fa121b
https://hg.mozilla.org/mozilla-central/rev/99f8bbe053e2
https://hg.mozilla.org/mozilla-central/rev/07819a70a6a5
https://hg.mozilla.org/mozilla-central/rev/83eeedda95c2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
A follow-up bustage fix was also pushed because this was landed directly on m-c after bug 773842 had been landed on inbound which had enabled FAIL_ON_WARNINGS in a few directories in content/. Needless to say, the next merge lit the tree up in flames. Let this serve as yet another warning for why using inbound is recommended over landing directly on m-c.

https://hg.mozilla.org/mozilla-central/rev/df8bdd5813f0
Keywords: dev-doc-needed

Updated

5 years ago
Blocks: 467530

Updated

5 years ago
Blocks: 782143

Updated

5 years ago
No longer blocks: 467530
Depends on: 782175
Depends on: 782181
Depends on: 782190
documents:

https://developer.mozilla.org/en-US/docs/DOM/WheelEvent
https://developer.mozilla.org/en-US/docs/en-US/DOM/DOM_event_reference/wheel
https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Preferences_for_customizing_delta_values_and_default_action
Cool!

A little feedback:

On OSX where the scroll is inverted ('kinetic scrolling'), the webkit wheelevent event gives a inverted deltaY value where the Firefox wheel event doesn't.

I think the Firefox behavior is the correct one but it might need to be discussed.

What do you think?
Actually maybe not because there is no way to know whether the user configuration is classic or kinetic scrolling. So giving the delta according to the scrolling direction might be the correct behavior.

My use case is handling the scrolling of an area myself. (so I can hide the scrollbars)
Sonny, thanks for testing. Could you please file a new bug about that OSX issue.
Make that bug block this one. CC me and masauyki.

Updated

5 years ago
Depends on: 782583
Depends on: 782552
Depends on: 782739
Depends on: 782853
updated:

https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference
https://developer.mozilla.org/en-US/docs/Gecko-Specific_DOM_Events
https://developer.mozilla.org/en-US/docs/DOM/element
https://developer.mozilla.org/en-US/docs/DOM/WheelEvent
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/wheel

added:

https://developer.mozilla.org/en-US/docs/DOM/MouseWheelEvent
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/mousewheel
https://developer.mozilla.org/en-US/docs/DOM/MouseScrollEvent
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/DOMMouseScroll
https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/MozMousePixelScroll
https://developer.mozilla.org/en-US/docs/DOM/element.onwheel
Depends on: 674477
Depends on: 786120
FYI: The type of deltaX, deltaY and deltaZ are now defined as double in the latest draft.
Depends on: 788401

Updated

5 years ago
Blocks: 741968

Updated

5 years ago
Depends on: 801101
Depends on: 813074
No longer depends on: 813074
Depends on: 814303

Updated

5 years ago
Depends on: 815415

Comment 277

5 years ago
And how do i scroll a page up/down using mouse now?
(In reply to chAlx from comment #277)
> And how do i scroll a page up/down using mouse now?

If you set delta_multiplier less than 100000 and a native wheel event's scroll amount is over one page, then, the actual scroll amount is limilted to one page. See bug 801101 for more detail.
No longer depends on: 815415

Comment 279

5 years ago
Noticed a few typos in this article:
https://developer.mozilla.org/en-US/docs/Mozilla_event_reference/wheel

implemenation instead of implementation (deltaX, deltaY, deltaZ)
deltaX	float	Expected amount that the page will scroll along the x-axis according to the deltaMode units; or an implemenation-specific value of movement of a wheel around the x-axis. Read Only.

consective should probably be consecutive:
# A vertical DOMMouseScroll event in both event group if accumulated deltaY of consective wheel events become larger than 1.0 or less than -1.0
# A vertical MozMousePixelScroll event in both event group if deltaY of the latest wheel event isn't zero
# A horizontal DOMMouseScroll event in both event group if accumulated deltaX of consective wheel events become larger than 1.0 or less than -1.0
# A horizontal MozMousePixelScroll event in both event group if deltaX of the latest wheel event isn't zero
Fixed, thanks.
Depends on: 848628
Depends on: 829952

Comment 281

4 years ago
Masayuki, could you please comment your choice to prevent forced convertion lines->pixels (so, deltaMode would always be DOM_DELTA_PIXEL) here - http://code.google.com/p/chromium/issues/detail?id=227454 ?
(Personally I need DOM_DELTA_LINE when available, but there are some cases we need pixels (eg custom scrollbars))
It would be great if you share your thoughts about discussed issue.
(In reply to danya.postfactum from comment #281)
> Masayuki, could you please comment your choice to prevent forced convertion
> lines->pixels (so, deltaMode would always be DOM_DELTA_PIXEL) here -
> http://code.google.com/p/chromium/issues/detail?id=227454 ?
> (Personally I need DOM_DELTA_LINE when available, but there are some cases
> we need pixels (eg custom scrollbars))
> It would be great if you share your thoughts about discussed issue.

Browsers should provide loss-less information to web applications.

If web applications need to convert line or page to pixels, then, they should define its line-height, page-width or page-height.
Doc updated
https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent (and subpages)
and
https://developer.mozilla.org/en-US/Firefox/Releases/17#DOM

I indicated clearly that these are deprecated (and what to use):
https://developer.mozilla.org/en-US/docs/Web/API/MouseWheelEvent
https://developer.mozilla.org/en-US/docs/Web/API/MouseScrollEvent
https://developer.mozilla.org/en-US/docs/Web/Events/mousewheel
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1121480
Blocks: 1191293

Updated

7 months ago
Depends on: 1328064
You need to log in before you can comment on or make changes to this bug.